Skip to content

Commit

Permalink
l2tp: initialise l2tp_eth sessions before registering them
Browse files Browse the repository at this point in the history
Sessions must be initialised before being made externally visible by
l2tp_session_register(). Otherwise the session may be concurrently
deleted before being initialised, which can confuse the deletion path
and eventually lead to kernel oops.

Therefore, we need to move l2tp_session_register() down in
l2tp_eth_create(), but also handle the intermediate step where only the
session or the netdevice has been registered.

We can't just call l2tp_session_register() in ->ndo_init() because
we'd have no way to properly undo this operation in ->ndo_uninit().
Instead, let's register the session and the netdevice in two different
steps and protect the session's device pointer with RCU.

And now that we allow the session's .dev field to be NULL, we don't
need to prevent the netdevice from being removed anymore. So we can
drop the dev_hold() and dev_put() calls in l2tp_eth_create() and
l2tp_eth_dev_uninit().

Fixes: d9e31d1 ("l2tp: Add L2TP ethernet pseudowire support")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Guillaume Nault authored and David S. Miller committed Oct 29, 2017
1 parent 3953ae7 commit ee28de6
Showing 1 changed file with 75 additions and 31 deletions.
106 changes: 75 additions & 31 deletions net/l2tp/l2tp_eth.c
Original file line number Diff line number Diff line change
@@ -54,7 +54,7 @@ struct l2tp_eth {

/* via l2tp_session_priv() */
struct l2tp_eth_sess {
struct net_device *dev;
struct net_device __rcu *dev;
};


@@ -72,7 +72,14 @@ static int l2tp_eth_dev_init(struct net_device *dev)

static void l2tp_eth_dev_uninit(struct net_device *dev)
{
dev_put(dev);
struct l2tp_eth *priv = netdev_priv(dev);
struct l2tp_eth_sess *spriv;

spriv = l2tp_session_priv(priv->session);
RCU_INIT_POINTER(spriv->dev, NULL);
/* No need for synchronize_net() here. We're called by
* unregister_netdev*(), which does the synchronisation for us.
*/
}

static int l2tp_eth_dev_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -130,8 +137,8 @@ static void l2tp_eth_dev_setup(struct net_device *dev)
static void l2tp_eth_dev_recv(struct l2tp_session *session, struct sk_buff *skb, int data_len)
{
struct l2tp_eth_sess *spriv = l2tp_session_priv(session);
struct net_device *dev = spriv->dev;
struct l2tp_eth *priv = netdev_priv(dev);
struct net_device *dev;
struct l2tp_eth *priv;

if (session->debug & L2TP_MSG_DATA) {
unsigned int length;
@@ -155,16 +162,25 @@ static void l2tp_eth_dev_recv(struct l2tp_session *session, struct sk_buff *skb,
skb_dst_drop(skb);
nf_reset(skb);

rcu_read_lock();
dev = rcu_dereference(spriv->dev);
if (!dev)
goto error_rcu;

priv = netdev_priv(dev);
if (dev_forward_skb(dev, skb) == NET_RX_SUCCESS) {
atomic_long_inc(&priv->rx_packets);
atomic_long_add(data_len, &priv->rx_bytes);
} else {
atomic_long_inc(&priv->rx_errors);
}
rcu_read_unlock();

return;

error_rcu:
rcu_read_unlock();
error:
atomic_long_inc(&priv->rx_errors);
kfree_skb(skb);
}

@@ -175,11 +191,15 @@ static void l2tp_eth_delete(struct l2tp_session *session)

if (session) {
spriv = l2tp_session_priv(session);
dev = spriv->dev;

rtnl_lock();
dev = rtnl_dereference(spriv->dev);
if (dev) {
unregister_netdev(dev);
spriv->dev = NULL;
unregister_netdevice(dev);
rtnl_unlock();
module_put(THIS_MODULE);
} else {
rtnl_unlock();
}
}
}
@@ -189,9 +209,20 @@ static void l2tp_eth_show(struct seq_file *m, void *arg)
{
struct l2tp_session *session = arg;
struct l2tp_eth_sess *spriv = l2tp_session_priv(session);
struct net_device *dev = spriv->dev;
struct net_device *dev;

rcu_read_lock();
dev = rcu_dereference(spriv->dev);
if (!dev) {
rcu_read_unlock();
return;
}
dev_hold(dev);
rcu_read_unlock();

seq_printf(m, " interface %s\n", dev->name);

dev_put(dev);
}
#endif

@@ -268,21 +299,14 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
peer_session_id, cfg);
if (IS_ERR(session)) {
rc = PTR_ERR(session);
goto out;
}

l2tp_session_inc_refcount(session);
rc = l2tp_session_register(session, tunnel);
if (rc < 0) {
kfree(session);
goto out;
goto err;
}

dev = alloc_netdev(sizeof(*priv), name, name_assign_type,
l2tp_eth_dev_setup);
if (!dev) {
rc = -ENOMEM;
goto out_del_session;
goto err_sess;
}

dev_net_set(dev, net);
@@ -302,28 +326,48 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
#endif

spriv = l2tp_session_priv(session);
spriv->dev = dev;

rc = register_netdev(dev);
if (rc < 0)
goto out_del_dev;
l2tp_session_inc_refcount(session);

rtnl_lock();

/* Register both device and session while holding the rtnl lock. This
* ensures that l2tp_eth_delete() will see that there's a device to
* unregister, even if it happened to run before we assign spriv->dev.
*/
rc = l2tp_session_register(session, tunnel);
if (rc < 0) {
rtnl_unlock();
goto err_sess_dev;
}

rc = register_netdevice(dev);
if (rc < 0) {
rtnl_unlock();
l2tp_session_delete(session);
l2tp_session_dec_refcount(session);
free_netdev(dev);

return rc;
}

__module_get(THIS_MODULE);
/* Must be done after register_netdev() */
strlcpy(session->ifname, dev->name, IFNAMSIZ);
rcu_assign_pointer(spriv->dev, dev);

rtnl_unlock();

l2tp_session_dec_refcount(session);

dev_hold(dev);
__module_get(THIS_MODULE);

return 0;

out_del_dev:
free_netdev(dev);
spriv->dev = NULL;
out_del_session:
l2tp_session_delete(session);
err_sess_dev:
l2tp_session_dec_refcount(session);
out:
free_netdev(dev);
err_sess:
kfree(session);
err:
return rc;
}

0 comments on commit ee28de6

Please sign in to comment.