On Tue, Dec 20, 2016 at 01:55:03PM +0100, Martin Pieuchot wrote: > rzalamena@ found that the IPv4 multicast code have two code paths ending > in ip_output() that are not yet taking the NET_LOCK: sosetopt() and > pffasttimo().
I see this also on my regress test machine. splassert: ip_output: want 1 have 0 Starting stack trace... ip_output(1,0,d09e32c1,d97702ec,0) at ip_output+0x6d ip_output(d8edd000,d9804200,0,0,f57a2d30) at ip_output+0x6d igmp_sendpkt(d384c030,d76fc1c0,16,0,0) at igmp_sendpkt+0x119 igmp_joingroup(d76fc1c0,80206931,f57a2dac,f57a2dc8,0) at igmp_joingroup+0x79 in_addmulti(d8edda14,d384c030,0,d03cc600,d8edda08) at in_addmulti+0x139 ip_setmoptions(c,d8dcebbc,d8edda00,0,d0b56f38) at ip_setmoptions+0x54d ip_ctloutput(1,d8e4d6a0,0,c,f57a2eac) at ip_ctloutput+0x577 sosetopt(d8e4d6a0,0,c,d8edda00,d0bbe320) at sosetopt+0x7e sys_setsockopt(d973fe28,f57a2f5c,f57a2f7c,0,286) at sys_setsockopt+0x14f syscall() at syscall+0x250 splassert: ip_output: want 1 have 0 Starting stack trace... ip_output(1,0,d09e32c1,d3831980,f52eed0c) at ip_output+0x6d ip_output(d8eda300,d9804200,0,0,f52eed78) at ip_output+0x6d igmp_sendpkt(d3701c00,d3d14240,16,0,da7a0000) at igmp_sendpkt+0x119 igmp_checktimer(d3701c00,d09e297d,d0952d9e,8541cb9c,80051622) at igmp_checktimer+0x7e igmp_fasttimo(40,f52eee60,30,8541cb9c,80051622) at igmp_fasttimo+0x4a pffasttimo(d0b7b624,f52eee60,f52eee40,d02031b9,200282) at pffasttimo+0x39 timeout_run(d0b7b624,f52eee90,d03a04e4,1d,f52eeecc) at timeout_run+0x40 softclock(0,f52eeeb0,d0866c11,d0c0256c,d0bbe320) at softclock+0x117 softintr_dispatch(0) at softintr_dispatch+0x5f Xsoftclock() at Xsoftclock+0x12 > Diff below fixes that and include pfslowtimo() as well as sogetopt() for > completeness. OK bluhm@ > While here remove a superfluous splsoftnet() which doesn't protect > anything in the SO_SPLICE case since we're running the input path > in a thread. The sosplice thread is only used for TCP. UDP runs in the softnet thread. As we are reading 64 bit values, we need some sort of locking. Now everything is in the kernel lock, but when we start working to unlock the socket layer, be need a flag that something has to be done here. Can we keep the splsoftnet() for that? Or replace it with a NET_LOCK()? bluhm > Index: kern/uipc_domain.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_domain.c,v > retrieving revision 1.46 > diff -u -p -r1.46 uipc_domain.c > --- kern/uipc_domain.c 22 Nov 2016 10:32:31 -0000 1.46 > +++ kern/uipc_domain.c 20 Dec 2016 10:18:50 -0000 > @@ -99,8 +99,8 @@ domaininit(void) > max_linkhdr = 64; > > max_hdr = max_linkhdr + max_protohdr; > - timeout_set(&pffast_timeout, pffasttimo, &pffast_timeout); > - timeout_set(&pfslow_timeout, pfslowtimo, &pfslow_timeout); > + timeout_set_proc(&pffast_timeout, pffasttimo, &pffast_timeout); > + timeout_set_proc(&pfslow_timeout, pfslowtimo, &pfslow_timeout); > timeout_add(&pffast_timeout, 1); > timeout_add(&pfslow_timeout, 1); > } > @@ -238,13 +238,13 @@ pfslowtimo(void *arg) > struct protosw *pr; > int i, s; > > - s = splsoftnet(); > + NET_LOCK(s); > for (i = 0; (dp = domains[i]) != NULL; i++) { > for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++) > if (pr->pr_slowtimo) > (*pr->pr_slowtimo)(); > } > - splx(s); > + NET_UNLOCK(s); > timeout_add_msec(to, 500); > } > > @@ -256,12 +256,12 @@ pffasttimo(void *arg) > struct protosw *pr; > int i, s; > > - s = splsoftnet(); > + NET_LOCK(s); > for (i = 0; (dp = domains[i]) != NULL; i++) { > for (pr = dp->dom_protosw; pr < dp->dom_protoswNPROTOSW; pr++) > if (pr->pr_fasttimo) > (*pr->pr_fasttimo)(); > } > - splx(s); > + NET_UNLOCK(s); > timeout_add_msec(to, 200); > } > Index: kern/uipc_socket.c > =================================================================== > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > retrieving revision 1.169 > diff -u -p -r1.169 uipc_socket.c > --- kern/uipc_socket.c 19 Dec 2016 08:36:49 -0000 1.169 > +++ kern/uipc_socket.c 20 Dec 2016 10:22:18 -0000 > @@ -1555,10 +1555,10 @@ sosetopt(struct socket *so, int level, i > > if (level != SOL_SOCKET) { > if (so->so_proto && so->so_proto->pr_ctloutput) { > - s = splsoftnet(); > + NET_LOCK(s); > error = (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so, > level, optname, &m0); > - splx(s); > + NET_UNLOCK(s); > return (error); > } > error = ENOPROTOOPT; > @@ -1702,10 +1702,10 @@ sosetopt(struct socket *so, int level, i > struct domain *dom = so->so_proto->pr_domain; > > level = dom->dom_protosw->pr_protocol; > - s = splsoftnet(); > + NET_LOCK(s); > error = (*so->so_proto->pr_ctloutput) > (PRCO_SETOPT, so, level, optname, &m0); > - splx(s); > + NET_UNLOCK(s); > return (error); > } > error = ENOPROTOOPT; > @@ -1734,10 +1734,10 @@ sosetopt(struct socket *so, int level, i > break; > } > if (error == 0 && so->so_proto && so->so_proto->pr_ctloutput) { > - s = splsoftnet(); > + NET_LOCK(s); > (*so->so_proto->pr_ctloutput)(PRCO_SETOPT, so, > level, optname, &m0); > - splx(s); > + NET_UNLOCK(s); > m = NULL; /* freed by protocol */ > } > } > @@ -1755,10 +1755,10 @@ sogetopt(struct socket *so, int level, i > > if (level != SOL_SOCKET) { > if (so->so_proto && so->so_proto->pr_ctloutput) { > - s = splsoftnet(); > + NET_LOCK(s); > error = (*so->so_proto->pr_ctloutput)(PRCO_GETOPT, so, > level, optname, mp); > - splx(s); > + NET_UNLOCK(s); > return (error); > } else > return (ENOPROTOOPT); > @@ -1839,10 +1839,10 @@ sogetopt(struct socket *so, int level, i > struct domain *dom = so->so_proto->pr_domain; > > level = dom->dom_protosw->pr_protocol; > - s = splsoftnet(); > + NET_LOCK(s); > error = (*so->so_proto->pr_ctloutput) > (PRCO_GETOPT, so, level, optname, mp); > - splx(s); > + NET_UNLOCK(s); > return (error); > } > return (ENOPROTOOPT); > @@ -1852,12 +1852,10 @@ sogetopt(struct socket *so, int level, i > case SO_SPLICE: > { > off_t len; > - int s = splsoftnet(); > > m->m_len = sizeof(off_t); > len = so->so_sp ? so->so_sp->ssp_len : 0; > memcpy(mtod(m, off_t *), &len, sizeof(off_t)); > - splx(s); > break; > } > #endif /* SOCKET_SPLICE */