[PATCH v4 net-next 0/2] add ppp_generic ioctl(s) to bridge channels

[PATCH v4 net-next 0/2] add ppp_generic ioctl(s) to bridge channels

[Tom Parkin]

Following on from my previous RFC[1], this series adds two ioctl calls
to the ppp code to implement "channel bridging".

When two ppp channels are bridged, frames presented to ppp_input() on
one channel are passed to the other channel's ->start_xmit function for
transmission.

The primary use-case for this functionality is in an L2TP Access
Concentrator where PPP frames are typically presented in a PPPoE session
(e.g. from a home broadband user) and are forwarded to the ISP network in
a PPPoL2TP session.

The two new ioctls, PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN form a
symmetric pair.

Userspace code testing and illustrating use of the ioctl calls is
available in the go-l2tp[2] and l2tp-ktest[3] repositories.

[1]. Previous RFC series:

https://lore.kernel.org/netdev/20201106181647.16358-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.
Support for the PPPIOCBRIDGECHAN ioctl is on a branch:

https://github.com/katalix/l2tp-ktest/tree/tp_ac_pppoe_tests_2

Changelog:

v4:
    * Fix NULL-pointer access in PPPIOCBRIDGECHAN in the case that the
      ID of the channel to be bridged wasn't found.
    * Add comment in ppp_unbridge_channels to better document the
      unbridge process.

v3:
    * Use rcu_dereference_protected for accessing struct channel
      'bridge' field during updates with lock 'upl' held.
    * Avoid race in ppp_unbridge_channels by ensuring that each channel
      in the bridge points to it's peer before decrementing refcounts.

v2:
    * Add missing __rcu annotation to struct channel 'bridge' field in
      order to squash a sparse warning from a C=1 build
    * Integrate review comments from gnault@redhat.com
    * Have ppp_unbridge_channels return -EINVAL if the channel isn't
      part of a bridge: this better aligns with the return code from
      ppp_disconnect_channel.
    * Improve docs update by including information on ioctl arguments
      and error return codes.

Tom Parkin (2):
  ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
  docs: update ppp_generic.rst to document new ioctls

 Documentation/networking/ppp_generic.rst |  16 +++
 drivers/net/ppp/ppp_generic.c            | 152 ++++++++++++++++++++++-
 include/uapi/linux/ppp-ioctl.h           |   2 +
 3 files changed, 167 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v4 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls

[Tom Parkin]

This new ioctl pair 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 channels
are unbridged, which can occur either explicitly on userspace calling
the PPPIOCUNBRIDGECHAN ioctl, or implicitly when either channel in the
bridge is unregistered.

In order to implement the channel bridge, struct channel is extended
with a new field, 'bridge', which points to the other struct channel
making up the bridge.

This pointer is RCU protected to avoid adding another lock to the data
path.

To guard against concurrent writes to the pointer, the existing struct
channel lock 'upl' coverage is extended rather than adding a new lock.

