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

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

[Tom Parkin]

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

[RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Tom Parkin]

This new ioctl allows two ppp channels to be bridged together: frames
arriving in one channel are transmitted in the other channel and vice
versa.

The practical use for this is primarily to support the L2TP Access
Concentrator use-case.  The end-user session is presented as a ppp
channel (typically PPPoE, although it could be e.g. PPPoA, or even PPP
over a serial link) and is switched into a PPPoL2TP session for
transmission to the LNS.  At the LNS the PPP session is terminated in
the ISP's network.

When a PPP channel is bridged to another it takes a reference on the
other's struct ppp_file.  This reference is dropped when the channel is
unregistered: if the dereference causes the bridged channel's reference
count to reach zero it is destroyed at that point.
---
 drivers/net/ppp/ppp_generic.c  | 35 +++++++++++++++++++++++++++++++++-
 include/uapi/linux/ppp-ioctl.h |  1 +
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 7d005896a0f9..d893bf4470f4 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -175,6 +175,7 @@ struct channel {
 	struct net	*chan_net;	/* the net channel belongs to */
 	struct list_head clist;		/* link in list of channels per unit */
 	rwlock_t	upl;		/* protects `ppp' */
+	struct channel *bridge;		/* "bridged" ppp channel */
 #ifdef CONFIG_PPP_MULTILINK
 	u8		avail;		/* flag used in multilink stuff */
 	u8		had_frag;	/* >= 1 fragments have been sent */
@@ -641,8 +642,9 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	}
 
 	if (pf->kind == CHANNEL) {
-		struct channel *pch;
+		struct channel *pch, *pchb;
 		struct ppp_channel *chan;
+		struct ppp_net *pn;
 
 		pch = PF_TO_CHANNEL(pf);
 
@@ -657,6 +659,24 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			err = ppp_disconnect_channel(pch);
 			break;
 
+		case PPPIOCBRIDGECHAN:
+			if (get_user(unit, p))
+				break;
+			err = -ENXIO;
+			if (pch->bridge) {
+				err = -EALREADY;
+				break;
+			}
+			pn = ppp_pernet(current->nsproxy->net_ns);
+			spin_lock_bh(&pn->all_channels_lock);
+			pchb = ppp_find_channel(pn, unit);
+			if (pchb) {
+				refcount_inc(&pchb->file.refcnt);
+				pch->bridge = pchb;
+				err = 0;
+			}
+			spin_unlock_bh(&pn->all_channels_lock);
+			break;
 		default:
 			down_read(&pch->chan_sem);
 			chan = pch->chan;
@@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 		return;
 	}
 
