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

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(-)

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
>

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.

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().

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.

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 :).