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

[Guillaume Nault <gnault@redhat.com>]

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
>