+	if (pch->bridge) {
+		skb_queue_tail(&pch->bridge->file.xq, skb);
+		ppp_channel_push(pch->bridge);
+		return;
+	}
+
 	read_lock_bh(&pch->upl);
 	if (!ppp_decompress_proto(skb)) {
 		kfree_skb(skb);
@@ -2791,6 +2817,13 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	up_write(&pch->chan_sem);
 	ppp_disconnect_channel(pch);
 
+	/* Drop our reference on a bridged channel, if any */
+	if (pch->bridge) {
+		if (refcount_dec_and_test(&pch->bridge->file.refcnt))
+			ppp_destroy_channel(pch->bridge);
+		pch->bridge = NULL;
+	}
+
 	pn = ppp_pernet(pch->chan_net);
 	spin_lock_bh(&pn->all_channels_lock);
 	list_del(&pch->list);
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index 7bd2a5a75348..4b97ab519c19 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -115,6 +115,7 @@ struct pppol2tp_ioc_stats {
 #define PPPIOCATTCHAN	_IOW('t', 56, int)	/* attach to ppp channel */
 #define PPPIOCGCHAN	_IOR('t', 55, int)	/* get ppp channel number */
 #define PPPIOCGL2TPSTATS _IOR('t', 54, struct pppol2tp_ioc_stats)
+#define PPPIOCBRIDGECHAN _IOW('t', 53, int)	/* bridge one channel to another */
 
 #define SIOCGPPPSTATS   (SIOCDEVPRIVATE + 0)
 #define SIOCGPPPVER     (SIOCDEVPRIVATE + 1)	/* NEVER change this!! */
-- 
2.17.1

[RFC PATCH 2/2] docs: update ppp_generic.rst to describe ioctl PPPIOCBRIDGECHAN

[Tom Parkin]

---
 Documentation/networking/ppp_generic.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/networking/ppp_generic.rst b/Documentation/networking/ppp_generic.rst
index e60504377900..aea950ce953f 100644
--- a/Documentation/networking/ppp_generic.rst
+++ b/Documentation/networking/ppp_generic.rst
@@ -314,6 +314,11 @@ channel are:
   it is connected to.  It will return an EINVAL error if the channel
   is not connected to an interface.
 
+* PPPIOCBRIDGECHAN bridges a channel with another.  When frames are
+  presented to a channel by a call to ppp_input, they are passed to the
+  bridged channel by appending them to the channel's transmit queue.
+  This allows frames from one channel to be switched into another.
+
 * All other ioctl commands are passed to the channel ioctl() function.
 
 The ioctl calls that are available on an instance that is attached to
-- 
2.17.1

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

[Guillaume Nault]

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
> 

Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Guillaume Nault]

On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> This new ioctl allows two ppp channels to be bridged together: frames
> arriving in one channel are transmitted in the other channel and vice
> versa.
> 
> The practical use for this is primarily to support the L2TP Access
> Concentrator use-case.  The end-user session is presented as a ppp
> channel (typically PPPoE, although it could be e.g. PPPoA, or even PPP
> over a serial link) and is switched into a PPPoL2TP session for
> transmission to the LNS.  At the LNS the PPP session is terminated in
> the ISP's network.
> 
> When a PPP channel is bridged to another it takes a reference on the
> other's struct ppp_file.  This reference is dropped when the channel is
> unregistered: if the dereference causes the bridged channel's reference
> count to reach zero it is destroyed at that point.
> ---
>  drivers/net/ppp/ppp_generic.c  | 35 +++++++++++++++++++++++++++++++++-
>  include/uapi/linux/ppp-ioctl.h |  1 +
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 7d005896a0f9..d893bf4470f4 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -175,6 +175,7 @@ struct channel {
>  	struct net	*chan_net;	/* the net channel belongs to */
>  	struct list_head clist;		/* link in list of channels per unit */
>  	rwlock_t	upl;		/* protects `ppp' */
> +	struct channel *bridge;		/* "bridged" ppp channel */
>  #ifdef CONFIG_PPP_MULTILINK
>  	u8		avail;		/* flag used in multilink stuff */
>  	u8		had_frag;	/* >= 1 fragments have been sent */
> @@ -641,8 +642,9 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	}
>  
>  	if (pf->kind == CHANNEL) {
> -		struct channel *pch;
> +		struct channel *pch, *pchb;
>  		struct ppp_channel *chan;
> +		struct ppp_net *pn;
>  
>  		pch = PF_TO_CHANNEL(pf);
>  
> @@ -657,6 +659,24 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  			err = ppp_disconnect_channel(pch);
>  			break;
>  
> +		case PPPIOCBRIDGECHAN:
> +			if (get_user(unit, p))
> +				break;
> +			err = -ENXIO;
> +			if (pch->bridge) {
> +				err = -EALREADY;
> +				break;
> +			}
> +			pn = ppp_pernet(current->nsproxy->net_ns);
> +			spin_lock_bh(&pn->all_channels_lock);
> +			pchb = ppp_find_channel(pn, unit);
> +			if (pchb) {
> +				refcount_inc(&pchb->file.refcnt);
> +				pch->bridge = pchb;

I think we shouldn't modify ->bridge if it's already set or if the
channel is already part of of a PPP unit  (that is, if pch->ppp or
pch->bridge is not NULL).

This also means that we have to use appropriate locking.

> +				err = 0;
> +			}
> +			spin_unlock_bh(&pn->all_channels_lock);
> +			break;
>  		default:
>  			down_read(&pch->chan_sem);
>  			chan = pch->chan;
> @@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
>  		return;
>  	}
>  
> +	if (pch->bridge) {
> +		skb_queue_tail(&pch->bridge->file.xq, skb);

The bridged channel might reside in a different network namespace.
So it seems that skb_scrub_packet() is needed before sending the
packet.

> +		ppp_channel_push(pch->bridge);

I'm not sure if the skb_queue_tail()/ppp_channel_push() sequence really
is necessary. We're not going through a PPP unit, so we have no risk of
recursion here. Also, if ->start_xmit() fails, I see no reason for
requeuing the skb, like __ppp_channel_push() does. I'd have to think
more about it, but I believe that we could call the channel's
->start_xmit() function directly (respecting the locking constraints
of course).

> +		return;
> +	}
> +
>  	read_lock_bh(&pch->upl);
>  	if (!ppp_decompress_proto(skb)) {
>  		kfree_skb(skb);
> @@ -2791,6 +2817,13 @@ ppp_unregister_channel(struct ppp_channel *chan)
>  	up_write(&pch->chan_sem);
>  	ppp_disconnect_channel(pch);
>  
> +	/* Drop our reference on a bridged channel, if any */
> +	if (pch->bridge) {
> +		if (refcount_dec_and_test(&pch->bridge->file.refcnt))
> +			ppp_destroy_channel(pch->bridge);
> +		pch->bridge = NULL;
> +	}
> +
>  	pn = ppp_pernet(pch->chan_net);
>  	spin_lock_bh(&pn->all_channels_lock);
>  	list_del(&pch->list);
> diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
> index 7bd2a5a75348..4b97ab519c19 100644
> --- a/include/uapi/linux/ppp-ioctl.h
> +++ b/include/uapi/linux/ppp-ioctl.h
> @@ -115,6 +115,7 @@ struct pppol2tp_ioc_stats {
>  #define PPPIOCATTCHAN	_IOW('t', 56, int)	/* attach to ppp channel */
>  #define PPPIOCGCHAN	_IOR('t', 55, int)	/* get ppp channel number */
>  #define PPPIOCGL2TPSTATS _IOR('t', 54, struct pppol2tp_ioc_stats)
> +#define PPPIOCBRIDGECHAN _IOW('t', 53, int)	/* bridge one channel to another */
>  
>  #define SIOCGPPPSTATS   (SIOCDEVPRIVATE + 0)
>  #define SIOCGPPPVER     (SIOCDEVPRIVATE + 1)	/* NEVER change this!! */
> -- 
> 2.17.1
> 

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

[Jakub Kicinski]

On Fri,  6 Nov 2020 18:16:45 +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!

I have little understanding of the ppp code, but I can't help but
wonder why this special channel connection is needed? We have great
many ways to redirect traffic between interfaces - bpf, tc, netfilter,
is there anything ppp specific that is required here?

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

[Guillaume Nault]

On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote:
> On Fri,  6 Nov 2020 18:16:45 +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!
> 
> I have little understanding of the ppp code, but I can't help but
> wonder why this special channel connection is needed? We have great
> many ways to redirect traffic between interfaces - bpf, tc, netfilter,
> is there anything ppp specific that is required here?

I can see two viable ways to implement this feature. The one described
in this patch series is the simplest. The reason why it doesn't reuse
existing infrastructure is because it has to work at the link layer
(no netfilter) and also has to work on PPP channels (no network
device).

The alternative, is to implement a virtual network device for the
protocols we want to support (at least PPPoE and L2TP, maybe PPTP)
and teach tunnel_key about them. Then we could use iproute2 commands
like:
 # ip link add name pppoe0 up type pppoe external
 # ip link add name l2tp0 up type l2tp external
 # tc qdisc add dev pppoe0 ingress
 # tc qdisc add dev l2tp0 ingress
 # tc filter add dev pppoe0 ingress matchall                        \
     action tunnel_key set l2tp_version 2 l2tp_tid XXX l2tp_sid YYY \
     action mirred egress redirect dev pppoe0
 # tc filter add dev l2tp0 ingress matchall  \
     action tunnel_key set pppoe_sid ZZZ     \
     action mirred egress redirect dev l2tp0

Note: I've used matchall for simplicity, but a real uses case would
have to filter on the L2TP session and tunnel IDs and on the PPPoE
session ID.

As I said in my reply to the original thread, I like this idea, but
haven't thought much about the details. So there might be some road
blocks. Beyond modernising PPP and making it better integrated into the
stack, that should also bring the possibility of hardware offload (but
would any NIC vendor be interested?).

I think the question is more about long term maintainance. Do we want
to keep PPP related module self contained, with low maintainance code
(the current proposal)? Or are we willing to modernise the
infrastructure, add support and maintain PPP features in other modules
like flower, tunnel_key, etc.?

Of course, I might have missed other ways to implement this feature.
But that's all I could think of for now.

And if anyone wants a quick recap about PPP (what are these PPP channel
and unit things? what's the relationship between PPPoE, L2TP and PPP?
etc.), just let me know.

Hope this clarifies the situation.

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

[Tom Parkin]

[-- 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 --]

Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Tom Parkin]

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

On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> > +		case PPPIOCBRIDGECHAN:
> > +			if (get_user(unit, p))
> > +				break;
> > +			err = -ENXIO;
> > +			if (pch->bridge) {
> > +				err = -EALREADY;
> > +				break;
> > +			}
> > +			pn = ppp_pernet(current->nsproxy->net_ns);
> > +			spin_lock_bh(&pn->all_channels_lock);
> > +			pchb = ppp_find_channel(pn, unit);
> > +			if (pchb) {
> > +				refcount_inc(&pchb->file.refcnt);
> > +				pch->bridge = pchb;
> 
> I think we shouldn't modify ->bridge if it's already set or if the
> channel is already part of of a PPP unit  (that is, if pch->ppp or
> pch->bridge is not NULL).
> 
> This also means that we have to use appropriate locking.

Yes, good point about checking for the channel being part of a PPP
unit.

> > +				err = 0;
> > +			}
> > +			spin_unlock_bh(&pn->all_channels_lock);
> > +			break;
> >  		default:
> >  			down_read(&pch->chan_sem);
> >  			chan = pch->chan;
> > @@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> >  		return;
> >  	}
> >  
> > +	if (pch->bridge) {
> > +		skb_queue_tail(&pch->bridge->file.xq, skb);
> 
> The bridged channel might reside in a different network namespace.
> So it seems that skb_scrub_packet() is needed before sending the
> packet.

I'm not sure about this.

PPPIOCBRIDGECHAN is looking up the bridged channel in the ppp_pernet
list.  Unless the channel can migrate across network namespaces after
the bridge is set up I don't think it would be possible for the
bridged channel to be in a different namespace.

Am I missing something here?

> 
> > +		ppp_channel_push(pch->bridge);
> 
> I'm not sure if the skb_queue_tail()/ppp_channel_push() sequence really
> is necessary. We're not going through a PPP unit, so we have no risk of
> recursion here. Also, if ->start_xmit() fails, I see no reason for
> requeuing the skb, like __ppp_channel_push() does. I'd have to think
> more about it, but I believe that we could call the channel's
> ->start_xmit() function directly (respecting the locking constraints
> of course).

I take your point about re-queuing based on the return of
->start_xmit().  For pppoe and pppol2tp start_xmit just swallows the
skb on failure in any case, so for this specific usecase queuing is
not an issue.

However, my primary motivation for using ppp_channel_push was actually
the handling for managing dropping the packet if the channel was
deregistered.

It'd be simple enough to add another function which performed the same
deregistration check but didn't transmit via. the queue.

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

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

[Tom Parkin]

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

On  Tue, Nov 10, 2020 at 10:28:34 +0100, Guillaume Nault wrote:
> On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote:
> > On Fri,  6 Nov 2020 18:16:45 +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.
> > 
> > I have little understanding of the ppp code, but I can't help but
> > wonder why this special channel connection is needed? We have great
> > many ways to redirect traffic between interfaces - bpf, tc, netfilter,
> > is there anything ppp specific that is required here?
> 
> I can see two viable ways to implement this feature. The one described
> in this patch series is the simplest. The reason why it doesn't reuse
> existing infrastructure is because it has to work at the link layer
> (no netfilter) and also has to work on PPP channels (no network
> device).
> 
> The alternative, is to implement a virtual network device for the
> protocols we want to support (at least PPPoE and L2TP, maybe PPTP)
> and teach tunnel_key about them.

One potential downside of this approach is the addition of two virtual
interfaces for each pppoe->pppol2tp mapping: the concern here
primarily being the cost of doing so.

I'm not saying the cost is necessarily prohibitive, but the "bridge the
channels" approach in the RFC is certainly cheaper.

Another concern would be the possibility of the virtual devices being
misconfigured in such a way as to e.g. allow locally generated
broadcast packets to go out on one of the interfaces.  Possibly this
would be easy to avoid, I'm not sure.

> I think the question is more about long term maintainance. Do we want
> to keep PPP related module self contained, with low maintainance code
> (the current proposal)? Or are we willing to modernise the
> infrastructure, add support and maintain PPP features in other modules
> like flower, tunnel_key, etc.?

FWIW I would tend to agree.

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

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

[Guillaume Nault]

On Tue, Nov 10, 2020 at 12:42:24PM +0000, Tom Parkin wrote:
> On  Tue, Nov 10, 2020 at 10:28:34 +0100, Guillaume Nault wrote:
> > On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote:
> > > On Fri,  6 Nov 2020 18:16:45 +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.
> > > 
> > > I have little understanding of the ppp code, but I can't help but
> > > wonder why this special channel connection is needed? We have great
> > > many ways to redirect traffic between interfaces - bpf, tc, netfilter,
> > > is there anything ppp specific that is required here?
> > 
> > I can see two viable ways to implement this feature. The one described
> > in this patch series is the simplest. The reason why it doesn't reuse
> > existing infrastructure is because it has to work at the link layer
> > (no netfilter) and also has to work on PPP channels (no network
> > device).
> > 
> > The alternative, is to implement a virtual network device for the
> > protocols we want to support (at least PPPoE and L2TP, maybe PPTP)
> > and teach tunnel_key about them.
> 
> One potential downside of this approach is the addition of two virtual
> interfaces for each pppoe->pppol2tp mapping: the concern here
> primarily being the cost of doing so.

No, this is fixed cost. There'd be only one PPPoE interface for
handling all the PPPoE sessions and one for L2TP. These virtual
interfaces wouldn't be specific to a particular session ID. Instead,
the encapsulation information would be attached to the skb and the
virtual PPPoE or L2TP device would build the header based on these
metadata.

> I'm not saying the cost is necessarily prohibitive, but the "bridge the
> channels" approach in the RFC is certainly cheaper.
> 
> Another concern would be the possibility of the virtual devices being
> misconfigured in such a way as to e.g. allow locally generated
> broadcast packets to go out on one of the interfaces.  Possibly this
> would be easy to avoid, I'm not sure.

I'm not too woried about that. A PPPoE or L2TP interface in external
mode couldn't build its header and forward the packet if the skb it
received doesn't have the proper metadat attached to it. So packets
couldn't be inadvertently sent through these interfaces, something
would have to attach the tunnel metadata to the skb beforehand.

But I agree that the setup becomes visible to the administrator, while
the ioctl() approach kept the whole kernel configuration in the hands
of the control-plane implementation. I think that's a good thing
(easier testing and troubleshooting), but yes, that also opens the
possibility for fat finger mistakes.

> > I think the question is more about long term maintainance. Do we want
> > to keep PPP related module self contained, with low maintainance code
> > (the current proposal)? Or are we willing to modernise the
> > infrastructure, add support and maintain PPP features in other modules
> > like flower, tunnel_key, etc.?
> 
> FWIW I would tend to agree.

Yes, it's really about how much we're ready to invest into PPP-related
features.

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

[Jakub Kicinski]

On Tue, 10 Nov 2020 10:28:34 +0100 Guillaume Nault wrote:
> On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote:
> > On Fri,  6 Nov 2020 18:16:45 +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!  
> > 
> > I have little understanding of the ppp code, but I can't help but
> > wonder why this special channel connection is needed? We have great
> > many ways to redirect traffic between interfaces - bpf, tc, netfilter,
> > is there anything ppp specific that is required here?  
> 
> I can see two viable ways to implement this feature. The one described
> in this patch series is the simplest. The reason why it doesn't reuse
> existing infrastructure is because it has to work at the link layer
> (no netfilter) and also has to work on PPP channels (no network
> device).
> 
> The alternative, is to implement a virtual network device for the
> protocols we want to support (at least PPPoE and L2TP, maybe PPTP)
> and teach tunnel_key about them. Then we could use iproute2 commands
> like:
>  # ip link add name pppoe0 up type pppoe external
>  # ip link add name l2tp0 up type l2tp external
>  # tc qdisc add dev pppoe0 ingress
>  # tc qdisc add dev l2tp0 ingress
>  # tc filter add dev pppoe0 ingress matchall                        \
>      action tunnel_key set l2tp_version 2 l2tp_tid XXX l2tp_sid YYY \
>      action mirred egress redirect dev pppoe0
>  # tc filter add dev l2tp0 ingress matchall  \
>      action tunnel_key set pppoe_sid ZZZ     \
>      action mirred egress redirect dev l2tp0
> 
> Note: I've used matchall for simplicity, but a real uses case would
> have to filter on the L2TP session and tunnel IDs and on the PPPoE
> session ID.
> 
> As I said in my reply to the original thread, I like this idea, but
> haven't thought much about the details. So there might be some road
> blocks. Beyond modernising PPP and making it better integrated into the
> stack, that should also bring the possibility of hardware offload (but
> would any NIC vendor be interested?).

Integrating with the stack gives you access to all its features, other
types of encap, firewalling, bpf, etc.

> I think the question is more about long term maintainance. Do we want
> to keep PPP related module self contained, with low maintainance code
> (the current proposal)? Or are we willing to modernise the
> infrastructure, add support and maintain PPP features in other modules
> like flower, tunnel_key, etc.?

Right, it's really not great to see new IOCTLs being added to drivers,
but the alternative would require easily 50 times more code.
 
> Of course, I might have missed other ways to implement this feature.
> But that's all I could think of for now.
> 
> And if anyone wants a quick recap about PPP (what are these PPP channel
> and unit things? what's the relationship between PPPoE, L2TP and PPP?
> etc.), just let me know.

Some pointers would be appreciated if you don't mind :)

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

[Guillaume Nault]

On Tue, Nov 10, 2020 at 11:54:07AM +0000, Tom Parkin wrote:
> On  Mon, Nov 09, 2020 at 23:51:53 +0100, Guillaume Nault wrote:
> > 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?

I don't know if there'd be a real production use case. I proposed it
because, in my experience, the diffucult part of any new feature is
the "undo" operation. That's where many race conditions are found.

Having a way to directly revert a BRIDGE operation might help testing
the undo path (otherwise it's just triggered as a side effect of
closing a file descriptor). I personally find that having symmetrical
"do" and "undo" operations helps me thinking precisely about how to
manage concurency. But that's probably a matter of preference. And that
can even be done without exposing the "undo" operation to user space
(it's just more difficult to test).

Anyway, that was just a suggestion. I have no strong opinion.

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

Thanks! :)

Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Guillaume Nault]

On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> > > +				err = 0;
> > > +			}
> > > +			spin_unlock_bh(&pn->all_channels_lock);
> > > +			break;
> > >  		default:
> > >  			down_read(&pch->chan_sem);
> > >  			chan = pch->chan;
> > > @@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> > >  		return;
> > >  	}
> > >  
> > > +	if (pch->bridge) {
> > > +		skb_queue_tail(&pch->bridge->file.xq, skb);
> > 
> > The bridged channel might reside in a different network namespace.
> > So it seems that skb_scrub_packet() is needed before sending the
> > packet.
> 
> I'm not sure about this.
> 
> PPPIOCBRIDGECHAN is looking up the bridged channel in the ppp_pernet
> list.  Unless the channel can migrate across network namespaces after
> the bridge is set up I don't think it would be possible for the
> bridged channel to be in a different namespace.
> 
> Am I missing something here?

So yes, channels can't migrate across namespaces. However, the bridged
channel is looked up from the caller's current namespace, which isn't
guaranteed to be the same namespace as the channel used in the ioctl().

For example:

setns(ns1, CLONE_NEWNET);
chan_ns1 = open("/dev/ppp");
...
setns(ns2, CLONE_NEWNET);
chan_ns2 = open("/dev/ppp");
...
ioctl(chan_ns1, PPPIOCBRIDGECHAN, chan_ns2_id);

Here, chan_ns1 belongs to ns1, but chan_ns2_id will be looked up in the
context of ns2. I find it nice to have the possibility to bridge
channels from different namespaces, but we have to handle the case
correctly.

> > > +		ppp_channel_push(pch->bridge);
> > 
> > I'm not sure if the skb_queue_tail()/ppp_channel_push() sequence really
> > is necessary. We're not going through a PPP unit, so we have no risk of
> > recursion here. Also, if ->start_xmit() fails, I see no reason for
> > requeuing the skb, like __ppp_channel_push() does. I'd have to think
> > more about it, but I believe that we could call the channel's
> > ->start_xmit() function directly (respecting the locking constraints
> > of course).
> 
> I take your point about re-queuing based on the return of
> ->start_xmit().  For pppoe and pppol2tp start_xmit just swallows the
> skb on failure in any case, so for this specific usecase queuing is
> not an issue.

Indeed.

> However, my primary motivation for using ppp_channel_push was actually
> the handling for managing dropping the packet if the channel was
> deregistered.

I might be missing something, but I don't see what ppp_channel_push()
does appart from holding the xmit lock and handling the xmit queue.
If we agree that there's no need to use the xmit queue, all
ppp_channel_push() does for us is taking pch->downl, which we probably
can do on our own.

> It'd be simple enough to add another function which performed the same
> deregistration check but didn't transmit via. the queue.

That's probably what I'm missing: what do you mean by "deregistration
check"? I can't see anything like this in ppp_channel_push().

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

[Tom Parkin]

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

On  Sun, Nov 15, 2020 at 12:59:59 +0100, Guillaume Nault wrote:
> On Tue, Nov 10, 2020 at 11:54:07AM +0000, Tom Parkin wrote:
> > On  Mon, Nov 09, 2020 at 23:51:53 +0100, Guillaume Nault wrote:
> > > 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?
> 
> I don't know if there'd be a real production use case. I proposed it
> because, in my experience, the diffucult part of any new feature is
> the "undo" operation. That's where many race conditions are found.
> 
> Having a way to directly revert a BRIDGE operation might help testing
> the undo path (otherwise it's just triggered as a side effect of
> closing a file descriptor). I personally find that having symmetrical
> "do" and "undo" operations helps me thinking precisely about how to
> manage concurency. But that's probably a matter of preference. And that
> can even be done without exposing the "undo" operation to user space
> (it's just more difficult to test).
> 
> Anyway, that was just a suggestion. I have no strong opinion.

Thanks for clarifying the point -- I agree with you about the "undo"
operation helping to expose race conditions.

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

Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Tom Parkin]

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

On  Sun, Nov 15, 2020 at 17:28:38 +0100, Guillaume Nault wrote:
> On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> > On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > > On Fri, Nov 06, 2020 at 06:16:46PM +0000, Tom Parkin wrote:
> > > > +				err = 0;
> > > > +			}
> > > > +			spin_unlock_bh(&pn->all_channels_lock);
> > > > +			break;
> > > >  		default:
> > > >  			down_read(&pch->chan_sem);
> > > >  			chan = pch->chan;
> > > > @@ -2100,6 +2120,12 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> > > >  		return;
> > > >  	}
> > > >  
> > > > +	if (pch->bridge) {
> > > > +		skb_queue_tail(&pch->bridge->file.xq, skb);
> > > 
> > > The bridged channel might reside in a different network namespace.
> > > So it seems that skb_scrub_packet() is needed before sending the
> > > packet.
> > 
> > I'm not sure about this.
> > 
> > PPPIOCBRIDGECHAN is looking up the bridged channel in the ppp_pernet
> > list.  Unless the channel can migrate across network namespaces after
> > the bridge is set up I don't think it would be possible for the
> > bridged channel to be in a different namespace.
> > 
> > Am I missing something here?
> 
> So yes, channels can't migrate across namespaces. However, the bridged
> channel is looked up from the caller's current namespace, which isn't
> guaranteed to be the same namespace as the channel used in the ioctl().
> 
> For example:
> 
> setns(ns1, CLONE_NEWNET);
> chan_ns1 = open("/dev/ppp");
> ...
> setns(ns2, CLONE_NEWNET);
> chan_ns2 = open("/dev/ppp");
> ...
> ioctl(chan_ns1, PPPIOCBRIDGECHAN, chan_ns2_id);
> 
> Here, chan_ns1 belongs to ns1, but chan_ns2_id will be looked up in the
> context of ns2. I find it nice to have the possibility to bridge
> channels from different namespaces, but we have to handle the case
> correctly.

Ah, of course, I see what you're saying.

Agreed we should add the skb_scrub_packet() call.

> > > > +		ppp_channel_push(pch->bridge);
> > > 
> > > I'm not sure if the skb_queue_tail()/ppp_channel_push() sequence really
> > > is necessary. We're not going through a PPP unit, so we have no risk of
> > > recursion here. Also, if ->start_xmit() fails, I see no reason for
> > > requeuing the skb, like __ppp_channel_push() does. I'd have to think
> > > more about it, but I believe that we could call the channel's
> > > ->start_xmit() function directly (respecting the locking constraints
> > > of course).
> > 
> > I take your point about re-queuing based on the return of
> > ->start_xmit().  For pppoe and pppol2tp start_xmit just swallows the
> > skb on failure in any case, so for this specific usecase queuing is
> > not an issue.
> 
> Indeed.
> 
> > However, my primary motivation for using ppp_channel_push was actually
> > the handling for managing dropping the packet if the channel was
> > deregistered.
> 
> I might be missing something, but I don't see what ppp_channel_push()
> does appart from holding the xmit lock and handling the xmit queue.
> If we agree that there's no need to use the xmit queue, all
> ppp_channel_push() does for us is taking pch->downl, which we probably
> can do on our own.
> 
> > It'd be simple enough to add another function which performed the same
> > deregistration check but didn't transmit via. the queue.
> 
> That's probably what I'm missing: what do you mean by "deregistration
> check"? I can't see anything like this in ppp_channel_push().

It's literally just the check on pch->chan once pch->downl is held.
So it would be trivial to do the same thing in a different codepath: I
just figured why reinvent the wheel :-)