The 'upl' lock is used to protect the existing unit pointer.  Since the
bridge effectively replaces the unit (they're mutually exclusive for a
channel) it makes coding easier to use the same lock to cover them
both.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 drivers/net/ppp/ppp_generic.c  | 152 ++++++++++++++++++++++++++++++++-
 include/uapi/linux/ppp-ioctl.h |   2 +
 2 files changed, 151 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 7d005896a0f9..09c27f7773f9 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -174,7 +174,8 @@ struct channel {
 	struct ppp	*ppp;		/* ppp unit we're connected to */
 	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' */
+	rwlock_t	upl;		/* protects `ppp' and 'bridge' */
+	struct channel __rcu *bridge;	/* "bridged" ppp channel */
 #ifdef CONFIG_PPP_MULTILINK
 	u8		avail;		/* flag used in multilink stuff */
 	u8		had_frag;	/* >= 1 fragments have been sent */
@@ -606,6 +607,83 @@ static struct bpf_prog *compat_ppp_get_filter(struct sock_fprog32 __user *p)
 #endif
 #endif
 
+/* Bridge one PPP channel to another.
+ * When two channels are bridged, ppp_input on one channel is redirected to
+ * the other's ops->start_xmit handler.
+ * In order to safely bridge channels we must reject channels which are already
+ * part of a bridge instance, or which form part of an existing unit.
+ * Once successfully bridged, each channel holds a reference on the other
+ * to prevent it being freed while the bridge is extant.
+ */
+static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
+{
+	write_lock_bh(&pch->upl);
+	if (pch->ppp ||
+	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {
+		write_unlock_bh(&pch->upl);
+		return -EALREADY;
+	}
+	rcu_assign_pointer(pch->bridge, pchb);
+	write_unlock_bh(&pch->upl);
+
+	write_lock_bh(&pchb->upl);
+	if (pchb->ppp ||
+	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {
+		write_unlock_bh(&pchb->upl);
+		goto err_unset;
+	}
+	rcu_assign_pointer(pchb->bridge, pch);
+	write_unlock_bh(&pchb->upl);
+
+	refcount_inc(&pch->file.refcnt);
+	refcount_inc(&pchb->file.refcnt);
+
+	return 0;
+
+err_unset:
+	write_lock_bh(&pch->upl);
+	RCU_INIT_POINTER(pch->bridge, NULL);
+	write_unlock_bh(&pch->upl);
+	synchronize_rcu();
+	return -EALREADY;
+}
+
+static int ppp_unbridge_channels(struct channel *pch)
+{
+	struct channel *pchb, *pchbb;
+
+	write_lock_bh(&pch->upl);
+	pchb = rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl));
+	if (!pchb) {
+		write_unlock_bh(&pch->upl);
+		return -EINVAL;
+	}
+	RCU_INIT_POINTER(pch->bridge, NULL);
+	write_unlock_bh(&pch->upl);
+
+	/* Only modify pchb if phcb->bridge points back to pch.
+	 * If not, it implies that there has been a race unbridging (and possibly
+	 * even rebridging) pchb.  We should leave pchb alone to avoid either a
+	 * refcount underflow, or breaking another established bridge instance.
+	 */
+	write_lock_bh(&pchb->upl);
+	pchbb = rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl));
+	if (pchbb == pch)
+		RCU_INIT_POINTER(pchb->bridge, NULL);
+	write_unlock_bh(&pchb->upl);
+
+	synchronize_rcu();
+
+	if (pchbb == pch)
+		if (refcount_dec_and_test(&pch->file.refcnt))
+			ppp_destroy_channel(pch);
+
+	if (refcount_dec_and_test(&pchb->file.refcnt))
+		ppp_destroy_channel(pchb);
+
+	return 0;
+}
+
 static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct ppp_file *pf;
@@ -641,8 +719,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 +736,31 @@ 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;
+			pn = ppp_pernet(current->nsproxy->net_ns);
+			spin_lock_bh(&pn->all_channels_lock);
+			pchb = ppp_find_channel(pn, unit);
+			/* Hold a reference to prevent pchb being freed while
+			 * we establish the bridge.
+			 */
+			if (pchb)
+				refcount_inc(&pchb->file.refcnt);
+			spin_unlock_bh(&pn->all_channels_lock);
+			if (!pchb)
+				break;
+			err = ppp_bridge_channels(pch, pchb);
+			/* Drop earlier refcount now bridge establishment is complete */
+			if (refcount_dec_and_test(&pchb->file.refcnt))
+				ppp_destroy_channel(pchb);
+			break;
+
+		case PPPIOCUNBRIDGECHAN:
+			err = ppp_unbridge_channels(pch);
+			break;
+
 		default:
 			down_read(&pch->chan_sem);
 			chan = pch->chan;
@@ -2089,6 +2193,40 @@ static bool ppp_decompress_proto(struct sk_buff *skb)
 	return pskb_may_pull(skb, 2);
 }
 
+/* Attempt to handle a frame via. a bridged channel, if one exists.
+ * If the channel is bridged, the frame is consumed by the bridge.
+ * If not, the caller must handle the frame by normal recv mechanisms.
+ * Returns true if the frame is consumed, false otherwise.
+ */
+static bool ppp_channel_bridge_input(struct channel *pch, struct sk_buff *skb)
+{
+	struct channel *pchb;
+
+	rcu_read_lock();
+	pchb = rcu_dereference(pch->bridge);
+	if (!pchb)
+		goto out_rcu;
+
+	spin_lock(&pchb->downl);
+	if (!pchb->chan) {
+		/* channel got unregistered */
+		kfree_skb(skb);
+		goto outl;
+	}
+
+	skb_scrub_packet(skb, !net_eq(pch->chan_net, pchb->chan_net));
+	if (!pchb->chan->ops->start_xmit(pchb->chan, skb))
+		kfree_skb(skb);
+
+outl:
+	spin_unlock(&pchb->downl);
+out_rcu:
+	rcu_read_unlock();
+
+	/* If pchb is set then we've consumed the packet */
+	return !!pchb;
+}
+
 void
 ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 {
@@ -2100,6 +2238,10 @@ ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
 		return;
 	}
 
