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@