Sorry for the confusion.

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

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

[Tom Parkin]

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

On  Tue, Nov 10, 2020 at 08:47:40 -0800, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 10:28:34 +0100 Guillaume Nault wrote:
> > On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote:
> > > On Fri,  6 Nov 2020 18:16:45 +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!  
> > > 
> > > I have little understanding of the ppp code, but I can't help but
> > > wonder why this special channel connection is needed? We have great
> > > many ways to redirect traffic between interfaces - bpf, tc, netfilter,
> > > is there anything ppp specific that is required here?  
> > 
> > I can see two viable ways to implement this feature. The one described
> > in this patch series is the simplest. The reason why it doesn't reuse
> > existing infrastructure is because it has to work at the link layer
> > (no netfilter) and also has to work on PPP channels (no network
> > device).
> > 
> > The alternative, is to implement a virtual network device for the
> > protocols we want to support (at least PPPoE and L2TP, maybe PPTP)
> > and teach tunnel_key about them. Then we could use iproute2 commands
> > like:
> >  # ip link add name pppoe0 up type pppoe external
> >  # ip link add name l2tp0 up type l2tp external
> >  # tc qdisc add dev pppoe0 ingress
> >  # tc qdisc add dev l2tp0 ingress
> >  # tc filter add dev pppoe0 ingress matchall                        \
> >      action tunnel_key set l2tp_version 2 l2tp_tid XXX l2tp_sid YYY \
> >      action mirred egress redirect dev pppoe0
> >  # tc filter add dev l2tp0 ingress matchall  \
> >      action tunnel_key set pppoe_sid ZZZ     \
> >      action mirred egress redirect dev l2tp0
> > 
> > Note: I've used matchall for simplicity, but a real uses case would
> > have to filter on the L2TP session and tunnel IDs and on the PPPoE
> > session ID.
> > 
> > As I said in my reply to the original thread, I like this idea, but
> > haven't thought much about the details. So there might be some road
> > blocks. Beyond modernising PPP and making it better integrated into the
> > stack, that should also bring the possibility of hardware offload (but
> > would any NIC vendor be interested?).
> 
> Integrating with the stack gives you access to all its features, other
> types of encap, firewalling, bpf, etc.
> 
> > I think the question is more about long term maintainance. Do we want
> > to keep PPP related module self contained, with low maintainance code
> > (the current proposal)? Or are we willing to modernise the
> > infrastructure, add support and maintain PPP features in other modules
> > like flower, tunnel_key, etc.?
> 
> Right, it's really not great to see new IOCTLs being added to drivers,
> but the alternative would require easily 50 times more code.