+	/* If the channel is bridged, transmit via. bridge */
+	if (ppp_channel_bridge_input(pch, skb))
+		return;
+
 	read_lock_bh(&pch->upl);
 	if (!ppp_decompress_proto(skb)) {
 		kfree_skb(skb);
@@ -2796,8 +2938,11 @@ ppp_unregister_channel(struct ppp_channel *chan)
 	list_del(&pch->list);
 	spin_unlock_bh(&pn->all_channels_lock);
 
+	ppp_unbridge_channels(pch);
+
 	pch->file.dead = 1;
 	wake_up_interruptible(&pch->file.rwait);
+
 	if (refcount_dec_and_test(&pch->file.refcnt))
 		ppp_destroy_channel(pch);
 }
@@ -3270,7 +3415,8 @@ ppp_connect_channel(struct channel *pch, int unit)
 		goto out;
 	write_lock_bh(&pch->upl);
 	ret = -EINVAL;
-	if (pch->ppp)
+	if (pch->ppp ||
+	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl)))
 		goto outl;
 
 	ppp_lock(ppp);
diff --git a/include/uapi/linux/ppp-ioctl.h b/include/uapi/linux/ppp-ioctl.h
index 7bd2a5a75348..8dbecb3ad036 100644
--- a/include/uapi/linux/ppp-ioctl.h
+++ b/include/uapi/linux/ppp-ioctl.h
@@ -115,6 +115,8 @@ 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 PPPIOCUNBRIDGECHAN _IO('t', 54)	/* unbridge channel */
 
 #define SIOCGPPPSTATS   (SIOCDEVPRIVATE + 0)
 #define SIOCGPPPVER     (SIOCDEVPRIVATE + 1)	/* NEVER change this!! */
-- 
2.17.1

[PATCH v4 net-next 2/2] docs: update ppp_generic.rst to document new ioctls

[Tom Parkin]

Add documentation of the newly-added PPPIOCBRIDGECHAN and
PPPIOCUNBRIDGECHAN ioctls.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 Documentation/networking/ppp_generic.rst | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/networking/ppp_generic.rst b/Documentation/networking/ppp_generic.rst
index e60504377900..5a10abce5964 100644
--- a/Documentation/networking/ppp_generic.rst
+++ b/Documentation/networking/ppp_generic.rst
@@ -314,6 +314,22 @@ 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. The argument should
+  point to an int containing the channel number of the channel to bridge
+  to. Once two channels are bridged, frames presented to one channel by
+  ppp_input() are passed to the bridge instance for onward transmission.
+  This allows frames to be switched from one channel into another: for
+  example, to pass PPPoE frames into a PPPoL2TP session. Since channel
+  bridging interrupts the normal ppp_input() path, a given channel may
+  not be part of a bridge at the same time as being part of a unit.
+  This ioctl will return an EALREADY error if the channel is already
+  part of a bridge or unit, or ENXIO if the requested channel does not
+  exist.
+
+* PPPIOCUNBRIDGECHAN performs the inverse of PPPIOCBRIDGECHAN, unbridging
+  a channel pair.  This ioctl will return an EINVAL error if the channel
+  does not form part of a bridge.
+
 * 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: [PATCH v4 net-next 0/2] add ppp_generic ioctl(s) to bridge channels

[Guillaume Nault]

On Thu, Dec 10, 2020 at 03:50:56PM +0000, Tom Parkin wrote:
> Following on from my previous RFC[1], this series adds two ioctl calls
> to the ppp code to implement "channel bridging".
> 
> When two ppp channels are bridged, frames presented to ppp_input() on
> one channel are passed to the other channel's ->start_xmit function for
> transmission.
> 
> The primary use-case for this functionality is in an L2TP Access
> Concentrator where PPP frames are typically presented in a PPPoE session
> (e.g. from a home broadband user) and are forwarded to the ISP network in
> a PPPoL2TP session.

