On 06/07/20(Mon) 16:42, Vitaliy Makkoveev wrote: > [...] > pipex(4) is simultaneously locked by NET_LOCK() and KERNEL_LOCK() but > with two exceptions: > > 1. As you pointed pipex_pppoe_input() called without KERNEL_LOCK() held. > 2. pppac_start() called without NET_LOCK() held. Or with NET_LOCK() > held. It depends on `if_snd' usage. > > Diff below enforces pppac_start() to be called with NET_LOCK() held. > Also all externally called pipex(4) input and output routines have > NET_ASSERT_LOCKED() assertion. > > Now pipex(4) is fully protected by NET_LOCK() so description of struct > members chenget too. > > Index: sys/net/if_pppx.c > =================================================================== > RCS file: /cvs/src/sys/net/if_pppx.c,v > retrieving revision 1.90 > diff -u -p -r1.90 if_pppx.c > --- sys/net/if_pppx.c 24 Jun 2020 08:52:53 -0000 1.90 > +++ sys/net/if_pppx.c 6 Jul 2020 11:10:17 -0000 > @@ -1117,6 +1117,8 @@ pppacopen(dev_t dev, int flags, int mode > ifp->if_output = pppac_output; > ifp->if_start = pppac_start; > ifp->if_ioctl = pppac_ioctl; > + /* XXXSMP: be sure pppac_start() called under NET_LOCK() */ > + IFQ_SET_MAXLEN(&ifp->if_snd, 1);
Is it possible to grab the NET_LOCK() inside pppac_start() instead of grabbing it outside? This should allow *start() routine to be called from any context. It might be interesting to see that as a difference between the NET_LOCK() used to protect the network stack internals and the NET_LOCK() used to protect pipex(4) internals. Such distinction might help to convert the latter into a different lock or primitive. > Index: sys/net/pipex.c > =================================================================== > RCS file: /cvs/src/sys/net/pipex.c,v > retrieving revision 1.117 > diff -u -p -r1.117 pipex.c > --- sys/net/pipex.c 30 Jun 2020 14:05:13 -0000 1.117 > +++ sys/net/pipex.c 6 Jul 2020 11:10:17 -0000 > @@ -869,6 +869,7 @@ pipex_output(struct mbuf *m0, int af, in > struct ip ip; > struct mbuf *mret; > > + NET_ASSERT_LOCKED(); This function doesn't touch any shared data structure, we'd better move the NET_ASSERT_LOCKED() above rn_lookuo() in pipex_lookup_by_ip_address(). Note that `pipex_rd_head4' and `pipex_rd_head6' are, with this diff, also protected by the NET_LOCK() and should be annotated as such. > session = NULL; > mret = NULL; > switch (af) { > @@ -962,6 +963,8 @@ pipex_ppp_output(struct mbuf *m0, struct > { > u_char *cp, hdr[16]; > > + NET_ASSERT_LOCKED(); Same here, it seems that the only reason the NET_LOCK() is necessary in the output path is to prevent corruption of the `session' descriptor being used. So we'd rather put the assertion above the LIST_FOREACH(). Anyway all of those can be addressed later, your diff is ok mpi@