[PATCH net-next 0/6] l2tp: add ac/pppoe driver

[PATCH net-next 0/6] l2tp: add ac/pppoe driver

[Tom Parkin]

L2TPv2 tunnels are often used as a part of a home broadband connection,
using a PPP link to connect the subscriber network into the Internet
Service Provider's network.

In this scenario, PPPoE is widely used between the L2TP Access
Concentrator (LAC) and the subscriber.  The LAC effectively acts as a
PPPoE server, switching PPP frames from incoming PPPoE packets into an
L2TP session.  The PPP session is then terminated at the L2TP Network
Server (LNS) on the edge of the ISP's IP network.

This patchset adds a driver to the L2TP subsystem to support this mode
of operation.

The new driver, l2tp_ac_pppoe, adds support for the existing pseudowire
type L2TP_PWTYPE_PPP_AC, and is instantiated using the existing L2TP
netlink L2TP_CMD_SESSION_CREATE.  It is expected to be used as follows:

 * A userspace PPPoE daemon running on the LAC handles the PPPoE
   discovery process up to the point of assigning a PPPoE session ID and
   sending the PADS packet to the PPPoE peer to establish the PPPoE
   session.
 * Userspace code running on the LAC then instantiates an L2TP tunnel
   and session with the LNS using the L2TP control protocol.
 * Finally, the data path for PPPoE session frames through the L2TP
   session to the LAC is instantiated by sending a genetlink
   L2TP_CMD_SESSION_CREATE command to the kernel, including
   the PPPoE-specific metadata required for L2TP_PWTYPE_PPP_AC sessions
   (this is documented in the patch series commit comments).

Supporting this driver submission we have two examples of userspace
projects which use L2TP_PWTYPE_PPP_AC:

 * https://github.com/katalix/l2tp-ktest

   This is a unit-test suite for the kernel L2TP subsystem which has
   been updated to include basic lifetime and datapath tests for
   l2tp_ac_pppoe.

   The new tests are automatically enabled when l2tp_ac_pppoe
   availability is detected, and hence support for l2tp_ac_pppoe is on
   the master branch of the git repository.

 * https://github.com/katalix/go-l2tp

   This is a Go library for building L2TP applications on Linux, and
   includes a suite of example daemons which utilise the library.

   The daemon kpppoed implements the PPPoE discovery protocol, and spawns
   an instance of a daemon kl2tpd which handles the L2TP control protocol
   and instantiates the kernel data path.

   The code utilising l2tp_ac_pppoe is on the branch tp_002_pppoe_1
   pending merge of this patchset in the kernel.

Notes on the patchset itself:

 * Patches 1-4 lay groundwork for the addition of the new driver, making
   tweaks to the l2tp netlink code to allow l2tp_ac_pppoe to access the
   netlink attributes it requires.
 * Patch 5 adds the new driver itself and hooks it into the kernel
   configuration and build system.
 * Patch 6 updates the l2tp documentation under Documentation/ to
   include information about the new driver.

Tom Parkin (6):
  l2tp: add netlink info to session create callback
  l2tp: tweak netlink session create to allow L2TPv2 ac_pppoe
  l2tp: allow v2 netlink session create to pass ifname attribute
  l2tp: add netlink attributes for ac_ppp session creation
  l2tp: add ac_pppoe pseudowire driver
  docs: networking: update l2tp.rst to document PPP_AC pseudowires

 Documentation/networking/l2tp.rst |  69 +++--
 include/uapi/linux/l2tp.h         |   2 +
 net/l2tp/Kconfig                  |   7 +
 net/l2tp/Makefile                 |   1 +
 net/l2tp/l2tp_ac_pppoe.c          | 446 ++++++++++++++++++++++++++++++
 net/l2tp/l2tp_core.h              |   4 +-
 net/l2tp/l2tp_eth.c               |   3 +-
 net/l2tp/l2tp_netlink.c           |  20 +-
 net/l2tp/l2tp_ppp.c               |   3 +-
 9 files changed, 527 insertions(+), 28 deletions(-)
 create mode 100644 net/l2tp/l2tp_ac_pppoe.c

-- 
2.17.1

[PATCH net-next 1/6] l2tp: add netlink info to session create callback

[Tom Parkin]

To support creating different types of pseudowire, l2tp's netlink
commands for session create and destroy are implemented using callbacks
into pseudowire-specific code.

The pseudowire types implemented so far (PPP and Ethernet) use common
parameters which are extracted by l2tp_netlink into a session
configuration structure.

Different pseudowires may require different parameters: rather than
adding these to the common configuration structure, make the netlink
info visible to the pseudowire code so that it can perform its own
validation of the extra attributes it requires.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.h    | 4 +++-
 net/l2tp/l2tp_eth.c     | 3 ++-
 net/l2tp/l2tp_netlink.c | 3 ++-
 net/l2tp/l2tp_ppp.c     | 3 ++-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index cb21d906343e..bd62a3e2c98d 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -10,6 +10,7 @@
 
 #include <net/dst.h>
 #include <net/sock.h>
+#include <net/genetlink.h>
 
 #ifdef CONFIG_XFRM
 #include <net/xfrm.h>
@@ -196,7 +197,8 @@ struct l2tp_nl_cmd_ops {
 	 */
 	int (*session_create)(struct net *net, struct l2tp_tunnel *tunnel,
 			      u32 session_id, u32 peer_session_id,
-			      struct l2tp_session_cfg *cfg);
+			      struct l2tp_session_cfg *cfg,
+			      struct genl_info *info);
 
 	/* The pseudowire session delete callback is responsible for initiating the deletion
 	 * of a session instance.
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 6cd97c75445c..b2545e699174 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -243,7 +243,8 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
 
 static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
 			   u32 session_id, u32 peer_session_id,
-			   struct l2tp_session_cfg *cfg)
+			   struct l2tp_session_cfg *cfg,
+			   struct genl_info *info)
 {
 	unsigned char name_assign_type;
 	struct net_device *dev;
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 5ca5056e9636..7045eb105e6a 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -629,7 +629,8 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 	ret = l2tp_nl_cmd_ops[cfg.pw_type]->session_create(net, tunnel,
 							   session_id,
 							   peer_session_id,
-							   &cfg);
+							   &cfg,
+							   info);
 
 	if (ret >= 0) {
 		session = l2tp_tunnel_get_session(tunnel, session_id);
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index aea85f91f059..a2f7896d047e 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -853,7 +853,8 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 /* Called when creating sessions via the netlink interface. */
 static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
 				   u32 session_id, u32 peer_session_id,
