Re: more NET_LOCK in pppx

[David Gwynne]

ok.

> On 28 Mar 2018, at 10:07, Jonathan Matthew  wrote:
> 
> Reading through if_pppx.c I spotted a couple of places we need NET_LOCK().
> 
> ok?
> 
> Index: if_pppx.c
> ===
> RCS file: /cvs/src/sys/net/if_pppx.c,v
> retrieving revision 1.63
> diff -u -p -u -p -r1.63 if_pppx.c
> --- if_pppx.c 12 Aug 2017 20:27:28 -  1.63
> +++ if_pppx.c 27 Mar 2018 23:56:56 -
> @@ -392,6 +392,8 @@ pppxwrite(dev_t dev, struct uio *uio, in
>   proto = ntohl(*(uint32_t *)(th + 1));
>   m_adj(top, sizeof(uint32_t));
> 
> + NET_LOCK();
> +
>   switch (proto) {
>   case AF_INET:
>   ipv4_input(>pxi_if, top);
> @@ -403,9 +405,12 @@ pppxwrite(dev_t dev, struct uio *uio, in
> #endif
>   default:
>   m_freem(top);
> - return (EAFNOSUPPORT);
> + error = EAFNOSUPPORT;
> + break;
>   }
> 
> + NET_UNLOCK();
> +
>   return (error);
> }
> 
> @@ -583,8 +588,10 @@ pppxclose(dev_t dev, int flags, int mode
>   pxd = pppx_dev_lookup(dev);
> 
>   /* XXX */
> + NET_LOCK();
>   while ((pxi = LIST_FIRST(>pxd_pxis)))
>   pppx_if_destroy(pxd, pxi);
> + NET_UNLOCK();
> 
>   LIST_REMOVE(pxd, pxd_entry);
> 
> 

Re: More NET_LOCK()

[Martin Pieuchot]

On 20/12/16(Tue) 19:51, Alexander Bluhm wrote:
> [...] 
> 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()?

I think that we should keep the code as simple as possible until we
know for sure what is the next step.  In other words if the KERNEL_LOCK
is what's needed then just keep that.

I don't want to put a NET_LOCK() when it is not needed.  Because I know
that a future step will be to shrink/remove the NET_LOCK().  So I'd
rather not make this task more complicated than it is already.

When we start working to unlock the socket layer, and we should, the
person that will lead the effort will come with a solution.  All the
fields will have to be audited anyway, so I'm not too worried.

Anyway I left it for the moment.

Re: More NET_LOCK()

[Alexander Bluhm]

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,da7a) 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.c22 Nov 2016 10:32:31 -  1.46
> +++ kern/uipc_domain.c20 Dec 2016 10:18:50 -
> @@ -99,8 +99,8 @@ domaininit(void)
>   max_linkhdr = 64;
>  
>   max_hdr = max_linkhdr + max_protohdr;
> - timeout_set(_timeout, pffasttimo, _timeout);
> - timeout_set(_timeout, pfslowtimo, _timeout);
> + timeout_set_proc(_timeout, pffasttimo, _timeout);
> + timeout_set_proc(_timeout, pfslowtimo, _timeout);
>   timeout_add(_timeout, 1);
>   timeout_add(_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.c19 Dec 2016 08:36:49 -  1.169
> +++ kern/uipc_socket.c20 Dec 2016 10:22:18 -
> @@ -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, );
> - 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;
>  
>