Message ID | a7d351fd5948fc69ef17da3c170cd7c152f899c4.1506606179.git.g.nault@alphalink.fr |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | [net] l2tp: fix l2tp_eth module loading | expand |
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 >
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.
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.
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); }
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(-)