Looks good to me now. Thanks Tom!

Reviewed-by: Guillaume Nault <gnault@redhat.com>

Re: [PATCH v4 net-next 0/2] add ppp_generic ioctl(s) to bridge channels

[Tom Parkin]

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

On  Thu, Dec 10, 2020 at 18:13:09 +0100, Guillaume Nault wrote:
> On Thu, Dec 10, 2020 at 03:50:56PM +0000, Tom Parkin wrote:
> > Following on from my previous RFC[1], this series adds two ioctl calls
> > to the ppp code to implement "channel bridging".
> > 
> > When two ppp channels are bridged, frames presented to ppp_input() on
> > one channel are passed to the other channel's ->start_xmit function for
> > transmission.
> > 
> > The primary use-case for this functionality is in an L2TP Access
> > Concentrator where PPP frames are typically presented in a PPPoE session
> > (e.g. from a home broadband user) and are forwarded to the ISP network in
> > a PPPoL2TP session.
> 
> Looks good to me now. Thanks Tom!
> 
> Reviewed-by: Guillaume Nault <gnault@redhat.com>
> 

Thanks again for your review and help with the series :-)

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

Re: [PATCH v4 net-next 0/2] add ppp_generic ioctl(s) to bridge channels

[David Miller]

From: Tom Parkin <tparkin@katalix.com>
Date: Thu, 10 Dec 2020 17:16:45 +0000

> On  Thu, Dec 10, 2020 at 18:13:09 +0100, Guillaume Nault wrote:
>> On Thu, Dec 10, 2020 at 03:50:56PM +0000, Tom Parkin wrote:
>> > Following on from my previous RFC[1], this series adds two ioctl calls
>> > to the ppp code to implement "channel bridging".
>> > 
>> > When two ppp channels are bridged, frames presented to ppp_input() on
>> > one channel are passed to the other channel's ->start_xmit function for
>> > transmission.
>> > 
>> > The primary use-case for this functionality is in an L2TP Access
>> > Concentrator where PPP frames are typically presented in a PPPoE session
>> > (e.g. from a home broadband user) and are forwarded to the ISP network in
>> > a PPPoL2TP session.
>> 
>> Looks good to me now. Thanks Tom!
>> 
>> Reviewed-by: Guillaume Nault <gnault@redhat.com>
>> 
> 
> Thanks again for your review and help with the series :-)

Series applied.

Re: [PATCH v4 net-next 0/2] add ppp_generic ioctl(s) to bridge channels

[Tom Parkin]

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

On  Thu, Dec 10, 2020 at 14:21:34 -0800, David Miller wrote:
> From: Tom Parkin <tparkin@katalix.com>
> Date: Thu, 10 Dec 2020 17:16:45 +0000
> 
> > On  Thu, Dec 10, 2020 at 18:13:09 +0100, Guillaume Nault wrote:
> >> On Thu, Dec 10, 2020 at 03:50:56PM +0000, Tom Parkin wrote:
> >> > Following on from my previous RFC[1], this series adds two ioctl calls
> >> > to the ppp code to implement "channel bridging".
> >> > 
> >> > When two ppp channels are bridged, frames presented to ppp_input() on
> >> > one channel are passed to the other channel's ->start_xmit function for
> >> > transmission.
> >> > 
> >> > The primary use-case for this functionality is in an L2TP Access
> >> > Concentrator where PPP frames are typically presented in a PPPoE session
> >> > (e.g. from a home broadband user) and are forwarded to the ISP network in
> >> > a PPPoL2TP session.
> >> 
> >> Looks good to me now. Thanks Tom!
> >> 
> >> Reviewed-by: Guillaume Nault <gnault@redhat.com>
> >> 
> > 
> > Thanks again for your review and help with the series :-)
> 
> Series applied.

Thanks Dave.  Nice to see you back :-)

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