-				   struct l2tp_session_cfg *cfg)
+				   struct l2tp_session_cfg *cfg,
+				   struct genl_info *info)
 {
 	int error;
 	struct l2tp_session *session;
-- 
2.17.1

[PATCH net-next 2/6] l2tp: tweak netlink session create to allow L2TPv2 ac_pppoe

[Tom Parkin]

When creating a session in an L2TPv2 tunnel, l2tp_netlink performs
some sanity checking of pseudowire type to prevent L2TPv3 pseudowires
from being instantiated in L2TPv2 tunnels.

To support PPPoE access concentrator functionality the L2TP subsystem
should allow PPP_AC pseudowires to be created in L2TPv2 tunnels.

Extend the l2tp_netlink sanity check to support PPP_AC.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_netlink.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 7045eb105e6a..8ef1a579a2b1 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -560,8 +560,14 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 		goto out_tunnel;
 	}
 
-	/* L2TPv2 only accepts PPP pseudo-wires */
-	if (tunnel->version == 2 && cfg.pw_type != L2TP_PWTYPE_PPP) {
+	/* L2TPv2 only accepts PPP pseudowires, which may be identified as
+	 * either L2TP_PWTYPE_PPP for locally-terminating PPP sessions, or
+	 * L2TP_PWTYPE_PPP_AC for the Access Concentrator case where the PPP
+	 * session is passed through the tunnel for remote termination.
+	 */
+	if (tunnel->version == 2 &&
+	    cfg.pw_type != L2TP_PWTYPE_PPP &&
+	    cfg.pw_type != L2TP_PWTYPE_PPP_AC) {
 		ret = -EPROTONOSUPPORT;
 		goto out_tunnel;
 	}
-- 
2.17.1

[PATCH net-next 3/6] l2tp: allow v2 netlink session create to pass ifname attribute

[Tom Parkin]

Previously L2TP_ATTR_IFNAME was only applicable to an L2TPv3 session,
since it was only used by l2tp_eth.

In order to implement an AC_PPP pseudowire in an L2TPv2 tunnel, it will
be necessary to allow userspace to send an interface name in the session
create message.

Allow the attribute to be handled by l2tp_netlink for L2TPv2 sessions.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_netlink.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 8ef1a579a2b1..2abfea82203d 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -604,10 +604,11 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
 			cfg.peer_cookie_len = len;
 			memcpy(&cfg.peer_cookie[0], nla_data(info->attrs[L2TP_ATTR_PEER_COOKIE]), len);
 		}
-		if (info->attrs[L2TP_ATTR_IFNAME])
-			cfg.ifname = nla_data(info->attrs[L2TP_ATTR_IFNAME]);
 	}
 
+	if (info->attrs[L2TP_ATTR_IFNAME])
+		cfg.ifname = nla_data(info->attrs[L2TP_ATTR_IFNAME]);
+
 	if (info->attrs[L2TP_ATTR_RECV_SEQ])
 		cfg.recv_seq = nla_get_u8(info->attrs[L2TP_ATTR_RECV_SEQ]);
 
-- 
2.17.1

[PATCH net-next 4/6] l2tp: add netlink attributes for ac_ppp session creation

[Tom Parkin]

The AC PPPoE driver will require userspace to pass in both the PPPoE
session ID and the PPPoE peer's MAC address.

Add netlink attributes to allow this to be conveyed in the session
create command.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 include/uapi/linux/l2tp.h | 2 ++
 net/l2tp/l2tp_netlink.c   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 30c80d5ba4bf..04e23b34669e 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -127,6 +127,8 @@ enum {
 	L2TP_ATTR_UDP_ZERO_CSUM6_TX,	/* flag */
 	L2TP_ATTR_UDP_ZERO_CSUM6_RX,	/* flag */
 	L2TP_ATTR_PAD,
+	L2TP_ATTR_PPPOE_SESSION_ID,	/* u16 */
+	L2TP_ATTR_PPPOE_PEER_MAC_ADDR,  /* 6 bytes */
 	__L2TP_ATTR_MAX,
 };
 
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 2abfea82203d..7050e8e5886e 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -920,6 +920,8 @@ static const struct nla_policy l2tp_nl_policy[L2TP_ATTR_MAX + 1] = {
 		.type = NLA_BINARY,
 		.len = 8,
 	},