Jakub, could I quickly poll you on your current gut-feel level of
opposition to the ioctl-based approach?

Guillaume has given good feedback on the RFC code which I can work
into an actual patch submission, but I don't really want to if you're
totally opposed to the whole idea :-)

I appreciate you may want to reserve judgement pending a recap of the
ppp subsystem as it stands.

Thanks!

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

Re: [RFC PATCH 1/2] ppp: add PPPIOCBRIDGECHAN ioctl

[Guillaume Nault]

On Tue, Nov 17, 2020 at 12:26:38PM +0000, Tom Parkin wrote:
> On  Sun, Nov 15, 2020 at 17:28:38 +0100, Guillaume Nault wrote:
> > On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote:
> > > On  Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote:
> > > However, my primary motivation for using ppp_channel_push was actually
> > > the handling for managing dropping the packet if the channel was
> > > deregistered.
> > 
> > I might be missing something, but I don't see what ppp_channel_push()
> > does appart from holding the xmit lock and handling the xmit queue.
> > If we agree that there's no need to use the xmit queue, all
> > ppp_channel_push() does for us is taking pch->downl, which we probably
> > can do on our own.
> > 
> > > It'd be simple enough to add another function which performed the same
> > > deregistration check but didn't transmit via. the queue.
> > 
> > That's probably what I'm missing: what do you mean by "deregistration
> > check"? I can't see anything like this in ppp_channel_push().
> 
> It's literally just the check on pch->chan once pch->downl is held.
> So it would be trivial to do the same thing in a different codepath: I
> just figured why reinvent the wheel :-)