Re: [PATCH v4 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls

[Simon Chopin]

Hello,

Le 10/12/2020 à 16:50, Tom Parkin a écrit :
> This new ioctl pair 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 channels
> are unbridged, which can occur either explicitly on userspace calling
> the PPPIOCUNBRIDGECHAN ioctl, or implicitly when either channel in the
> bridge is unregistered.
> 
> In order to implement the channel bridge, struct channel is extended
> with a new field, 'bridge', which points to the other struct channel
> making up the bridge.
> 
> This pointer is RCU protected to avoid adding another lock to the data
> path.
> 
> To guard against concurrent writes to the pointer, the existing struct
> channel lock 'upl' coverage is extended rather than adding a new lock.
> 
> The 'upl' lock is used to protect the existing unit pointer.  Since the
> bridge effectively replaces the unit (they're mutually exclusive for a
> channel) it makes coding easier to use the same lock to cover them
> both.
> 
> Signed-off-by: Tom Parkin <tparkin@katalix.com>
> ---
>  drivers/net/ppp/ppp_generic.c  | 152 ++++++++++++++++++++++++++++++++-
>  include/uapi/linux/ppp-ioctl.h |   2 +
>  2 files changed, 151 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 7d005896a0f9..09c27f7773f9 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -174,7 +174,8 @@ struct channel {
>  	struct ppp	*ppp;		/* ppp unit we're connected to */
>  	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' */
> +	rwlock_t	upl;		/* protects `ppp' and 'bridge' */
> +	struct channel __rcu *bridge;	/* "bridged" ppp channel */
>  #ifdef CONFIG_PPP_MULTILINK
>  	u8		avail;		/* flag used in multilink stuff */
>  	u8		had_frag;	/* >= 1 fragments have been sent */
> @@ -606,6 +607,83 @@ static struct bpf_prog *compat_ppp_get_filter(struct sock_fprog32 __user *p)
>  #endif
>  #endif
>  
> +/* Bridge one PPP channel to another.
> + * When two channels are bridged, ppp_input on one channel is redirected to
> + * the other's ops->start_xmit handler.
> + * In order to safely bridge channels we must reject channels which are already
> + * part of a bridge instance, or which form part of an existing unit.
> + * Once successfully bridged, each channel holds a reference on the other
> + * to prevent it being freed while the bridge is extant.
> + */
> +static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
> +{
> +	write_lock_bh(&pch->upl);
> +	if (pch->ppp ||
> +	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {
> +		write_unlock_bh(&pch->upl);
> +		return -EALREADY;
> +	}
> +	rcu_assign_pointer(pch->bridge, pchb);
> +	write_unlock_bh(&pch->upl);
> +
This is mostly for my own education:

I might be misunderstanding something, but is there anything
that would prevent a packet from being forwarded from pch to pchb at this
precise moment? If not, then it might be theoretically possible to have
any answer to said packet (say, a LCP ACK) to be received before the 
pchb->bridge is set?


> +	write_lock_bh(&pchb->upl);
> +	if (pchb->ppp ||
> +	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {
> +		write_unlock_bh(&pchb->upl);
> +		goto err_unset;
> +	}
> +	rcu_assign_pointer(pchb->bridge, pch);
> +	write_unlock_bh(&pchb->upl);
> +
> +	refcount_inc(&pch->file.refcnt);
> +	refcount_inc(&pchb->file.refcnt);
> +
> +	return 0;
> +
> +err_unset:
> +	write_lock_bh(&pch->upl);
> +	RCU_INIT_POINTER(pch->bridge, NULL);
> +	write_unlock_bh(&pch->upl);
> +	synchronize_rcu();
> +	return -EALREADY;
> +}

Re: [PATCH v4 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls

[Guillaume Nault]

On Mon, Jan 11, 2021 at 02:17:13PM +0100, Simon Chopin wrote:
> Hello,
> 
> Le 10/12/2020 à 16:50, Tom Parkin a écrit :
> > This new ioctl pair 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 channels
> > are unbridged, which can occur either explicitly on userspace calling
> > the PPPIOCUNBRIDGECHAN ioctl, or implicitly when either channel in the
> > bridge is unregistered.
> > 
> > In order to implement the channel bridge, struct channel is extended
> > with a new field, 'bridge', which points to the other struct channel
> > making up the bridge.
> > 
> > This pointer is RCU protected to avoid adding another lock to the data
> > path.
> > 
> > To guard against concurrent writes to the pointer, the existing struct
> > channel lock 'upl' coverage is extended rather than adding a new lock.
> > 
> > The 'upl' lock is used to protect the existing unit pointer.  Since the
> > bridge effectively replaces the unit (they're mutually exclusive for a
> > channel) it makes coding easier to use the same lock to cover them
> > both.
> > 
> > Signed-off-by: Tom Parkin <tparkin@katalix.com>
> > ---
> >  drivers/net/ppp/ppp_generic.c  | 152 ++++++++++++++++++++++++++++++++-
> >  include/uapi/linux/ppp-ioctl.h |   2 +
> >  2 files changed, 151 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> > index 7d005896a0f9..09c27f7773f9 100644
> > --- a/drivers/net/ppp/ppp_generic.c
> > +++ b/drivers/net/ppp/ppp_generic.c
> > @@ -174,7 +174,8 @@ struct channel {
> >  	struct ppp	*ppp;		/* ppp unit we're connected to */
> >  	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' */
> > +	rwlock_t	upl;		/* protects `ppp' and 'bridge' */
> > +	struct channel __rcu *bridge;	/* "bridged" ppp channel */
> >  #ifdef CONFIG_PPP_MULTILINK
> >  	u8		avail;		/* flag used in multilink stuff */
> >  	u8		had_frag;	/* >= 1 fragments have been sent */
> > @@ -606,6 +607,83 @@ static struct bpf_prog *compat_ppp_get_filter(struct sock_fprog32 __user *p)
> >  #endif
> >  #endif
> >  
> > +/* Bridge one PPP channel to another.
> > + * When two channels are bridged, ppp_input on one channel is redirected to
> > + * the other's ops->start_xmit handler.
> > + * In order to safely bridge channels we must reject channels which are already
> > + * part of a bridge instance, or which form part of an existing unit.
> > + * Once successfully bridged, each channel holds a reference on the other
> > + * to prevent it being freed while the bridge is extant.
> > + */
> > +static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
> > +{
> > +	write_lock_bh(&pch->upl);
> > +	if (pch->ppp ||
> > +	    rcu_dereference_protected(pch->bridge, lockdep_is_held(&pch->upl))) {
> > +		write_unlock_bh(&pch->upl);
> > +		return -EALREADY;
> > +	}
> > +	rcu_assign_pointer(pch->bridge, pchb);
> > +	write_unlock_bh(&pch->upl);
> > +
> This is mostly for my own education:
> 
> I might be misunderstanding something, but is there anything
> that would prevent a packet from being forwarded from pch to pchb at this
> precise moment? If not, then it might be theoretically possible to have
> any answer to said packet (say, a LCP ACK) to be received before the 
> pchb->bridge is set?

That's possible in theory. But I can't see any problem in practice,
because:

  * It's unlikely the round trip time would be small enough to trigger
    this situation.

  * If this situation ever happens, the reply is passed to user space,
    which is free to forward it to the other channel. If the user
    space implementation isn't prepared for this situation and just
    drops the packet, that's fine too. It's just a transient packet
    drop, which PPP peers are supposed to handle just fine (this can
    happen anywhere in the network after all).

  * Any use case I know for channel bridging involves a "live" channel
    (where LCP and authentication protocols are running) and an "idle"
    channel (where no protocol is running at all yet). So the problem
    can be avoided entirely by calling the PPPIOCBRIDGECHAN ioctl on
    the idle channel file descriptor, rather than on the live channel.

Or did you have any other possible problem in mind?

> > +	write_lock_bh(&pchb->upl);
> > +	if (pchb->ppp ||
> > +	    rcu_dereference_protected(pchb->bridge, lockdep_is_held(&pchb->upl))) {
> > +		write_unlock_bh(&pchb->upl);
> > +		goto err_unset;
> > +	}
> > +	rcu_assign_pointer(pchb->bridge, pch);
> > +	write_unlock_bh(&pchb->upl);
> > +
> > +	refcount_inc(&pch->file.refcnt);
> > +	refcount_inc(&pchb->file.refcnt);
> > +
> > +	return 0;
> > +
> > +err_unset:
> > +	write_lock_bh(&pch->upl);
> > +	RCU_INIT_POINTER(pch->bridge, NULL);
> > +	write_unlock_bh(&pch->upl);
> > +	synchronize_rcu();
> > +	return -EALREADY;
> > +}
>