Date:   Fri,  6 Nov 2020 18:16:45 +0000
From:   Tom Parkin <tparkin@...alix.com>
To:     netdev@...r.kernel.org
Cc:     gnault@...hat.com, jchapman@...alix.com,
        Tom Parkin <tparkin@...alix.com>
Subject: [RFC PATCH 0/2] add ppp_generic ioctl to bridge channels

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!

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

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

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

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