Okay, I was thinking of something more complex. I agree with not
reinventing existing functions, but in this case, I think that
ppp_channel_push() does too many unecessary operations (like recursion
handling and processing the parent unit's queue). Also, a helper
function would be the natural place for calling skb_scrub_packet()
and for handling concurent access or modification of the ->bridge
pointer (as discussed earlier in this thread).

> 
> Sorry for the confusion.

No problem. It's nice to see some work being done in this area :).

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

[Guillaume Nault]

On Tue, Nov 17, 2020 at 12:54:22PM +0000, Tom Parkin wrote:
> On  Tue, Nov 10, 2020 at 08:47:40 -0800, Jakub Kicinski wrote:
> > On Tue, 10 Nov 2020 10:28:34 +0100 Guillaume Nault wrote:
> > > I think the question is more about long term maintainance. Do we want
> > > to keep PPP related module self contained, with low maintainance code
> > > (the current proposal)? Or are we willing to modernise the
> > > infrastructure, add support and maintain PPP features in other modules
> > > like flower, tunnel_key, etc.?
> > 
> > Right, it's really not great to see new IOCTLs being added to drivers,
> > but the alternative would require easily 50 times more code.
> 
> Jakub, could I quickly poll you on your current gut-feel level of
> opposition to the ioctl-based approach?
> 
> Guillaume has given good feedback on the RFC code which I can work
> into an actual patch submission, but I don't really want to if you're
> totally opposed to the whole idea :-)
> 
> I appreciate you may want to reserve judgement pending a recap of the
> ppp subsystem as it stands.

