[PATCH net] l2tp: fix l2tp_eth module loading

[PATCH net] l2tp: fix l2tp_eth module loading

[Guillaume Nault]

The l2tp_eth module crashes if its netlink callbacks are run when the
pernet data aren't initialised.

We should normally register_pernet_device() before the genl callbacks.
However, the pernet data only maintain a list of l2tpeth interfaces,
and this list is never used. So let's just drop pernet handling
instead.

Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
 net/l2tp/l2tp_eth.c | 51 ++-------------------------------------------------
 1 file changed, 2 insertions(+), 49 deletions(-)

diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 87da9ef61860..014a7bc2a872 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -44,7 +44,6 @@ struct l2tp_eth {
 	struct net_device	*dev;
 	struct sock		*tunnel_sock;
 	struct l2tp_session	*session;
-	struct list_head	list;
 	atomic_long_t		tx_bytes;
 	atomic_long_t		tx_packets;
 	atomic_long_t		tx_dropped;
@@ -58,17 +57,6 @@ struct l2tp_eth_sess {
 	struct net_device	*dev;
 };
 
-/* per-net private data for this module */
-static unsigned int l2tp_eth_net_id;
-struct l2tp_eth_net {
-	struct list_head l2tp_eth_dev_list;
-	spinlock_t l2tp_eth_lock;
-};
-
-static inline struct l2tp_eth_net *l2tp_eth_pernet(struct net *net)
-{
-	return net_generic(net, l2tp_eth_net_id);
-}
 
 static int l2tp_eth_dev_init(struct net_device *dev)
 {
@@ -84,12 +72,6 @@ static int l2tp_eth_dev_init(struct net_device *dev)
 
 static void l2tp_eth_dev_uninit(struct net_device *dev)
 {
-	struct l2tp_eth *priv = netdev_priv(dev);
-	struct l2tp_eth_net *pn = l2tp_eth_pernet(dev_net(dev));
-
-	spin_lock(&pn->l2tp_eth_lock);
-	list_del_init(&priv->list);
-	spin_unlock(&pn->l2tp_eth_lock);
 	dev_put(dev);
 }
 
@@ -273,7 +255,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
 	struct l2tp_eth *priv;
 	struct l2tp_eth_sess *spriv;
 	int rc;
-	struct l2tp_eth_net *pn;
 
 	if (cfg->ifname) {
 		strlcpy(name, cfg->ifname, IFNAMSIZ);
@@ -305,7 +286,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
 	priv = netdev_priv(dev);
 	priv->dev = dev;
 	priv->session = session;
-	INIT_LIST_HEAD(&priv->list);
 
 	priv->tunnel_sock = tunnel->sock;
 	session->recv_skb = l2tp_eth_dev_recv;
@@ -326,10 +306,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
 	strlcpy(session->ifname, dev->name, IFNAMSIZ);
 
 	dev_hold(dev);
-	pn = l2tp_eth_pernet(dev_net(dev));
-	spin_lock(&pn->l2tp_eth_lock);
-	list_add(&priv->list, &pn->l2tp_eth_dev_list);
-	spin_unlock(&pn->l2tp_eth_lock);
 
 	return 0;
 
@@ -342,22 +318,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
 	return rc;
 }
 
-static __net_init int l2tp_eth_init_net(struct net *net)
-{
-	struct l2tp_eth_net *pn = net_generic(net, l2tp_eth_net_id);
-
-	INIT_LIST_HEAD(&pn->l2tp_eth_dev_list);
-	spin_lock_init(&pn->l2tp_eth_lock);
-
-	return 0;
-}
-
-static struct pernet_operations l2tp_eth_net_ops = {
-	.init = l2tp_eth_init_net,
-	.id   = &l2tp_eth_net_id,
-	.size = sizeof(struct l2tp_eth_net),
-};
-
 
 static const struct l2tp_nl_cmd_ops l2tp_eth_nl_cmd_ops = {
 	.session_create	= l2tp_eth_create,
@@ -371,25 +331,18 @@ static int __init l2tp_eth_init(void)
 
 	err = l2tp_nl_register_ops(L2TP_PWTYPE_ETH, &l2tp_eth_nl_cmd_ops);
 	if (err)
-		goto out;
-
-	err = register_pernet_device(&l2tp_eth_net_ops);
-	if (err)
-		goto out_unreg;
+		goto err;
 
 	pr_info("L2TP ethernet pseudowire support (L2TPv3)\n");
 
 	return 0;
 
-out_unreg:
-	l2tp_nl_unregister_ops(L2TP_PWTYPE_ETH);
-out:
+err:
 	return err;
 }
 
 static void __exit l2tp_eth_exit(void)
 {
-	unregister_pernet_device(&l2tp_eth_net_ops);
 	l2tp_nl_unregister_ops(L2TP_PWTYPE_ETH);
 }
 
-- 
2.14.2

Re: [PATCH net] l2tp: fix l2tp_eth module loading

[Tom Parkin]

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

On Thu, Sep 28, 2017 at 03:44:38PM +0200, Guillaume Nault wrote:
> The l2tp_eth module crashes if its netlink callbacks are run when the
> pernet data aren't initialised.
> 
> We should normally register_pernet_device() before the genl callbacks.
> However, the pernet data only maintain a list of l2tpeth interfaces,
> and this list is never used. So let's just drop pernet handling
> instead.
> 
> Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

Whoops.  I think this was intended to clear up the devices in the net
namespace, but since l2tp_core.c already deletes tunnels on namespace
exit I don't think it's necessary for l2tp_eth.c to do anything more.

> ---
>  net/l2tp/l2tp_eth.c | 51 ++-------------------------------------------------
>  1 file changed, 2 insertions(+), 49 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
> index 87da9ef61860..014a7bc2a872 100644
> --- a/net/l2tp/l2tp_eth.c
> +++ b/net/l2tp/l2tp_eth.c
> @@ -44,7 +44,6 @@ struct l2tp_eth {
>  	struct net_device	*dev;
>  	struct sock		*tunnel_sock;
>  	struct l2tp_session	*session;
> -	struct list_head	list;
>  	atomic_long_t		tx_bytes;
>  	atomic_long_t		tx_packets;
>  	atomic_long_t		tx_dropped;
> @@ -58,17 +57,6 @@ struct l2tp_eth_sess {
>  	struct net_device	*dev;
>  };
>  
> -/* per-net private data for this module */
> -static unsigned int l2tp_eth_net_id;
> -struct l2tp_eth_net {
> -	struct list_head l2tp_eth_dev_list;
> -	spinlock_t l2tp_eth_lock;
> -};
> -
> -static inline struct l2tp_eth_net *l2tp_eth_pernet(struct net *net)
> -{
> -	return net_generic(net, l2tp_eth_net_id);
> -}
>  
>  static int l2tp_eth_dev_init(struct net_device *dev)
>  {
> @@ -84,12 +72,6 @@ static int l2tp_eth_dev_init(struct net_device *dev)
>  
>  static void l2tp_eth_dev_uninit(struct net_device *dev)
>  {
> -	struct l2tp_eth *priv = netdev_priv(dev);
> -	struct l2tp_eth_net *pn = l2tp_eth_pernet(dev_net(dev));
> -
> -	spin_lock(&pn->l2tp_eth_lock);
> -	list_del_init(&priv->list);
> -	spin_unlock(&pn->l2tp_eth_lock);
>  	dev_put(dev);
>  }
>  
> @@ -273,7 +255,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
>  	struct l2tp_eth *priv;
>  	struct l2tp_eth_sess *spriv;
>  	int rc;
> -	struct l2tp_eth_net *pn;
>  
>  	if (cfg->ifname) {
>  		strlcpy(name, cfg->ifname, IFNAMSIZ);
> @@ -305,7 +286,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
>  	priv = netdev_priv(dev);
>  	priv->dev = dev;
>  	priv->session = session;
> -	INIT_LIST_HEAD(&priv->list);
>  
>  	priv->tunnel_sock = tunnel->sock;
>  	session->recv_skb = l2tp_eth_dev_recv;
> @@ -326,10 +306,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
>  	strlcpy(session->ifname, dev->name, IFNAMSIZ);
>  
>  	dev_hold(dev);
> -	pn = l2tp_eth_pernet(dev_net(dev));
> -	spin_lock(&pn->l2tp_eth_lock);
> -	list_add(&priv->list, &pn->l2tp_eth_dev_list);
> -	spin_unlock(&pn->l2tp_eth_lock);
>  
>  	return 0;
>  
> @@ -342,22 +318,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
>  	return rc;
>  }
>  
> -static __net_init int l2tp_eth_init_net(struct net *net)
> -{
> -	struct l2tp_eth_net *pn = net_generic(net, l2tp_eth_net_id);
> -
> -	INIT_LIST_HEAD(&pn->l2tp_eth_dev_list);
> -	spin_lock_init(&pn->l2tp_eth_lock);
> -
> -	return 0;
> -}
> -
> -static struct pernet_operations l2tp_eth_net_ops = {
> -	.init = l2tp_eth_init_net,
> -	.id   = &l2tp_eth_net_id,
> -	.size = sizeof(struct l2tp_eth_net),
> -};
> -
>  
>  static const struct l2tp_nl_cmd_ops l2tp_eth_nl_cmd_ops = {
>  	.session_create	= l2tp_eth_create,
> @@ -371,25 +331,18 @@ static int __init l2tp_eth_init(void)
>  
>  	err = l2tp_nl_register_ops(L2TP_PWTYPE_ETH, &l2tp_eth_nl_cmd_ops);
>  	if (err)
> -		goto out;
> -
> -	err = register_pernet_device(&l2tp_eth_net_ops);
> -	if (err)
> -		goto out_unreg;
> +		goto err;
>  
>  	pr_info("L2TP ethernet pseudowire support (L2TPv3)\n");
>  
>  	return 0;
>  
> -out_unreg:
> -	l2tp_nl_unregister_ops(L2TP_PWTYPE_ETH);
> -out:
> +err:
>  	return err;
>  }
>  
>  static void __exit l2tp_eth_exit(void)
>  {
> -	unregister_pernet_device(&l2tp_eth_net_ops);
>  	l2tp_nl_unregister_ops(L2TP_PWTYPE_ETH);
>  }
>  
> -- 
> 2.14.2
> 

-- 
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH net] l2tp: fix l2tp_eth module loading

[Guillaume Nault]

On Thu, Sep 28, 2017 at 03:17:28PM +0100, Tom Parkin wrote:
> On Thu, Sep 28, 2017 at 03:44:38PM +0200, Guillaume Nault wrote:
> > The l2tp_eth module crashes if its netlink callbacks are run when the
> > pernet data aren't initialised.
> > 
> > We should normally register_pernet_device() before the genl callbacks.
> > However, the pernet data only maintain a list of l2tpeth interfaces,
> > and this list is never used. So let's just drop pernet handling
> > instead.
> > 
> > Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
> > Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
> 
> Whoops.  I think this was intended to clear up the devices in the net
> namespace,
Yes, that's what I thought too. That's what virtual devices are
supposed to do and think I'll eventually implement this at a later
time.

> but since l2tp_core.c already deletes tunnels on namespace
> exit I don't think it's necessary for l2tp_eth.c to do anything more.
> 
Well, removing l2tpeth devices is just a side effect of closing the
tunnel. But the tunnel may be in a different namespace than the device,
in case the later was moved after creation. In this case, the l2tpeth
interface isn't deleted when its namespace is destroyed. It's moved
to the initial namespace instead (because, for now, l2tpeth devices
don't implement ->rtnl_link_ops).

These are shortcomings I'd like to fix, but there are more important
issues to tackle first.

Re: [PATCH net] l2tp: fix l2tp_eth module loading

[David Miller]

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Thu, 28 Sep 2017 15:44:38 +0200

> The l2tp_eth module crashes if its netlink callbacks are run when the
> pernet data aren't initialised.
> 
> We should normally register_pernet_device() before the genl callbacks.
> However, the pernet data only maintain a list of l2tpeth interfaces,
> and this list is never used. So let's just drop pernet handling
> instead.
> 
> Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

Applied and queued up for -stable, thanks.