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