I've started writing some general explanations about the protocol and
the actual kernel implementation. I'm planning to send them in the days
to come. I just have to finish some higher priority tasks first. Sorry
for the delay.

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

[Jakub Kicinski]

On Tue, 17 Nov 2020 12:54:22 +0000 Tom Parkin wrote:
> > > I think the question is more about long term maintainance. Do we want
> > > to keep PPP related module self contained, with low maintainance code
> > > (the current proposal)? Or are we willing to modernise the
> > > infrastructure, add support and maintain PPP features in other modules
> > > like flower, tunnel_key, etc.?  
> > 
> > Right, it's really not great to see new IOCTLs being added to drivers,
> > but the alternative would require easily 50 times more code.  
> 
> Jakub, could I quickly poll you on your current gut-feel level of
> opposition to the ioctl-based approach?
> 
> Guillaume has given good feedback on the RFC code which I can work
> into an actual patch submission, but I don't really want to if you're
> totally opposed to the whole idea :-)

I'll merge it if no one else speaks up in opposition.

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

[Guillaume Nault]

On Tue, Nov 10, 2020 at 08:47:40AM -0800, Jakub Kicinski wrote:
> On Tue, 10 Nov 2020 10:28:34 +0100 Guillaume Nault wrote:
> > On Mon, Nov 09, 2020 at 03:52:37PM -0800, Jakub Kicinski wrote:
> > > On Fri,  6 Nov 2020 18:16:45 +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!  
> > > 
> > > I have little understanding of the ppp code, but I can't help but
> > > wonder why this special channel connection is needed? We have great
> > > many ways to redirect traffic between interfaces - bpf, tc, netfilter,
> > > is there anything ppp specific that is required here?  
> > 
> > I can see two viable ways to implement this feature. The one described
> > in this patch series is the simplest. The reason why it doesn't reuse
> > existing infrastructure is because it has to work at the link layer
> > (no netfilter) and also has to work on PPP channels (no network
> > device).
> > 
> > The alternative, is to implement a virtual network device for the
> > protocols we want to support (at least PPPoE and L2TP, maybe PPTP)
> > and teach tunnel_key about them. Then we could use iproute2 commands
> > like:
> >  # ip link add name pppoe0 up type pppoe external
> >  # ip link add name l2tp0 up type l2tp external
> >  # tc qdisc add dev pppoe0 ingress
> >  # tc qdisc add dev l2tp0 ingress
> >  # tc filter add dev pppoe0 ingress matchall                        \
> >      action tunnel_key set l2tp_version 2 l2tp_tid XXX l2tp_sid YYY \
> >      action mirred egress redirect dev pppoe0
> >  # tc filter add dev l2tp0 ingress matchall  \
> >      action tunnel_key set pppoe_sid ZZZ     \
> >      action mirred egress redirect dev l2tp0
> > 
> > Note: I've used matchall for simplicity, but a real uses case would
> > have to filter on the L2TP session and tunnel IDs and on the PPPoE
> > session ID.
> > 
> > As I said in my reply to the original thread, I like this idea, but
> > haven't thought much about the details. So there might be some road
> > blocks. Beyond modernising PPP and making it better integrated into the
> > stack, that should also bring the possibility of hardware offload (but
> > would any NIC vendor be interested?).
> 
> Integrating with the stack gives you access to all its features, other
> types of encap, firewalling, bpf, etc.
> 
> > I think the question is more about long term maintainance. Do we want
> > to keep PPP related module self contained, with low maintainance code
> > (the current proposal)? Or are we willing to modernise the
> > infrastructure, add support and maintain PPP features in other modules
> > like flower, tunnel_key, etc.?
> 
> Right, it's really not great to see new IOCTLs being added to drivers,
> but the alternative would require easily 50 times more code.
>  
> > Of course, I might have missed other ways to implement this feature.
> > But that's all I could think of for now.
> > 
> > And if anyone wants a quick recap about PPP (what are these PPP channel
> > and unit things? what's the relationship between PPPoE, L2TP and PPP?
> > etc.), just let me know.
> 
> Some pointers would be appreciated if you don't mind :)

