On Wed, Feb 24, 2016 at 03:32:02PM -0500, David Miller wrote:
> From: Guillaume Nault <[email protected]>
> Date: Mon, 22 Feb 2016 20:47:13 +0100
>
> > PPP's Tx and Rx paths read ppp->mru under protection of ppp_xmit_lock()
> > and ppp_recv_lock() respectively.
> > Therefore ppp_ioctl() must hold the xmit and recv locks before
> > concurrently updating ppp->mru.
> >
> > Signed-off-by: Guillaume Nault <[email protected]>
> ...
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index fc8ad00..4d342ae 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -654,7 +654,10 @@ static long ppp_ioctl(struct file *file, unsigned int
> > cmd, unsigned long arg)
> > case PPPIOCSMRU:
> > if (get_user(val, p))
> > break;
> > + ppp_lock(ppp);
> > ppp->mru = val;
> > + ppp_unlock(ppp);
> > +
>
> I see no bug here at all.
>
> The store here is atomic, and all of those mentioned code paths only
> read the MRU once and then use that value for the duration of the
> rest of the processing of that PPP frame.
>
Ok, I didn't think we could assume atomic stores for int on all arch.
> No possible corruptions or misbehavior can occur and I therefore think
> the lack of locking here is completely legitimate.
>
Then this is also legitimate for most of the other fields considered in
this series. I'll drop the patches.
One exception is the n_channels and flags fields (patch #2). The update
side is done with read-modify-write instructions ('ppp->flags &= ~XXX'
in ppp_ccp_closed(), '++ppp->n_channels' in ppp_connect_channel()). So
locking should be required. I haven't succeeded in triggering any
misbehaviour from userspace though.
> You absolutely must demonstrate a case of corruption or misbehavior
> when you want to add supposedly "missing locking". Otherwise I'll have
> a hard time accepting your changes. This is especially for a subsystem
> that as been around as long as PPP.
Understood. Just to be sure, does patch #2 falls under lack of
demonstration? Or should I repost it separately?
