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 */

Reply via email to