Here's a high level view of:
  * the protocol,
  * the kernel implementation,
  * the context of this RFC,
  * and a few pointers at the end :)

Hope this helps. I've tried to keep it short. Feel free to ask for
clarifications and details.


The Point-to-Point Protocol
===========================

PPP is a layer 2 protocol. The header is a single field that identifies
the upper protocol (just like an Ethertype). PPP is point-to-point, so
there's no need for source and destination link layer addresses in the
header: whatever is sent on one end of the pipe is received by the host
on the other end, it's that simple (no shared medium, no switching).
Some protocols have been defined to tunnel PPP packets (PPPoE, L2TP).

PPP doesn't just define how to handle data frames, it also has control
protocols. The Link Control Protocol (LCP) is used to negotiate
link-layer parameters (maximum packet size, optionally request the peer
to authenticate, etc.). LCP is part of the PPP specification. All other
control protocols are defined in different RFCs, but they use the same
protocol structure as LCP.

Once both peers agree on the link parameters, they can proceed to the
optional authentication phase (if that was negotiated during the LCP
phase). There are several authentication protocols available; the one
to use is selected during the LCP phase.

Finally, the peers can negotiate whatever network protocol they want to
use: with PPP, all network protocols need to have an equivalent NCP
(Network Control Protocol). For example, IPv4 has IPCP, IPv6 has IPv6CP,
MPLS has MPLSCP, etc. In some cases, the NCP is used to negotiate
network specific parameters. For example IPCP allows each peer to
advertise its IPv4 address or to request an address from the remote
peer. NCPs are generally very simple. Some don't even have any
parameter to negotiate (like MPLSCP).

