* [PATCH net-next 0/2] add ppp_generic ioctl(s) to bridge channels
@ 2020-11-26 12:24 Tom Parkin
2020-11-26 12:24 ` [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls Tom Parkin
2020-11-26 12:24 ` [PATCH net-next 2/2] docs: update ppp_generic.rst to document new ioctls Tom Parkin
0 siblings, 2 replies; 7+ messages in thread
From: Tom Parkin @ 2020-11-26 12:24 UTC (permalink / raw)
To: netdev; +Cc: gnault, jchapman, 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
Tom Parkin (2):
ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
docs: update ppp_generic.rst to document new ioctls
Documentation/networking/ppp_generic.rst | 9 ++
drivers/net/ppp/ppp_generic.c | 143 ++++++++++++++++++++++-
include/uapi/linux/ppp-ioctl.h | 2 +
3 files changed, 152 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
2020-11-26 12:24 [PATCH net-next 0/2] add ppp_generic ioctl(s) to bridge channels Tom Parkin
@ 2020-11-26 12:24 ` Tom Parkin
2020-11-26 14:51 ` kernel test robot
2020-11-27 19:31 ` Guillaume Nault
2020-11-26 12:24 ` [PATCH net-next 2/2] docs: update ppp_generic.rst to document new ioctls Tom Parkin
1 sibling, 2 replies; 7+ messages in thread
From: Tom Parkin @ 2020-11-26 12:24 UTC (permalink / raw)
To: netdev; +Cc: gnault, jchapman, 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 'downl' use is extended (rather than adding a new lock).
Order of lock acquisition is maintained: i.e. the channel 'upl' lock is
always acquired before 'downl' in code paths that need to hold both.
Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
drivers/net/ppp/ppp_generic.c | 147 ++++++++++++++++++++++++++++++++-
include/uapi/linux/ppp-ioctl.h | 2 +
2 files changed, 147 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 7d005896a0f9..5e563bfb8e2a 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -170,11 +170,12 @@ struct channel {
struct list_head list; /* link in all/new_channels list */
struct ppp_channel *chan; /* public channel data structure */
struct rw_semaphore chan_sem; /* protects `chan' during chan ioctl */
- spinlock_t downl; /* protects `chan', file.xq dequeue */
+ spinlock_t downl; /* protects `chan', 'bridge', file.xq dequeue */
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' */
+ 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 */
@@ -606,6 +607,95 @@ 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)
+{
+ int ret = -EALREADY;
+
+ /* We need to take each channel upl for access to the 'ppp' field,
+ * and each channel downl for write access to the 'bridge' field.
+ */
+
+ read_lock_bh(&pch->upl);
+ if (pch->ppp)
+ goto out0;
+
+ spin_lock(&pch->downl);
+
+ read_lock_bh(&pchb->upl);
+ if (pchb->ppp)
+ goto out1;
+
+ spin_lock(&pchb->downl);
+
+ if (pch->bridge || pchb->bridge)
+ goto out2;
+
+ rcu_assign_pointer(pch->bridge, pchb);
+ refcount_inc(&pchb->file.refcnt);
+
+ rcu_assign_pointer(pchb->bridge, pch);
+ refcount_inc(&pch->file.refcnt);
+
+ ret = 0;
+
+out2:
+ spin_unlock(&pchb->downl);
+out1:
+ read_unlock_bh(&pchb->upl);
+ spin_unlock(&pch->downl);
+out0:
+ read_unlock_bh(&pch->upl);
+
+ return ret;
+}
+
+static int ppp_unbridge_channels(struct channel *pch)
+{
+ struct channel *pchb;
+
+ rcu_read_lock();
+
+ pchb = rcu_dereference(pch->bridge);
+ if (!pchb) {
+ rcu_read_unlock();
+ return -ENXIO;
+ }
+
+ if (pch != rcu_dereference(pchb->bridge)) {
+ rcu_read_unlock();
+ return -ENXIO;
+ }
+
+ spin_lock(&pch->downl);
+ spin_lock(&pchb->downl);
+
+ rcu_assign_pointer(pch->bridge, NULL);
+ rcu_assign_pointer(pchb->bridge, NULL);
+
+ spin_unlock(&pchb->downl);
+ spin_unlock(&pch->downl);
+
+ rcu_read_unlock();
+
+ synchronize_rcu();
+
+ 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 +731,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 +748,22 @@ 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);
+ if (pchb)
+ err = ppp_bridge_channels(pch, pchb);
+ spin_unlock_bh(&pn->all_channels_lock);
+ break;
+
+ case PPPIOCUNBRIDGECHAN:
+ err = ppp_unbridge_channels(pch);
+ break;
+
default:
down_read(&pch->chan_sem);
chan = pch->chan;
@@ -2089,6 +2196,35 @@ 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) {
+ spin_lock(&pchb->downl);
+ if (pchb->chan) {
+ skb_scrub_packet(skb, !net_eq(pch->chan_net, pchb->chan_net));
+ if (!pchb->chan->ops->start_xmit(pchb->chan, skb))
+ kfree_skb(skb);
+ } else {
+ /* channel got unregistered */
+ kfree_skb(skb);
+ }
+ spin_unlock(&pchb->downl);
+ }
+ 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 +2236,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 +2936,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);
}
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 2/2] docs: update ppp_generic.rst to document new ioctls
2020-11-26 12:24 [PATCH net-next 0/2] add ppp_generic ioctl(s) to bridge channels Tom Parkin
2020-11-26 12:24 ` [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls Tom Parkin
@ 2020-11-26 12:24 ` Tom Parkin
1 sibling, 0 replies; 7+ messages in thread
From: Tom Parkin @ 2020-11-26 12:24 UTC (permalink / raw)
To: netdev; +Cc: gnault, jchapman, 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 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/networking/ppp_generic.rst b/Documentation/networking/ppp_generic.rst
index e60504377900..be25f48468a6 100644
--- a/Documentation/networking/ppp_generic.rst
+++ b/Documentation/networking/ppp_generic.rst
@@ -314,6 +314,15 @@ 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. 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.
+
+* PPPIOCUNBRIDGECHAN performs the inverse of PPPIOCBRIDGECHAN, unbridging
+ a channel pair.
+
* 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
2020-11-26 12:24 ` [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls Tom Parkin
@ 2020-11-26 14:51 ` kernel test robot
2020-11-27 19:31 ` Guillaume Nault
1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-11-26 14:51 UTC (permalink / raw)
To: Tom Parkin, netdev; +Cc: kbuild-all, gnault, jchapman, Tom Parkin
[-- Attachment #1: Type: text/plain, Size: 5045 bytes --]
Hi Tom,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tom-Parkin/add-ppp_generic-ioctl-s-to-bridge-channels/20201126-202933
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 594e31bcebd6b8127ab8bcf37068ecef6c996459
config: alpha-randconfig-s032-20201126 (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-151-g540c2c4b-dirty
# https://github.com/0day-ci/linux/commit/d956f9918541d0b973ce241bd26020c3d08878db
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tom-Parkin/add-ppp_generic-ioctl-s-to-bridge-channels/20201126-202933
git checkout d956f9918541d0b973ce241bd26020c3d08878db
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=alpha
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
"sparse warnings: (new ones prefixed by >>)"
>> drivers/net/ppp/ppp_generic.c:641:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> drivers/net/ppp/ppp_generic.c:641:9: sparse: struct channel [noderef] __rcu *
>> drivers/net/ppp/ppp_generic.c:641:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:644:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:644:9: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:644:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:666:16: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:666:16: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:666:16: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:672:20: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:672:20: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:672:20: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:680:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:680:9: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:680:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:681:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:681:9: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:681:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:2209:16: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:2209:16: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:2209:16: sparse: struct channel *
vim +641 drivers/net/ppp/ppp_generic.c
609
610 /* Bridge one PPP channel to another.
611 * When two channels are bridged, ppp_input on one channel is redirected to
612 * the other's ops->start_xmit handler.
613 * In order to safely bridge channels we must reject channels which are already
614 * part of a bridge instance, or which form part of an existing unit.
615 * Once successfully bridged, each channel holds a reference on the other
616 * to prevent it being freed while the bridge is extant.
617 */
618 static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
619 {
620 int ret = -EALREADY;
621
622 /* We need to take each channel upl for access to the 'ppp' field,
623 * and each channel downl for write access to the 'bridge' field.
624 */
625
626 read_lock_bh(&pch->upl);
627 if (pch->ppp)
628 goto out0;
629
630 spin_lock(&pch->downl);
631
632 read_lock_bh(&pchb->upl);
633 if (pchb->ppp)
634 goto out1;
635
636 spin_lock(&pchb->downl);
637
638 if (pch->bridge || pchb->bridge)
639 goto out2;
640
> 641 rcu_assign_pointer(pch->bridge, pchb);
642 refcount_inc(&pchb->file.refcnt);
643
644 rcu_assign_pointer(pchb->bridge, pch);
645 refcount_inc(&pch->file.refcnt);
646
647 ret = 0;
648
649 out2:
650 spin_unlock(&pchb->downl);
651 out1:
652 read_unlock_bh(&pchb->upl);
653 spin_unlock(&pch->downl);
654 out0:
655 read_unlock_bh(&pch->upl);
656
657 return ret;
658 }
659
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26302 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
@ 2020-11-26 14:51 ` kernel test robot
0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-11-26 14:51 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5153 bytes --]
Hi Tom,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Tom-Parkin/add-ppp_generic-ioctl-s-to-bridge-channels/20201126-202933
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 594e31bcebd6b8127ab8bcf37068ecef6c996459
config: alpha-randconfig-s032-20201126 (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-151-g540c2c4b-dirty
# https://github.com/0day-ci/linux/commit/d956f9918541d0b973ce241bd26020c3d08878db
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tom-Parkin/add-ppp_generic-ioctl-s-to-bridge-channels/20201126-202933
git checkout d956f9918541d0b973ce241bd26020c3d08878db
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=alpha
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
"sparse warnings: (new ones prefixed by >>)"
>> drivers/net/ppp/ppp_generic.c:641:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
>> drivers/net/ppp/ppp_generic.c:641:9: sparse: struct channel [noderef] __rcu *
>> drivers/net/ppp/ppp_generic.c:641:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:644:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:644:9: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:644:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:666:16: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:666:16: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:666:16: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:672:20: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:672:20: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:672:20: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:680:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:680:9: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:680:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:681:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:681:9: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:681:9: sparse: struct channel *
drivers/net/ppp/ppp_generic.c:2209:16: sparse: sparse: incompatible types in comparison expression (different address spaces):
drivers/net/ppp/ppp_generic.c:2209:16: sparse: struct channel [noderef] __rcu *
drivers/net/ppp/ppp_generic.c:2209:16: sparse: struct channel *
vim +641 drivers/net/ppp/ppp_generic.c
609
610 /* Bridge one PPP channel to another.
611 * When two channels are bridged, ppp_input on one channel is redirected to
612 * the other's ops->start_xmit handler.
613 * In order to safely bridge channels we must reject channels which are already
614 * part of a bridge instance, or which form part of an existing unit.
615 * Once successfully bridged, each channel holds a reference on the other
616 * to prevent it being freed while the bridge is extant.
617 */
618 static int ppp_bridge_channels(struct channel *pch, struct channel *pchb)
619 {
620 int ret = -EALREADY;
621
622 /* We need to take each channel upl for access to the 'ppp' field,
623 * and each channel downl for write access to the 'bridge' field.
624 */
625
626 read_lock_bh(&pch->upl);
627 if (pch->ppp)
628 goto out0;
629
630 spin_lock(&pch->downl);
631
632 read_lock_bh(&pchb->upl);
633 if (pchb->ppp)
634 goto out1;
635
636 spin_lock(&pchb->downl);
637
638 if (pch->bridge || pchb->bridge)
639 goto out2;
640
> 641 rcu_assign_pointer(pch->bridge, pchb);
642 refcount_inc(&pchb->file.refcnt);
643
644 rcu_assign_pointer(pchb->bridge, pch);
645 refcount_inc(&pch->file.refcnt);
646
647 ret = 0;
648
649 out2:
650 spin_unlock(&pchb->downl);
651 out1:
652 read_unlock_bh(&pchb->upl);
653 spin_unlock(&pch->downl);
654 out0:
655 read_unlock_bh(&pch->upl);
656
657 return ret;
658 }
659
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 26302 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
2020-11-26 12:24 ` [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls Tom Parkin
2020-11-26 14:51 ` kernel test robot
@ 2020-11-27 19:31 ` Guillaume Nault
2020-11-30 9:59 ` Tom Parkin
1 sibling, 1 reply; 7+ messages in thread
From: Guillaume Nault @ 2020-11-27 19:31 UTC (permalink / raw)
To: Tom Parkin; +Cc: netdev, jchapman
On Thu, Nov 26, 2020 at 12:24:25PM +0000, Tom Parkin wrote:
> 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.
Thanks!
Some comments below (mostly about locking).
> 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 'downl' use is extended (rather than adding a new lock).
> Order of lock acquisition is maintained: i.e. the channel 'upl' lock is
> always acquired before 'downl' in code paths that need to hold both.
>
> Signed-off-by: Tom Parkin <tparkin@katalix.com>
> ---
> drivers/net/ppp/ppp_generic.c | 147 ++++++++++++++++++++++++++++++++-
> include/uapi/linux/ppp-ioctl.h | 2 +
> 2 files changed, 147 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 7d005896a0f9..5e563bfb8e2a 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -170,11 +170,12 @@ struct channel {
> struct list_head list; /* link in all/new_channels list */
> struct ppp_channel *chan; /* public channel data structure */
> struct rw_semaphore chan_sem; /* protects `chan' during chan ioctl */
> - spinlock_t downl; /* protects `chan', file.xq dequeue */
> + spinlock_t downl; /* protects `chan', 'bridge', file.xq dequeue */
> 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' */
> + struct channel *bridge; /* "bridged" ppp channel */
Missing __rcu annotation (as reported by kernel test robot):
struct channel __rcu *bridge;
With RCU protection, it might make sense to use ->upl, instead of
->downl, to protect the update side. Since ->upl is used to protect the
pointer to the parent unit, it probably makes sense to use it for
->bridge too, which somehow replaces the parent unit (as both are
mutually exclusive). Also, using ->upl would avoid some lock nesting
when updating ->bridge.
> #ifdef CONFIG_PPP_MULTILINK
> u8 avail; /* flag used in multilink stuff */
> u8 had_frag; /* >= 1 fragments have been sent */
> @@ -606,6 +607,95 @@ 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)
> +{
> + int ret = -EALREADY;
> +
> + /* We need to take each channel upl for access to the 'ppp' field,
> + * and each channel downl for write access to the 'bridge' field.
> + */
> +
> + read_lock_bh(&pch->upl);
> + if (pch->ppp)
> + goto out0;
> +
> + spin_lock(&pch->downl);
> +
> + read_lock_bh(&pchb->upl);
> + if (pchb->ppp)
> + goto out1;
You're verifying that ->ppp isn't set, however, you haven't added a
test in ppp_connect_channel() to avoid setting ->ppp when ->bridge
is already set. Therefore, it'd still be possible to set both ->ppp and
->bridge on a channel.
> + spin_lock(&pchb->downl);
> +
> + if (pch->bridge || pchb->bridge)
> + goto out2;
> +
> + rcu_assign_pointer(pch->bridge, pchb);
> + refcount_inc(&pchb->file.refcnt);
> +
> + rcu_assign_pointer(pchb->bridge, pch);
> + refcount_inc(&pch->file.refcnt);
> +
> + ret = 0;
> +
> +out2:
> + spin_unlock(&pchb->downl);
> +out1:
> + read_unlock_bh(&pchb->upl);
> + spin_unlock(&pch->downl);
> +out0:
> + read_unlock_bh(&pch->upl);
> +
> + return ret;
> +}
Locking looks dangerous here: given that ppp_bridge_channels() is
called with pn->all_channels_lock held, that's 5 nested locks.
Since we have to hold the channels anyway, why not incrementing the
refcount immediately and unlock everything as soon as possible?
That is, instead of doing:
LOCK(all_channels_lock)
LOCK(channel->upl)
LOCK(channel->downl)
LOCK(bridge->upl)
LOCK(bridge->downl)
assign_pointers
UNLOCK()
...
UNLOCK()
what about something like:
LOCK(all_channels_lock)
bridge = find_channel()
refcount_inc(&bridge->refcount)
UNLOCK(all_channels_lock)
LOCK(channel->upl)
LOCK(channel->downl)
set ->bridge
UNLOCK(channel->downl)
UNLOCK(channel->upl)
refcount_inc(&channel->refcount) // so that bridge holds a ref
LOCK(bridge->upl)
LOCK(bridge->downl)
set ->bridge
UNLOCK(bridge->downl)
UNLOCK(bridge->upl)
We could even avoid locking ->downl if ->bridge was protected directly
by ->upl. That way we'd avoid nesting locks entirely.
> +static int ppp_unbridge_channels(struct channel *pch)
> +{
> + struct channel *pchb;
> +
> + rcu_read_lock();
> +
> + pchb = rcu_dereference(pch->bridge);
> + if (!pchb) {
> + rcu_read_unlock();
> + return -ENXIO;
> + }
> +
> + if (pch != rcu_dereference(pchb->bridge)) {
> + rcu_read_unlock();
> + return -ENXIO;
> + }
Looks like we have a TOCTOU problem here: ->bridge might change before
we lock ->downl.
> + spin_lock(&pch->downl);
> + spin_lock(&pchb->downl);
I think we can have a deadlock here. Since ppp_unbridge_channels()
isn't running under the protection of an external lock, we could have
the bridge channel call this function concurrently. Then we'd have
lock inversion:
ppp_unbridge_channels(channel) ppp_unbridge_channels(bridge)
LOCK(channel->downl) LOCK(bridge->downl)
LOCK(bridge->downl) LOCK(channel->downl) // deadlock
Here again I think we should avoid nesting locks and clear each
->bridge pointer independently.
> + rcu_assign_pointer(pch->bridge, NULL);
> + rcu_assign_pointer(pchb->bridge, NULL);
Nit, we can use RCU_INIT_POINTER() when resetting a pointer with NULL.
> + spin_unlock(&pchb->downl);
> + spin_unlock(&pch->downl);
> +
> + rcu_read_unlock();
> +
> + synchronize_rcu();
> +
> + 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 +731,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 +748,22 @@ 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);
> + if (pchb)
> + err = ppp_bridge_channels(pch, pchb);
> + spin_unlock_bh(&pn->all_channels_lock);
> + break;
> +
> + case PPPIOCUNBRIDGECHAN:
> + err = ppp_unbridge_channels(pch);
> + break;
> +
> default:
> down_read(&pch->chan_sem);
> chan = pch->chan;
> @@ -2089,6 +2196,35 @@ 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) {
> + spin_lock(&pchb->downl);
> + if (pchb->chan) {
> + skb_scrub_packet(skb, !net_eq(pch->chan_net, pchb->chan_net));
> + if (!pchb->chan->ops->start_xmit(pchb->chan, skb))
> + kfree_skb(skb);
> + } else {
> + /* channel got unregistered */
> + kfree_skb(skb);
> + }
> + spin_unlock(&pchb->downl);
> + }
> + rcu_read_unlock();
> +
> + /* If pchb is set then we've consumed the packet */
> + return pchb;
> +}
Maybe "return !!pchb;". I always find it unexpected to store a pointer
into a bool. But maybe it's just me.
Also, I believe the code could be made more readable by returning early
in unhandled cases, instead of nesting all the conditions.
> void
> ppp_input(struct ppp_channel *chan, struct sk_buff *skb)
> {
> @@ -2100,6 +2236,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 +2936,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);
> }
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls
2020-11-27 19:31 ` Guillaume Nault
@ 2020-11-30 9:59 ` Tom Parkin
0 siblings, 0 replies; 7+ messages in thread
From: Tom Parkin @ 2020-11-30 9:59 UTC (permalink / raw)
To: Guillaume Nault; +Cc: netdev, jchapman
[-- Attachment #1: Type: text/plain, Size: 498 bytes --]
On Fri, Nov 27, 2020 at 20:31:34 +0100, Guillaume Nault wrote:
> On Thu, Nov 26, 2020 at 12:24:25PM +0000, Tom Parkin wrote:
> > 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.
>
> Thanks!
> Some comments below (mostly about locking).
Thanks for your review Guillaume. I'll work on integrating your
comments (and a fix for the build test robot warning) into a v2 series.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-30 10:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 12:24 [PATCH net-next 0/2] add ppp_generic ioctl(s) to bridge channels Tom Parkin
2020-11-26 12:24 ` [PATCH net-next 1/2] ppp: add PPPIOCBRIDGECHAN and PPPIOCUNBRIDGECHAN ioctls Tom Parkin
2020-11-26 14:51 ` kernel test robot
2020-11-26 14:51 ` kernel test robot
2020-11-27 19:31 ` Guillaume Nault
2020-11-30 9:59 ` Tom Parkin
2020-11-26 12:24 ` [PATCH net-next 2/2] docs: update ppp_generic.rst to document new ioctls Tom Parkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.