Date:   Mon, 9 Nov 2020 23:51:53 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Tom Parkin <tparkin@...alix.com>
Cc:     netdev@...r.kernel.org, jchapman@...alix.com
Subject: Re: [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels

On Fri, Nov 06, 2020 at 06:16:45PM +0000, Tom Parkin wrote:
> This small RFC series implements a suggestion from Guillaume Nault in
> response to my previous submission to add an ac/pppoe driver to the l2tp
> subsystem[1].
> 
> Following Guillaume's advice, this series adds an ioctl to the ppp code
> to allow a ppp channel to be bridged to another.  Quoting Guillaume:
> 
> "It's just a matter of extending struct channel (in ppp_generic.c) with
> a pointer to another channel, then testing this pointer in ppp_input().
> If the pointer is NULL, use the classical path, if not, forward the PPP
> frame using the ->start_xmit function of the peer channel."
> 
> This allows userspace to easily take PPP frames from e.g. a PPPoE
> session, and forward them over a PPPoL2TP session; accomplishing the
> same thing my earlier ac/pppoe driver in l2tp did but in much less code!

Nice to see this RFC. Thanks!

> Since I am not an expert in the ppp code, this patch set is RFC to
> gather any comments prior to making a proper submission.  I have tested
> this code using go-l2tp[2] and l2tp-ktest[3], but I have some
> uncertainties about the current implementation:
> 
>  * 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()).

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

>  * 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.

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.

> Any comments on the design welcome, especially thoughts on the two
> points above.

I haven't been go through all the details yet, but the general design
looks good to me. I'll comment inline for more precise feedbacks.

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

> Thanks :-)
> 
> [1]. Previous l2tp ac/pppoe patch set:
> 
> https://lore.kernel.org/netdev/20200930210707.10717-1-tparkin@katalix.com/
> 
> [2]. go-l2tp: a Go library for building L2TP applications on Linux
> systems, support for the PPPIOCBRIDGECHAN ioctl is on a branch:
> 
> https://github.com/katalix/go-l2tp/tree/tp_002_pppoe_2
> 
> [3]. l2tp-ktest: a test suite for the Linux Kernel L2TP subsystem
> 
> https://github.com/katalix/l2tp-ktest
> 
> Tom Parkin (2):
>   ppp: add PPPIOCBRIDGECHAN ioctl
>   docs: update ppp_generic.rst to describe ioctl PPPIOCBRIDGECHAN
> 
>  Documentation/networking/ppp_generic.rst |  5 ++++
>  drivers/net/ppp/ppp_generic.c            | 35 +++++++++++++++++++++++-
>  include/uapi/linux/ppp-ioctl.h           |  1 +
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> -- 
> 2.17.1
> 

Powered by blists - more mailing lists