Date: Mon, 31 Jul 2023 11:35:12 +0200 From: tuexen@freebsd.org To: Dmitry Chagin <dchagin@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: c620788150d2 - main - sctp: keep sb_acc and sb_ccc in sync Message-ID: <B203807A-6815-49E3-A8E8-0A65731DC76B@freebsd.org> In-Reply-To: <ZMd0fmX-XvNf3RSd@heemeyer.club> References: <202307282217.36SMHSJD073301@gitrepo.freebsd.org> <ZMd0fmX-XvNf3RSd@heemeyer.club>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 31. Jul 2023, at 10:44, Dmitry Chagin <dchagin@freebsd.org> wrote: >=20 > On Fri, Jul 28, 2023 at 10:17:28PM +0000, Michael Tuexen wrote: >> The branch main has been updated by tuexen: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3Dc620788150d274c09a070ab486602c98= 407d73b0 >>=20 >> commit c620788150d274c09a070ab486602c98407d73b0 >> Author: Michael Tuexen <tuexen@FreeBSD.org> >> AuthorDate: 2023-07-28 13:16:23 +0000 >> Commit: Michael Tuexen <tuexen@FreeBSD.org> >> CommitDate: 2023-07-28 13:16:23 +0000 >>=20 >> sctp: keep sb_acc and sb_ccc in sync >>=20 >> PR: 260116 >> MFC after: 1 week >> --- >=20 > Hi, Michael, > perhaps some of the recent fixes have broken these tests: > = https://ci.freebsd.org/view/all/job/FreeBSD-main-amd64-test/lastFailedBuil= d/console Hi Dmitry, part of the change is that the macro is now checking for underflows. = This seems to trigger (Counter goes negative). I guess the patch is not breaking any = tests, but highlighting a problem. Will look into it. Best regards Michael >=20 > 02:04:05 sys/netpfil/pf/sctp:basic_v4 -> epair1a: Ethernet address: = 02:b9:89:4e:84:0a > 02:04:05 epair1b: Ethernet address: 02:b9:89:4e:84:0b > 02:04:05 epair1a: link state changed to UP > 02:04:05 epair1b: link state changed to UP > 02:04:06 panic: Counter goes negative > 02:04:06 cpuid =3D 1 > 02:04:06 time =3D 1690758247 > 02:04:06 KDB: stack backtrace: > 02:04:06 db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame = 0xfffffe00a0775480 > 02:04:06 vpanic() at vpanic+0x149/frame 0xfffffe00a07754d0 > 02:04:06 panic() at panic+0x43/frame 0xfffffe00a0775530 > 02:04:06 sctp_abort_notification() at sctp_abort_notification/frame = 0xfffffe00a0775540 > 02:04:06 sctp_express_handle_sack() at = sctp_express_handle_sack+0x647/frame 0xfffffe00a0775640 > 02:04:06 sctp_process_control() at sctp_process_control+0xf62/frame = 0xfffffe00a0775990 > 02:04:06 sctp_common_input_processing() at = sctp_common_input_processing+0x561/frame 0xfffffe00a0775b10 > 02:04:06 sctp_input_with_port() at sctp_input_with_port+0x1fa/frame = 0xfffffe00a0775be0 > 02:04:06 sctp_input() at sctp_input+0x10/frame 0xfffffe00a0775bf0 > 02:04:06 ip_input() at ip_input+0x2ab/frame 0xfffffe00a0775c50 > 02:04:06 netisr_dispatch_src() at netisr_dispatch_src+0xad/frame = 0xfffffe00a0775cb0 > 02:04:06 ether_demux() at ether_demux+0x17a/frame 0xfffffe00a0775ce0 > 02:04:06 ether_nh_input() at ether_nh_input+0x39f/frame = 0xfffffe00a0775d30 > 02:04:06 netisr_dispatch_src() at netisr_dispatch_src+0xad/frame = 0xfffffe00a0775d90 > 02:04:06 ether_input() at ether_input+0xd9/frame 0xfffffe00a0775df0 > 02:04:06 epair_tx_start_deferred() at = epair_tx_start_deferred+0xd7/frame 0xfffffe00a0775e40 > 02:04:06 taskqueue_run_locked() at taskqueue_run_locked+0xab/frame = 0xfffffe00a0775ec0 > 02:04:06 taskqueue_thread_loop() at taskqueue_thread_loop+0xd3/frame = 0xfffffe00a0775ef0 > 02:04:06 fork_exit() at fork_exit+0x82/frame 0xfffffe00a0775f30 > 02:04:06 fork_trampoline() at fork_trampoline+0xe/frame = 0xfffffe00a0775f30 > 02:04:06 --- trap 0xc, rip =3D 0xf04bf926c9a, rsp =3D 0xf04c04a4f48, = rbp =3D 0xf04c04a4f60 --- >=20 >=20 >> sys/netinet/sctp_os_bsd.h | 18 ++++++++++++------ >> sys/netinet/sctp_output.c | 2 +- >> sys/netinet/sctp_pcb.c | 2 +- >> sys/netinet/sctp_var.h | 4 ++-- >> sys/netinet/sctputil.c | 4 ++-- >> sys/netinet/sctputil.h | 4 ++-- >> 6 files changed, 20 insertions(+), 14 deletions(-) >>=20 >> diff --git a/sys/netinet/sctp_os_bsd.h b/sys/netinet/sctp_os_bsd.h >> index 80c187b3c5ef..0d88377523c8 100644 >> --- a/sys/netinet/sctp_os_bsd.h >> +++ b/sys/netinet/sctp_os_bsd.h >> @@ -372,11 +372,6 @@ typedef struct callout sctp_os_timer_t; >> #define SCTP_CLEAR_SO_NBIO(so) ((so)->so_state &=3D ~SS_NBIO) >> /* get the socket type */ >> #define SCTP_SO_TYPE(so) ((so)->so_type) >> -/* Use a macro for renaming sb_cc to sb_acc. >> - * Initially sb_ccc was used, but this broke select() when used >> - * with SCTP sockets. >> - */ >> -#define sb_cc sb_acc >> /* reserve sb space for a socket */ >> #define SCTP_SORESERVE(so, send, recv) soreserve(so, send, recv) >> /* wakeup a socket */ >> @@ -384,8 +379,19 @@ typedef struct callout sctp_os_timer_t; >> /* number of bytes ready to read */ >> #define SCTP_SBAVAIL(sb) sbavail(sb) >> /* clear the socket buffer state */ >> +#define SCTP_SB_INCR(sb, incr) \ >> +{ \ >> + atomic_add_int(&(sb)->sb_acc, incr); \ >> + atomic_add_int(&(sb)->sb_ccc, incr); \ >> +} >> +#define SCTP_SB_DECR(sb, decr) \ >> +{ \ >> + SCTP_SAVE_ATOMIC_DECREMENT(&(sb)->sb_acc, decr); \ >> + SCTP_SAVE_ATOMIC_DECREMENT(&(sb)->sb_ccc, decr); \ >> +} >> #define SCTP_SB_CLEAR(sb) \ >> - (sb).sb_cc =3D 0; \ >> + (sb).sb_acc =3D 0; \ >> + (sb).sb_ccc =3D 0; \ >> (sb).sb_mb =3D NULL; \ >> (sb).sb_mbcnt =3D 0; >>=20 >> diff --git a/sys/netinet/sctp_output.c b/sys/netinet/sctp_output.c >> index a146ad401b30..7e32cc662f38 100644 >> --- a/sys/netinet/sctp_output.c >> +++ b/sys/netinet/sctp_output.c >> @@ -7290,7 +7290,7 @@ one_more_time: >> if ((stcb->sctp_socket !=3D NULL) && >> ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) || >> (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL))) { >> - atomic_subtract_int(&stcb->sctp_socket->so_snd.sb_cc, sp->length); >> + SCTP_SB_DECR(&stcb->sctp_socket->so_snd, sp->length); >> } >> if (sp->data) { >> sctp_m_freem(sp->data); >> diff --git a/sys/netinet/sctp_pcb.c b/sys/netinet/sctp_pcb.c >> index 9df2f82bdaa6..220d8b72667d 100644 >> --- a/sys/netinet/sctp_pcb.c >> +++ b/sys/netinet/sctp_pcb.c >> @@ -3612,7 +3612,7 @@ sctp_inpcb_free(struct sctp_inpcb *inp, int = immediate, int from) >> TAILQ_REMOVE(&inp->read_queue, sq, next); >> sctp_free_remote_addr(sq->whoFrom); >> if (so) >> - so->so_rcv.sb_cc -=3D sq->length; >> + SCTP_SB_DECR(&so->so_rcv, sq->length); >> if (sq->data) { >> sctp_m_freem(sq->data); >> sq->data =3D NULL; >> diff --git a/sys/netinet/sctp_var.h b/sys/netinet/sctp_var.h >> index 8301ec38cf6e..ef01b0b6c3bd 100644 >> --- a/sys/netinet/sctp_var.h >> +++ b/sys/netinet/sctp_var.h >> @@ -198,7 +198,7 @@ extern struct protosw sctp_seqpacket_protosw, = sctp_stream_protosw; >> } >>=20 >> #define sctp_sbfree(ctl, stcb, sb, m) { \ >> - SCTP_SAVE_ATOMIC_DECREMENT(&(sb)->sb_cc, SCTP_BUF_LEN((m))); \ >> + SCTP_SB_DECR(sb, SCTP_BUF_LEN((m))); \ >> SCTP_SAVE_ATOMIC_DECREMENT(&(sb)->sb_mbcnt, MSIZE); \ >> if (((ctl)->do_not_ref_stcb =3D=3D 0) && stcb) {\ >> SCTP_SAVE_ATOMIC_DECREMENT(&(stcb)->asoc.sb_cc, SCTP_BUF_LEN((m))); \ >> @@ -210,7 +210,7 @@ extern struct protosw sctp_seqpacket_protosw, = sctp_stream_protosw; >> } >>=20 >> #define sctp_sballoc(stcb, sb, m) { \ >> - atomic_add_int(&(sb)->sb_cc,SCTP_BUF_LEN((m))); \ >> + SCTP_SB_INCR(sb, SCTP_BUF_LEN((m))); \ >> atomic_add_int(&(sb)->sb_mbcnt, MSIZE); \ >> if (stcb) { \ >> atomic_add_int(&(stcb)->asoc.sb_cc, SCTP_BUF_LEN((m))); \ >> diff --git a/sys/netinet/sctputil.c b/sys/netinet/sctputil.c >> index ef8d763efabf..a4d4e806c00e 100644 >> --- a/sys/netinet/sctputil.c >> +++ b/sys/netinet/sctputil.c >> @@ -5044,7 +5044,7 @@ sctp_free_bufspace(struct sctp_tcb *stcb, = struct sctp_association *asoc, >> if ((stcb->sctp_socket !=3D NULL) && >> (((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL)) || >> ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE)))) { >> - SCTP_SAVE_ATOMIC_DECREMENT(&stcb->sctp_socket->so_snd.sb_cc, = tp1->book_size); >> + SCTP_SB_DECR(&stcb->sctp_socket->so_snd, tp1->book_size); >> } >> } >>=20 >> @@ -6121,7 +6121,7 @@ get_more_data: >> if (SCTP_BASE_SYSCTL(sctp_logging_level) & SCTP_SB_LOGGING_ENABLE) { >> sctp_sblog(&so->so_rcv, control->do_not_ref_stcb ? NULL : stcb, = SCTP_LOG_SBFREE, (int)cp_len); >> } >> - SCTP_SAVE_ATOMIC_DECREMENT(&so->so_rcv.sb_cc, (int)cp_len); >> + SCTP_SB_DECR(&so->so_rcv, (int)cp_len); >> if ((control->do_not_ref_stcb =3D=3D 0) && >> stcb) { >> atomic_subtract_int(&stcb->asoc.sb_cc, (int)cp_len); >> diff --git a/sys/netinet/sctputil.h b/sys/netinet/sctputil.h >> index 60b5661c309c..d74780cdccf7 100644 >> --- a/sys/netinet/sctputil.h >> +++ b/sys/netinet/sctputil.h >> @@ -253,7 +253,7 @@ do { \ >> } \ >> if (stcb->sctp_socket && ((stcb->sctp_ep->sctp_flags & = SCTP_PCB_FLAGS_TCPTYPE) || \ >> (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL))) { \ >> - SCTP_SAVE_ATOMIC_DECREMENT(&stcb->sctp_socket->so_snd.sb_cc, = sp->length); \ >> + SCTP_SB_DECR(&stcb->sctp_socket->so_snd, sp->length); \ >> } \ >> } \ >> } while (0) >> @@ -264,7 +264,7 @@ do { \ >> if ((stcb->sctp_socket !=3D NULL) && \ >> ((stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) || \ >> (stcb->sctp_ep->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL))) { \ >> - atomic_add_int(&stcb->sctp_socket->so_snd.sb_cc,sz); \ >> + SCTP_SB_INCR(&stcb->sctp_socket->so_snd, sz); \ >> } \ >> } while (0) >>=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B203807A-6815-49E3-A8E8-0A65731DC76B>