Once an NCP has been negotiated, the peers can exchange data packets of
that protocol. Of course several network protocols can be used
simultaneously.

PPP can run over physical links or be tunnelled into other protocols.
For example, PPPoE carries PPP over Ethernet and L2TP tunnels PPP into
UDP.

Kernel Implementation
=====================

The Linux kernel implementation exposes a /dev/ppp virtual file that's
used by user space to implement the control and authentication
protocols.

Typically, user space starts by opening /dev/ppp. By calling the
PPPIOCATTCHAN ioctl, it attaches the file descriptor to a lower layer
that implements the rx and tx handlers. The lower layer may be a serial
link, an L2TP or PPPoE session, etc. We don't have a networking device
yet, but the file descriptor can now receive and send data over the
link, which is enough to implement LCP and authentication protocols.
This is what ppp_generic.c calls a PPP channel.

Then, to create a network device, one needs to open another file
descriptor on /dev/ppp and call the PPPIOCNEWUNIT ioctl. Alternatively,
it's possible to use a netlink call instead of PPPIOCNEWUNIT to create
the netdevice and attach it to the new file descriptor. We now have
what ppp_generic.c calls a PPP unit.

The unit currently doesn't know how to send data on the wire, so one
needs to connect it to the channel. This is done by another ioctl:
PPPIOCCONNECT. Now, the PPP networking device is able to send data
packets on the wire, and the unit file descriptor can be used to
implement the network control protocols.

The reason for having channels and units is PPP multilink: one can
connect several channels to a given unit. In this case, the unit
will use all channels when sending packets. That's the PPP way to
do link aggregation.

Overlays
========

It's possible to encapsulate PPP into other protocols. For example,
the Linux kernel supports PPTP, L2TP and PPPoE. PPTP is (was?) often
used for VPNs. PPPoE and L2TP are typically used by ISPs to provide
DSL connections. The kernel implementation of these protocols provides
the lower layer necessary for PPP channels to send and receive data.

There's an ISP use case that isn't covered by the current
implementation though: "bridging" a PPPoE and an L2TP session together
(or two L2TP sessions together). This is used to stretch a PPP
connection across an IP network (essentially simulating circuit
switching on top of IP).

Tom's RFC addresses this use case, by adding a new ioctl to bridge two
channels together (each channel can run on a different lower layer
technology). Units aren't necessary in this use case, because only
the LCP and authentication protocols need to run on the hosts that do
only bridging: once the authentication succeeds, every packet received
on one channel is forwarded over the other channel. NCPs are still
negotiated end to end.

The other solution envisioned in this thread (virtual L2TP and PPPoE
devices in collect_md mode) wouldn't use ppp_generic.c at all:
act_mirred would directly redirect the PPP packets between the virtual
PPPoE or L2TP devices. I don't have any code for this approach though.

Pointers
========

  * Documentation/networking/ppp_generic.rst:
      Documentation for the kernel implementation (including the ioctls).

  * drivers/net/ppp/*:
      Kernel implementation of PPP, PPPoE, PPTP...

  * net/l2tp/*:
      Kernel implementation of L2TP (v2 and v3). The PPP-specific part
      is in l2tp_ppp.c.

  * RFC 1661:
      The PPP specification, including LCP (IPCP is in RFC 1332, IPv6CP
      in RFC 5072, MPLSCP in RFC 3032 section 4).

  * RFC 1990:
      The PPP multilink specification.

  * RFC 2516:
      The PPPoE specification.

  * RFC 2637:
      The PPTP specification.

  * RFC 2661:
    The L2TPv2 specification (L2TPv3, defined in RFC 3931, was defined
    later as a generalisation of L2TPv2 that could transport more than
    just PPP).

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

[Jakub Kicinski]

On Wed, 18 Nov 2020 21:24:53 +0100 Guillaume Nault wrote:
> Here's a high level view of:
>   * the protocol,
>   * the kernel implementation,
>   * the context of this RFC,
>   * and a few pointers at the end :)
> 
> Hope this helps. I've tried to keep it short. Feel free to ask for
> clarifications and details.

Thanks of the write up, much appreciated!