+	[L2TP_ATTR_PPPOE_SESSION_ID]	= { .type = NLA_U16, },
+	[L2TP_ATTR_PPPOE_PEER_MAC_ADDR] = NLA_POLICY_ETH_ADDR,
 };
 
 static const struct genl_ops l2tp_nl_ops[] = {
-- 
2.17.1

[PATCH net-next 5/6] l2tp: add ac_pppoe pseudowire driver

[Tom Parkin]

The AC/PPPoE driver implements pseudowire type L2TP_PWTYPE_PPP_AC, for
use in a PPPoE Access Concentrator configuration.  Rather than
terminating the PPP session locally, the AC/PPPoE driver forwards PPP
packets over an L2TP tunnel for termination at the LNS.

l2tp_ac_pppoe provides a data path for PPPoE session packets, and
should be instantiated once a userspace process has completed the PPPoE
discovery process.

To create an instance of an L2TP_PWTYPE_PPP_AC pseudowire, userspace
must use the L2TP_CMD_SESSION_CREATE netlink command, and pass the
following attributes:

 * L2TP_ATTR_IFNAME, to specify the name of the interface associated
   with the PPPoE session;
 * L2TP_ATTR_PPPOE_SESSION_ID, to specify the PPPoE session ID assigned
   to the session;
 * L2TP_ATTR_PPPOE_PEER_MAC_ADDR, to specify the MAC address of the
   PPPoE peer

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/Kconfig         |   7 +
 net/l2tp/Makefile        |   1 +
 net/l2tp/l2tp_ac_pppoe.c | 446 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 454 insertions(+)
 create mode 100644 net/l2tp/l2tp_ac_pppoe.c

diff --git a/net/l2tp/Kconfig b/net/l2tp/Kconfig
index b7856748e960..f34d72070a6f 100644
--- a/net/l2tp/Kconfig
+++ b/net/l2tp/Kconfig
@@ -108,3 +108,10 @@ config L2TP_ETH
 
 	  To compile this driver as a module, choose M here. The module
 	  will be called l2tp_eth.
+
+config L2TP_AC_PPPOE
+	tristate "L2TP PPP Access Concentrator support"
+	depends on L2TP
+	help
+	  Support for tunneling PPP frames from PPPoE sessions in an L2TP
+	  session.
diff --git a/net/l2tp/Makefile b/net/l2tp/Makefile
index cf8f27071d3f..5bd66ae45eb6 100644
--- a/net/l2tp/Makefile
+++ b/net/l2tp/Makefile
@@ -16,3 +16,4 @@ obj-$(subst y,$(CONFIG_L2TP),$(CONFIG_L2TP_DEBUGFS)) += l2tp_debugfs.o
 ifneq ($(CONFIG_IPV6),)
 obj-$(subst y,$(CONFIG_L2TP),$(CONFIG_L2TP_IP)) += l2tp_ip6.o
 endif
+obj-$(subst y,$(CONFIG_L2TP),$(CONFIG_L2TP_AC_PPPOE)) += l2tp_ac_pppoe.o
diff --git a/net/l2tp/l2tp_ac_pppoe.c b/net/l2tp/l2tp_ac_pppoe.c
new file mode 100644
index 000000000000..59dce046c813
--- /dev/null
+++ b/net/l2tp/l2tp_ac_pppoe.c
@@ -0,0 +1,446 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* L2TP PPPoE access concentrator driver
+ *
+ * Copyright (c) 2020 Katalix Systems Ltd
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/if_pppox.h>
+#include <linux/l2tp.h>
+#include <linux/ppp_defs.h>
+#include <linux/etherdevice.h>
+
+#include <net/net_namespace.h>
+#include <net/netns/generic.h>
+
+#include "l2tp_core.h"
+
+#define L2TP_AC_PPPOE_SESSION_HASH_BITS 5
+#define L2TP_AC_PPPOE_SESSION_HASH_SIZE BIT(L2TP_AC_PPPOE_SESSION_HASH_BITS)
+
+/* Global hash list of PPPoE sessions.
+ * We hash on the PPPoE session ID, and scope session lookups to the
+ * associated netdev instance.
+ * Because the lookup is scoped to the netdev, it is practically
+ * scoped to the network namespace the netdev exists in.
+ */
+static struct hlist_head pppoe_session_hlist[L2TP_AC_PPPOE_SESSION_HASH_SIZE];
+static spinlock_t pppoe_session_hlist_lock;
+
+/* An AC PPPoE session instance */
+struct l2tp_ac_pppoe_session {
+	/* "Dead" flag used to prevent races between l2tp_core session delete
+	 * and session removal via. a netdev event.
+	 */
+	unsigned long			dead;
+	/* Device associated with the PPPoE session */
+	struct net_device __rcu		*dev;
+	/* PPPoE session ID */
+	u16				id;
+	/* Destination MAC address for PPPoE frames */
+	unsigned char			h_dest[ETH_ALEN];
+	/* L2TP session for this PPPoE session */
+	struct l2tp_session		*ls;
+	/* Entry on global hashlist */
+	struct hlist_node		hlist;
+};
+
+static struct hlist_head *l2tp_ac_pppoe_id_hash(u16 id)
+{
+	return &pppoe_session_hlist[hash_32(id, L2TP_AC_PPPOE_SESSION_HASH_BITS)];
+}
+
+/* Look up a PPPoE session instance by its ID.
+ * Must be called inside an rcu read lock.
+ */
+static struct l2tp_ac_pppoe_session *l2tp_ac_pppoe_find_by_id(struct net_device *dev, u16 id)
+{
+	struct l2tp_ac_pppoe_session *ps;
+	struct hlist_head *head;
+
+	head = l2tp_ac_pppoe_id_hash(id);
+
+	hlist_for_each_entry_rcu(ps, head, hlist)
+		if (ps->id == id)
+			if (rcu_dereference(ps->dev) == dev)
+				return ps;
+
+	return NULL;
+}
+
+static void l2tp_ac_pppoe_unhash_session(struct l2tp_ac_pppoe_session *ps)
+{
+	spin_lock_bh(&pppoe_session_hlist_lock);
+	hlist_del_init_rcu(&ps->hlist);
+	spin_unlock_bh(&pppoe_session_hlist_lock);
+	synchronize_rcu();
+}
+
+static void l2tp_ac_pppoe_kill_session(struct l2tp_ac_pppoe_session *ps)
+{
+	struct net_device *dev;
+
+	if (test_and_set_bit(0, &ps->dead))
+		return;
+
+	rcu_read_lock();
+	dev = rcu_dereference(ps->dev);
+	rcu_assign_pointer(ps->dev, NULL);
+	rcu_read_unlock();
+
+	/* This shouldn't occur, ref: l2tp_ac_pppoe_create_session
+	 * which holds a session reference around assigning the dev
+	 * pointer.
+	 */
+	if (WARN_ON(!dev))
+		return;
+
+	l2tp_ac_pppoe_unhash_session(ps);
+
+	/* Drop the references taken by the session */
+	dev_put(dev);
+	module_put(THIS_MODULE);
+}
+
+/* struct l2tp_session pseudowire close callback */
+static void l2tp_ac_pppoe_session_close(struct l2tp_session *ls)
+{
+	l2tp_ac_pppoe_kill_session(l2tp_session_priv(ls));
+}
+
+/* struct l2tp_session pseudowire recv callback */
+static void l2tp_ac_pppoe_recv_skb(struct l2tp_session *ls, struct sk_buff *skb, int l2tp_data_len)
+{
+	struct l2tp_ac_pppoe_session *ps = l2tp_session_priv(ls);
+	int data_len = skb->len;
+	struct net_device *dev;
+	struct pppoe_hdr *ph;
+
+	rcu_read_lock();
+
+	dev = rcu_dereference(ps->dev);
+	if (!dev)
+		goto drop;
+
+	if (skb_cow_head(skb, sizeof(*ph) + dev->hard_header_len))
+		goto drop;
+
+	/* If the user data has PPP Address and Control fields, strip them out.
+	 * This follows the approach of l2tp_ppp.c, which notes that although
+	 * use of these fields should in theory be negotiated and handled at
+	 * the PPP layer, the L2TP subsystem has always detected and removed
+	 * them.
+	 */
+	if (skb->data[0] == PPP_ALLSTATIONS && skb->data[1] == PPP_UI) {
+		if (pskb_may_pull(skb, 2)) {
+			skb_pull(skb, 2);
+			data_len -= 2;
+		}
+	}
+
+	/* Add PPPoE header */
+	__skb_push(skb, sizeof(*ph));
+	skb_reset_network_header(skb);
+
+	ph = pppoe_hdr(skb);
+	ph->ver = 0x1;
+	ph->type = 0x1;
+	ph->code = 0;
+	ph->sid = htons(ps->id);
+	ph->length = htons(data_len);
+
+	/* SKB settings */
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_PPP_SES);
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	/* Add Ethernet header */
+	dev_hard_header(skb, dev, ETH_P_PPP_SES, ps->h_dest, NULL, data_len);
+
+	rcu_read_unlock();
+
+	dev_queue_xmit(skb);
+
+	return;
+
+drop:
+	rcu_read_unlock();
+	kfree_skb(skb);
+}
+
+/* struct l2tp_session pseudowire show callback */
+static void l2tp_ac_pppoe_show(struct seq_file *m, void *arg)
+{
+	struct l2tp_ac_pppoe_session *ps = l2tp_session_priv(arg);
+	struct net_device *dev;
+
+	rcu_read_lock();
+	dev = rcu_dereference(ps->dev);
+	if (!dev) {
+		rcu_read_unlock();
+		return;
+	}
+	rcu_read_unlock();
+
+	seq_printf(m, "   interface %s\n", dev->name);
+	seq_printf(m, "   PPPoE session %d\n", ps->id);
+	seq_printf(m, "   client hwaddr %02X:%02X:%02X:%02X:%02X:%02X\n",
+		   ps->h_dest[0], ps->h_dest[1], ps->h_dest[2],
+		   ps->h_dest[3], ps->h_dest[4], ps->h_dest[5]);
+}
+
+static int l2tp_ac_pppoe_create_session(struct net_device *dev, u16 id,
+					unsigned char *peer_mac,
+					struct l2tp_tunnel *tunnel, u32 sid,
+					u32 psid, struct l2tp_session_cfg *cfg,
+					struct l2tp_ac_pppoe_session **out)
+{
+	struct l2tp_ac_pppoe_session *ps;
+	struct l2tp_session *ls;
+	int ret = 0;
+
+	ls = l2tp_session_create(sizeof(*ps), tunnel, sid, psid, cfg);
+	if (IS_ERR(ls)) {
+		ret = PTR_ERR(ls);
+		goto out;
+	}
+
+	ps = l2tp_session_priv(ls);
+	memcpy(ps->h_dest, peer_mac, ETH_ALEN);
+	ps->id = id;
+	ps->ls = ls;
+	INIT_HLIST_NODE(&ps->hlist);
+
+	ls->session_close = l2tp_ac_pppoe_session_close;
+	ls->recv_skb = l2tp_ac_pppoe_recv_skb;
+	if (IS_ENABLED(CONFIG_L2TP_DEBUGFS))
+		ls->show = l2tp_ac_pppoe_show;
+
+	/* Hold session refcount to ensure it can't go away until we have
+	 * assigned the dev pointer in struct l2tp_ac_pppoe_session and
+	 * taken a reference on the device.
+	 */
+	l2tp_session_inc_refcount(ls);
+
+	ret = l2tp_session_register(ls, tunnel);
+	if (ret < 0) {
+		l2tp_session_dec_refcount(ls);
+		goto out;
+	}
+
+	rcu_assign_pointer(ps->dev, dev);
+	dev_hold(ps->dev);
+
+	l2tp_session_dec_refcount(ls);
+
+	__module_get(THIS_MODULE);
+
+	*out = ps;
+
+out:
+	return ret;
+}
+
+/* Pass PPPoE packet into the associated L2TP session */
+static int l2tp_ac_pppoe_l2tp_xmit(struct net_device *dev, struct sk_buff *skb)
+{
+	struct l2tp_ac_pppoe_session *ps;
+	struct pppoe_hdr *ph;
+	int ret;
+
+	if (!pskb_may_pull(skb, sizeof(*ph)))
+		goto drop;
+
+	ph = pppoe_hdr(skb);
+
+	skb_pull(skb, sizeof(*ph));
+
+	rcu_read_lock();
+
+	ps = l2tp_ac_pppoe_find_by_id(dev, ntohs(ph->sid));
+	if (!ps)
+		goto unlock_drop;
+
+	ret = l2tp_xmit_skb(ps->ls, skb);
+	rcu_read_unlock();
+	return ret;
+
+unlock_drop:
+	rcu_read_unlock();
+drop:
+	kfree_skb(skb);
+	return NET_RX_DROP;
+}
+
+/* PPPoE session packet rx handler */
+static int l2tp_ac_pppoe_recv_pppoe(struct sk_buff *skb, struct net_device *dev,
+				    struct packet_type *pt,
+				    struct net_device *orig_dev)
+{
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb)
+		return NET_RX_DROP;
+	return l2tp_ac_pppoe_l2tp_xmit(dev, skb);
+}
+
+/* L2TP/netlink pseudowire create callback */
+static int l2tp_ac_pppoe_nl_create(struct net *net, struct l2tp_tunnel *tunnel,
+				   u32 sid, u32 psid,
+				   struct l2tp_session_cfg *cfg,
+				   struct genl_info *info)
+{
+	unsigned char peer_mac[ETH_ALEN];
+	struct l2tp_ac_pppoe_session *ps;
+	struct net_device *dev = NULL;
+	u16 pppoe_id;
+	int ret;
+
+	/* We must have PPPoE session ID, the PPPoE peer's MAC address.
+	 * and the name of the interface.  The latter has already been
+	 * unpacked into the session config structure by l2tp_netlink.c.
+	 */
+	if (!info->attrs[L2TP_ATTR_PPPOE_SESSION_ID]) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	pppoe_id = nla_get_u16(info->attrs[L2TP_ATTR_PPPOE_SESSION_ID]);
+	if (!pppoe_id) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!info->attrs[L2TP_ATTR_PPPOE_PEER_MAC_ADDR]) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* l2tp_netlink policy for mandates that PEER_MAC_ADDR must be of ETH_ALEN bytes */
+	memcpy(peer_mac, nla_data(info->attrs[L2TP_ATTR_PPPOE_PEER_MAC_ADDR]), ETH_ALEN);
+	if (!is_valid_ether_addr(peer_mac)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!cfg->ifname) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Look up the netdev of the specified name */
+	dev = dev_get_by_name(net, cfg->ifname);
+	if (!dev) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* Prevent ID clashes.
+	 * Note the genl lock prevents any race due to the gap between checking
+	 * for a clash and adding this session to the hash list, since:
+	 *  - ac_pppoe sessions can only be created by netlink command, and
+	 *  - l2tp_netlink doesn't enable parallel genl ops.
+	 */
+	rcu_read_lock();
+	if (l2tp_ac_pppoe_find_by_id(dev, pppoe_id)) {
+		ret = -EALREADY;
+		rcu_read_unlock();
+		goto out;
+	}
+	rcu_read_unlock();
+
+	ret = l2tp_ac_pppoe_create_session(dev, pppoe_id, peer_mac, tunnel, sid, psid, cfg, &ps);
+	if (ret != 0)
+		goto out;
+
+	/* Add session to global hash */
+	spin_lock_bh(&pppoe_session_hlist_lock);
+	hlist_add_head_rcu(&ps->hlist, l2tp_ac_pppoe_id_hash(pppoe_id));
+	spin_unlock_bh(&pppoe_session_hlist_lock);
+
+out:
+	/* Drop dev reference if we have it: the session takes its own reference */
+	if (dev)
+		dev_put(dev);
+	return ret;
+}
+
+static int l2tp_ac_pppoe_netdevice_event(struct notifier_block *unused,
+					 unsigned long event, void *ptr)
+{
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+	struct l2tp_ac_pppoe_session *ps;
+	int hash;
+
+	if (event == NETDEV_UNREGISTER) {
+		rcu_read_lock();
+		for (hash = 0; hash < L2TP_AC_PPPOE_SESSION_HASH_SIZE; hash++)
+			hlist_for_each_entry_rcu(ps, &pppoe_session_hlist[hash], hlist)
+				if (ps->dev == dev)
+					l2tp_ac_pppoe_kill_session(ps);
+		rcu_read_unlock();
+		return NOTIFY_OK;
+	}
+	return NOTIFY_DONE;
+}
+
+/******************************************************************************
+ * Init and cleanup
+ */
+
+static const struct l2tp_nl_cmd_ops l2tp_ac_pppoe_nl_cmd_ops = {
+	.session_create	= l2tp_ac_pppoe_nl_create,
+	/* Our cleanup is handled via. the session_close callback, called by l2tp_session_delete */
+	.session_delete	= l2tp_session_delete,
+};
+
+static struct packet_type pppoes_ptype = {
+	.type	= htons(ETH_P_PPP_SES),
+	.func	= l2tp_ac_pppoe_recv_pppoe,
+};
+
+static struct notifier_block l2tp_ac_pppoe_notifier_block = {
+	.notifier_call = l2tp_ac_pppoe_netdevice_event,
+};
+
+static int __init l2tp_ac_pppoe_init(void)
+{
+	int err, hash;
+
+	spin_lock_init(&pppoe_session_hlist_lock);
+	for (hash = 0; hash < L2TP_AC_PPPOE_SESSION_HASH_SIZE; hash++)
+		INIT_HLIST_HEAD(&pppoe_session_hlist[hash]);
+
+	err = l2tp_nl_register_ops(L2TP_PWTYPE_PPP_AC, &l2tp_ac_pppoe_nl_cmd_ops);
+	if (err)
+		return err;
+
+	err = register_netdevice_notifier(&l2tp_ac_pppoe_notifier_block);
+	if (err) {
+		l2tp_nl_unregister_ops(L2TP_PWTYPE_PPP_AC);
+		return err;
+	}
+
+	dev_add_pack(&pppoes_ptype);
+
+	pr_info("L2TP AC PPPoE support\n");
+
+	return err;
+}
+
+static void __exit l2tp_ac_pppoe_exit(void)
+{
+	l2tp_nl_unregister_ops(L2TP_PWTYPE_PPP_AC);
+	unregister_netdevice_notifier(&l2tp_ac_pppoe_notifier_block);
+	dev_remove_pack(&pppoes_ptype);
+}
+
+module_init(l2tp_ac_pppoe_init);
+module_exit(l2tp_ac_pppoe_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Tom Parkin <tparkin@katalix.com>");
+MODULE_DESCRIPTION("L2TP AC PPPoE driver");
+MODULE_VERSION("1.0");
+MODULE_ALIAS_L2TP_PWTYPE(8);
-- 
2.17.1

[PATCH net-next 6/6] docs: networking: update l2tp.rst to document PPP_AC pseudowires

[Tom Parkin]

Alongside this, add a bit more of a description of the PPP and ETH
pseudowire types.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 Documentation/networking/l2tp.rst | 69 ++++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/Documentation/networking/l2tp.rst b/Documentation/networking/l2tp.rst
index 498b382d25a0..76df531882e6 100644
--- a/Documentation/networking/l2tp.rst
+++ b/Documentation/networking/l2tp.rst
@@ -119,6 +119,26 @@ l2tp" commands). If ``L2TP_ATTR_FD`` is given, it must be a socket fd
 that is already bound and connected. There is more information about
 unmanaged tunnels later in this document.
 
+L2TP sessions, or pseudowires, may be of several different types. The
+``L2TP_CMD_SESSION_CREATE`` command specifies the pseudowire type to
+be created.
+
+================== ==================================================
+Pseudowire type    Description
+================== ==================================================
+ETH                L2TPv3 only.  Pseudowire carries Ethernet frames.
+                   The session instance is associated with a virtual
+                   network device ``l2tpethN``.
+PPP                Pseudowire carries PPP frames.  The session instance
+                   is associated with a virtual network device
+                   ``pppN`` which terminates the PPP session locally.
+PPP_AC             Pseudowire carries PPP frames which are switched
+                   from incoming PPPoE frames into the L2TP tunnel,
+                   allowing the PPP session to be terminated at the
+                   other end of the tunnel.  There is no virtual
+                   network device associated with the session instance.
+================== ==================================================
+
 ``L2TP_CMD_TUNNEL_CREATE`` attributes:-
 
 ================== ======== ===
@@ -177,26 +197,35 @@ CONN_ID            N        Identifies the tunnel id to be queried.
 
 ``L2TP_CMD_SESSION_CREATE`` attributes:-
 
-================== ======== ===
-Attribute          Required Use
-================== ======== ===
-CONN_ID            Y        The parent tunnel id.
-SESSION_ID         Y        Sets the session id.
-PEER_SESSION_ID    Y        Sets the parent session id.
-PW_TYPE            Y        Sets the pseudowire type.
-DEBUG              N        Debug flags.
-RECV_SEQ           N        Enable rx data sequence numbers.
-SEND_SEQ           N        Enable tx data sequence numbers.
-LNS_MODE           N        Enable LNS mode (auto-enable data sequence
-                            numbers).
-RECV_TIMEOUT       N        Timeout to wait when reordering received
-                            packets.
-L2SPEC_TYPE        N        Sets layer2-specific-sublayer type (L2TPv3
-                            only).
-COOKIE             N        Sets optional cookie (L2TPv3 only).
-PEER_COOKIE        N        Sets optional peer cookie (L2TPv3 only).
-IFNAME             N        Sets interface name (L2TPv3 only).
-================== ======== ===
+==================== ======== ===
+Attribute            Required Use
+==================== ======== ===
+CONN_ID              Y        The parent tunnel id.
+SESSION_ID           Y        Sets the session id.
+PEER_SESSION_ID      Y        Sets the parent session id.
+PW_TYPE              Y        Sets the pseudowire type.
+DEBUG                N        Debug flags.
+RECV_SEQ             N        Enable rx data sequence numbers.
+SEND_SEQ             N        Enable tx data sequence numbers.
+LNS_MODE             N        Enable LNS mode (auto-enable data sequence
+                              numbers).
+RECV_TIMEOUT         N        Timeout to wait when reordering received
+                              packets.
+L2SPEC_TYPE          N        Sets layer2-specific-sublayer type (L2TPv3
+                              only).
+COOKIE               N        Sets optional cookie (L2TPv3 only).
+PEER_COOKIE          N        Sets optional peer cookie (L2TPv3 only).
+IFNAME               N        Pseudowire-specific parameter:
+
+                              - For Ethernet, IFNAME sets the name to be assigned
+                                to the L2TP session network interface.
+                              - For AC/PPPoE, IFNAME specifies the name of the
+                                interface associated with the PPPoE session.
+                              - For PPP pseudowires, IFNAME is ignored.
+
+PPPOE_SESSION_ID     N        Sets the PPPoE session ID (L2TPv2 AC/PPPoE only).
+PPPOE_PEER_MAC_ADDR  N        Sets the PPPoE peer MAC address (L2TPv2 AC/PPPoE only).
+==================== ======== ===
 
 For Ethernet session types, this will create an l2tpeth virtual
 interface which can then be configured as required. For PPP session
-- 
2.17.1

Re: [PATCH net-next 0/6] l2tp: add ac/pppoe driver

[James Chapman]

On 30/09/2020 22:07, Tom Parkin wrote:
> L2TPv2 tunnels are often used as a part of a home broadband connection,
> using a PPP link to connect the subscriber network into the Internet
> Service Provider's network.
>
> In this scenario, PPPoE is widely used between the L2TP Access
> Concentrator (LAC) and the subscriber.  The LAC effectively acts as a
> PPPoE server, switching PPP frames from incoming PPPoE packets into an
> L2TP session.  The PPP session is then terminated at the L2TP Network
> Server (LNS) on the edge of the ISP's IP network.
>
> This patchset adds a driver to the L2TP subsystem to support this mode
> of operation.
>
> The new driver, l2tp_ac_pppoe, adds support for the existing pseudowire
> type L2TP_PWTYPE_PPP_AC, and is instantiated using the existing L2TP
> netlink L2TP_CMD_SESSION_CREATE.  It is expected to be used as follows:
>
>  * A userspace PPPoE daemon running on the LAC handles the PPPoE
>    discovery process up to the point of assigning a PPPoE session ID and
>    sending the PADS packet to the PPPoE peer to establish the PPPoE
>    session.
>  * Userspace code running on the LAC then instantiates an L2TP tunnel
>    and session with the LNS using the L2TP control protocol.
>  * Finally, the data path for PPPoE session frames through the L2TP
>    session to the LAC is instantiated by sending a genetlink
>    L2TP_CMD_SESSION_CREATE command to the kernel, including
>    the PPPoE-specific metadata required for L2TP_PWTYPE_PPP_AC sessions
>    (this is documented in the patch series commit comments).
>
> Supporting this driver submission we have two examples of userspace
> projects which use L2TP_PWTYPE_PPP_AC:
>
>  * https://github.com/katalix/l2tp-ktest
>
>    This is a unit-test suite for the kernel L2TP subsystem which has
>    been updated to include basic lifetime and datapath tests for
>    l2tp_ac_pppoe.
>
>    The new tests are automatically enabled when l2tp_ac_pppoe
>    availability is detected, and hence support for l2tp_ac_pppoe is on
>    the master branch of the git repository.
>
>  * https://github.com/katalix/go-l2tp
>
>    This is a Go library for building L2TP applications on Linux, and
>    includes a suite of example daemons which utilise the library.
>
>    The daemon kpppoed implements the PPPoE discovery protocol, and spawns
>    an instance of a daemon kl2tpd which handles the L2TP control protocol
>    and instantiates the kernel data path.
>
>    The code utilising l2tp_ac_pppoe is on the branch tp_002_pppoe_1
>    pending merge of this patchset in the kernel.
>
> Notes on the patchset itself:
>
>  * Patches 1-4 lay groundwork for the addition of the new driver, making
>    tweaks to the l2tp netlink code to allow l2tp_ac_pppoe to access the
>    netlink attributes it requires.
>  * Patch 5 adds the new driver itself and hooks it into the kernel
>    configuration and build system.
>  * Patch 6 updates the l2tp documentation under Documentation/ to
>    include information about the new driver.
>
> Tom Parkin (6):
>   l2tp: add netlink info to session create callback
>   l2tp: tweak netlink session create to allow L2TPv2 ac_pppoe
>   l2tp: allow v2 netlink session create to pass ifname attribute
>   l2tp: add netlink attributes for ac_ppp session creation
>   l2tp: add ac_pppoe pseudowire driver
>   docs: networking: update l2tp.rst to document PPP_AC pseudowires
>
>  Documentation/networking/l2tp.rst |  69 +++--
>  include/uapi/linux/l2tp.h         |   2 +
>  net/l2tp/Kconfig                  |   7 +
>  net/l2tp/Makefile                 |   1 +
>  net/l2tp/l2tp_ac_pppoe.c          | 446 ++++++++++++++++++++++++++++++
>  net/l2tp/l2tp_core.h              |   4 +-
>  net/l2tp/l2tp_eth.c               |   3 +-
>  net/l2tp/l2tp_netlink.c           |  20 +-
>  net/l2tp/l2tp_ppp.c               |   3 +-
>  9 files changed, 527 insertions(+), 28 deletions(-)
>  create mode 100644 net/l2tp/l2tp_ac_pppoe.c
>
Reviewed-by: James Chapman <jchapman@katalix.com>

Re: [PATCH net-next 0/6] l2tp: add ac/pppoe driver

[Guillaume Nault]

On Wed, Sep 30, 2020 at 10:07:01PM +0100, Tom Parkin wrote:
> L2TPv2 tunnels are often used as a part of a home broadband connection,
> using a PPP link to connect the subscriber network into the Internet
> Service Provider's network.
> 
> In this scenario, PPPoE is widely used between the L2TP Access
> Concentrator (LAC) and the subscriber.  The LAC effectively acts as a
> PPPoE server, switching PPP frames from incoming PPPoE packets into an
> L2TP session.  The PPP session is then terminated at the L2TP Network
> Server (LNS) on the edge of the ISP's IP network.
> 
> This patchset adds a driver to the L2TP subsystem to support this mode
> of operation.

Hi Tom,

Nice to see someone working on this use case. However, have you
considered other implementation approaches?

This new module reimplements PPPoE in net/l2tp (ouch!), so we'd now
have two PPPoE implementations with two different packet handlers for
ETH_P_PPP_SES. Also this implementation doesn't take into account other
related use cases, like forwarding PPP frames between two L2TP sessions
(not even talking about PPTP).

A much simpler and more general approach would be to define a new PPP
ioctl, to "bridge" two PPP channels together. I discussed this with
DaveM at netdevconf 2.2 (Seoul, 2017) and we agreed that it was
probably the best way forward.

It's just a matter of extending struct channel (in ppp_generic.c) with
a pointer to another channel, then testing this pointer in ppp_input().
If the pointer is NULL, use the classical path, if not, forward the PPP
frame using the ->start_xmit function of the peer channel. There are a
few details to take into account of course (crossing netns, locking),
but nothing big (I could implement it the following night in my hotel
room before leaving Seoul). This approach should work for forwarding
PPP frames between any type of PPP transport.

I unfortunately didn't propose the code upstream at that time, because
I didn't want to add this kernel feature without having a userspace
implementation making use of it and ready to release (and I finally
left the company before that happened). But I know that this
implementation worked fine as it did receive quite a lot of testing.

Yet another way to implement this feature would to define virtual PPPoE
and L2TP devices, working in external mode. In practice, one PPPoE and
one L2TP network device would be enough for handling all the traffic.
Then TC could be used to pass the PPP frames between PPPoE and L2TP.

Example (assuming flower and tunnel_key were extented to support PPPoE
and L2TP):

# Forward PPPoE frames with Session-ID 5 to L2TP tunnel 1 session 1
$ tc filter add dev pppoe0 ingress flower pppoe_sid 5   \
    action tunnel_key src_ip 192.0.2.1 dst_ip 192.0.2.2 \
                      l2tp_tid 1 l2tp_peertid 1         \
                      l2tp_sid 1 l2tp_peer_sid 1        \
    action mirred egress redirect dev l2tp0

# Reverse path
$ tc filter add dev l2tp0 ingress flower l2tp_tid 1 l2tp_sid 1            \
    action tunnel_key dst_mac 02:00:00:00:00:01 src_mac 02:00:00:00:00:02 \
                      id 5                                                \
    action mirred egress redirect dev pppoe0

Of course the commands would be a bit longer in practice (one would
probably want to match on the src and dst IP addresses in the reverse
path, or set the L2TP version, etc.), but that's the general idea.

Such approach would probably not allow the use of L2TP sequence numbers
though (which might not be a bad thing in the end). It'd also require
more work, but would avoid going through the PPP layer and might even
be offloadable (if a NIC vendor ever wants to support it).

Regards,

Guillaume

Re: [PATCH net-next 5/6] l2tp: add ac_pppoe pseudowire driver

[Jakub Kicinski]

On Wed, 30 Sep 2020 22:07:06 +0100 Tom Parkin wrote:
> The AC/PPPoE driver implements pseudowire type L2TP_PWTYPE_PPP_AC, for
> use in a PPPoE Access Concentrator configuration.  Rather than
> terminating the PPP session locally, the AC/PPPoE driver forwards PPP
> packets over an L2TP tunnel for termination at the LNS.
> 
> l2tp_ac_pppoe provides a data path for PPPoE session packets, and
> should be instantiated once a userspace process has completed the PPPoE
> discovery process.
> 
> To create an instance of an L2TP_PWTYPE_PPP_AC pseudowire, userspace
> must use the L2TP_CMD_SESSION_CREATE netlink command, and pass the
> following attributes:
> 
>  * L2TP_ATTR_IFNAME, to specify the name of the interface associated
>    with the PPPoE session;
>  * L2TP_ATTR_PPPOE_SESSION_ID, to specify the PPPoE session ID assigned
>    to the session;
>  * L2TP_ATTR_PPPOE_PEER_MAC_ADDR, to specify the MAC address of the
>    PPPoE peer

C=1 generates:

net/l2tp/l2tp_ac_pppoe.c:234:20: warning: incorrect type in argument 1 (different address spaces)
net/l2tp/l2tp_ac_pppoe.c:234:20:    expected struct net_device *dev
net/l2tp/l2tp_ac_pppoe.c:234:20:    got struct net_device [noderef] __rcu *dev
net/l2tp/l2tp_ac_pppoe.c:380:45: error: incompatible types in comparison expression (different address spaces):
net/l2tp/l2tp_ac_pppoe.c:380:45:    struct net_device [noderef] __rcu *
net/l2tp/l2tp_ac_pppoe.c:380:45:    struct net_device *

Re: [PATCH net-next 0/6] l2tp: add ac/pppoe driver

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

On  Thu, Oct 01, 2020 at 14:26:17 +0200, Guillaume Nault wrote:
> On Wed, Sep 30, 2020 at 10:07:01PM +0100, Tom Parkin wrote:
> > L2TPv2 tunnels are often used as a part of a home broadband connection,
> > using a PPP link to connect the subscriber network into the Internet
> > Service Provider's network.
> > 
> > In this scenario, PPPoE is widely used between the L2TP Access
> > Concentrator (LAC) and the subscriber.  The LAC effectively acts as a
> > PPPoE server, switching PPP frames from incoming PPPoE packets into an
> > L2TP session.  The PPP session is then terminated at the L2TP Network
> > Server (LNS) on the edge of the ISP's IP network.
> > 
> > This patchset adds a driver to the L2TP subsystem to support this mode
> > of operation.
> 
> Hi Tom,
> 
> Nice to see someone working on this use case. However, have you
> considered other implementation approaches?
> 
> This new module reimplements PPPoE in net/l2tp (ouch!), so we'd now
> have two PPPoE implementations with two different packet handlers for
> ETH_P_PPP_SES. Also this implementation doesn't take into account other
> related use cases, like forwarding PPP frames between two L2TP sessions
> (not even talking about PPTP).
> 
> A much simpler and more general approach would be to define a new PPP
> ioctl, to "bridge" two PPP channels together. I discussed this with
> DaveM at netdevconf 2.2 (Seoul, 2017) and we agreed that it was
> probably the best way forward.

Hi Guillaume,

Thank you for reviewing the patchset.

I hadn't considered supporting this usecase in the ppp subsystem
directly, so thank you for that suggestion.  I can definitely see the
appeal of avoiding reimplementing the PPPoE session packet handling.
Having looked at the ppp code, I think it'd be a smaller change
overall than this series, so that's also appealing.

I'll wait on a little to let any other review comments come in, but
if doing as you suggest is still the preferred approach I'll happily
look at implementing it -- assuming you don't have a patch ready to go?

Best regards,
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 5/6] l2tp: add ac_pppoe pseudowire driver

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On  Thu, Oct 01, 2020 at 07:56:40 -0700, Jakub Kicinski wrote:
> On Wed, 30 Sep 2020 22:07:06 +0100 Tom Parkin wrote:
> > The AC/PPPoE driver implements pseudowire type L2TP_PWTYPE_PPP_AC, for
> > use in a PPPoE Access Concentrator configuration.  Rather than
> > terminating the PPP session locally, the AC/PPPoE driver forwards PPP
> > packets over an L2TP tunnel for termination at the LNS.
> > 
> > l2tp_ac_pppoe provides a data path for PPPoE session packets, and
> > should be instantiated once a userspace process has completed the PPPoE
> > discovery process.
> > 
> > To create an instance of an L2TP_PWTYPE_PPP_AC pseudowire, userspace
> > must use the L2TP_CMD_SESSION_CREATE netlink command, and pass the
> > following attributes:
> > 
> >  * L2TP_ATTR_IFNAME, to specify the name of the interface associated
> >    with the PPPoE session;
> >  * L2TP_ATTR_PPPOE_SESSION_ID, to specify the PPPoE session ID assigned
> >    to the session;
> >  * L2TP_ATTR_PPPOE_PEER_MAC_ADDR, to specify the MAC address of the
> >    PPPoE peer
> 
> C=1 generates:
> 
> net/l2tp/l2tp_ac_pppoe.c:234:20: warning: incorrect type in argument 1 (different address spaces)
> net/l2tp/l2tp_ac_pppoe.c:234:20:    expected struct net_device *dev
> net/l2tp/l2tp_ac_pppoe.c:234:20:    got struct net_device [noderef] __rcu *dev
> net/l2tp/l2tp_ac_pppoe.c:380:45: error: incompatible types in comparison expression (different address spaces):
> net/l2tp/l2tp_ac_pppoe.c:380:45:    struct net_device [noderef] __rcu *
> net/l2tp/l2tp_ac_pppoe.c:380:45:    struct net_device *

Thanks Jakub, and apologies for that slipping through.  My Sparse
installation on Ubuntu wasn't working -- I've updated it now and can
see the error you reported.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 0/6] l2tp: add ac/pppoe driver

[Guillaume Nault]

On Thu, Oct 01, 2020 at 03:57:29PM +0100, Tom Parkin wrote:
> I'll wait on a little to let any other review comments come in, but
> if doing as you suggest is still the preferred approach I'll happily
> look at implementing it -- assuming you don't have a patch ready to go?

I unfortunately don't. All I saved from my previous ongoing L2TP and
PPP work is a (long) TODO list.