Re: [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels

[Tom Parkin <tparkin@katalix.com>]

[-- Attachment #1: Type: text/plain, Size: 2967 bytes --]

On  Mon, Nov 09, 2020 at 23:51:53 +0100, Guillaume Nault wrote:
> >  * I believe that the fact we're not explicitly locking anything in the
> >    ppp_input path for access to the channel bridge field is OK since:
> >    
> >     - ppp_input is called from the socket backlog recv
> > 
> >     - pppox_unbind (which calls ppp_channel_unregister, which unsets the
> >       channel bridge field) is called from the socket release
> > 
> >    As such I think the bridge pointer cannot change in the recv
> >    path since as the pppoe.c code says: "Semantics of backlog rcv
> >    preclude any code from executing in lock_sock()/release_sock()
> >    bounds".
> 
> But ppp_input() is used beyond pppoe. For example, I'm pretty sure these
> pre-conditions aren't met for L2TP (pppol2tp_recv() processes packets
> directly, packets aren't queued by sk_receive_skb()).

Yes, that's true.  I was basing my assumption on the fact that the
l2tp/pppox recv path made similar checks to those in in pppoe.c, e.g.
sk_state.

I take your point more widely though: ppp_input is used by multiple
pppox drivers, so it probably makes more sense to protect the bridge
with a lock than rely on the driver implementation(s) all behaving in
the same way.

> To avoid locking the channel bridge in the data path, you can protect
> the pointer with RCU.

Ack, I'll look at doing so.

> >  * When userspace makes a PPPIOCBRIDGECHAN ioctl call, the channel the
> >    ioctl is called on is updated to point to the channel identified
> >    using the index passed in the ioctl call.
> > 
> >    As such, allow PPP frames to pass in both directions from channel A
> >    to channel B, userspace must call ioctl twice: once to bridge A to B,
> >    and once to bridge B to A.
> > 
> >    This approach makes the kernel coding easier, because the ioctl
> >    handler doesn't need to do anything to lock the channel which is
> >    identified by index: it's sufficient to find it in the per-net list
> >    (under protection of the list lock) and take a reference on it.
> > 
> >    The downside is that userspace must make two ioctl calls to fully set
> >    up the bridge.
> 
> That's probably okay, but that'd allow for very strange setups, like
> channel A pointing to channel B and channel B being used by a PPP unit.
> I'd prefer to avoid having to think about such scenarios when reasoning
> about the code.

Good point about the cognitive load.  I agree with you there.

> I think that the channel needs to be locked anyway to safely modify the
> bridge pointer. So the "no lock needed" benefit of the 2 ioctl calls
> approach doesn't seem to stand.

Agreed.

> BTW, shouldn't we have an "UNBRIDGE" command to remove the bridge
> between two channels?

I'm not sure of the usecase for it to be honest.  Do you have
something specific in mind?


Thanks very much for your review and comments, it's much appreciated
:-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]