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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Jan 2021 14:17:13 +0100
From:   Simon Chopin <s.chopin@...halink.fr>
To:     Tom Parkin <tparkin@...alix.com>, netdev@...r.kernel.org
Cc:     gnault@...hat.com
Subject: Re: [PATCH v4 net-next 1/2] ppp: add PPPIOCBRIDGECHAN and
 PPPIOCUNBRIDGECHAN ioctls

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@...alix.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;
> +}