From 5846c131c39b6d0add36ec19dc8650700690f930 Mon Sep 17 00:00:00 2001 From: Guillaume Nault Date: Thu, 12 Apr 2018 20:50:33 +0200 Subject: [PATCH 1/3] l2tp: hold reference on tunnels in netlink dumps l2tp_tunnel_find_nth() is unsafe: no reference is held on the returned tunnel, therefore it can be freed whenever the caller uses it. This patch defines l2tp_tunnel_get_nth() which works similarly, but also takes a reference on the returned tunnel. The caller then has to drop it after it stops using the tunnel. Convert netlink dumps to make them safe against concurrent tunnel deletion. Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP") Signed-off-by: Guillaume Nault Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 20 ++++++++++++++++++++ net/l2tp/l2tp_core.h | 2 ++ net/l2tp/l2tp_netlink.c | 11 ++++++++--- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 0fbd3ee26165dd..c8c4183f0f379d 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -183,6 +183,26 @@ struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id) } EXPORT_SYMBOL_GPL(l2tp_tunnel_get); +struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth) +{ + const struct l2tp_net *pn = l2tp_pernet(net); + struct l2tp_tunnel *tunnel; + int count = 0; + + rcu_read_lock_bh(); + list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) { + if (++count > nth) { + l2tp_tunnel_inc_refcount(tunnel); + rcu_read_unlock_bh(); + return tunnel; + } + } + rcu_read_unlock_bh(); + + return NULL; +} +EXPORT_SYMBOL_GPL(l2tp_tunnel_get_nth); + /* Lookup a session. A new reference is held on the returned session. */ struct l2tp_session *l2tp_session_get(const struct net *net, struct l2tp_tunnel *tunnel, diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h index ba33cbec71eb2e..e4896413b2b633 100644 --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -212,6 +212,8 @@ static inline void *l2tp_session_priv(struct l2tp_session *session) } struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id); +struct l2tp_tunnel *l2tp_tunnel_get_nth(const struct net *net, int nth); + void l2tp_tunnel_free(struct l2tp_tunnel *tunnel); struct l2tp_session *l2tp_session_get(const struct net *net, diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c index b05dbd9ffcb2b1..6616c9fd292f1d 100644 --- a/net/l2tp/l2tp_netlink.c +++ b/net/l2tp/l2tp_netlink.c @@ -487,14 +487,17 @@ static int l2tp_nl_cmd_tunnel_dump(struct sk_buff *skb, struct netlink_callback struct net *net = sock_net(skb->sk); for (;;) { - tunnel = l2tp_tunnel_find_nth(net, ti); + tunnel = l2tp_tunnel_get_nth(net, ti); if (tunnel == NULL) goto out; if (l2tp_nl_tunnel_send(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, NLM_F_MULTI, - tunnel, L2TP_CMD_TUNNEL_GET) < 0) + tunnel, L2TP_CMD_TUNNEL_GET) < 0) { + l2tp_tunnel_dec_refcount(tunnel); goto out; + } + l2tp_tunnel_dec_refcount(tunnel); ti++; } @@ -848,7 +851,7 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback for (;;) { if (tunnel == NULL) { - tunnel = l2tp_tunnel_find_nth(net, ti); + tunnel = l2tp_tunnel_get_nth(net, ti); if (tunnel == NULL) goto out; } @@ -856,6 +859,7 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback session = l2tp_session_get_nth(tunnel, si); if (session == NULL) { ti++; + l2tp_tunnel_dec_refcount(tunnel); tunnel = NULL; si = 0; continue; @@ -865,6 +869,7 @@ static int l2tp_nl_cmd_session_dump(struct sk_buff *skb, struct netlink_callback cb->nlh->nlmsg_seq, NLM_F_MULTI, session, L2TP_CMD_SESSION_GET) < 0) { l2tp_session_dec_refcount(session); + l2tp_tunnel_dec_refcount(tunnel); break; } l2tp_session_dec_refcount(session); From 0e0c3fee3a59a387aeecc4fca6f3a2e9615a5443 Mon Sep 17 00:00:00 2001 From: Guillaume Nault Date: Thu, 12 Apr 2018 20:50:34 +0200 Subject: [PATCH 2/3] l2tp: hold reference on tunnels printed in pppol2tp proc file Use l2tp_tunnel_get_nth() instead of l2tp_tunnel_find_nth(), to be safe against concurrent tunnel deletion. Unlike sessions, we can't drop the reference held on tunnels in pppol2tp_seq_show(). Tunnels are reused across several calls to pppol2tp_seq_start() when iterating over sessions. These iterations need the tunnel for accessing the next session. Therefore the only safe moment for dropping the reference is just before searching for the next tunnel. Normally, the last invocation of pppol2tp_next_tunnel() doesn't find any new tunnel, so it drops the last tunnel without taking any new reference. However, in case of error, pppol2tp_seq_stop() is called directly, so we have to drop the reference there. Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") Signed-off-by: Guillaume Nault Signed-off-by: David S. Miller --- net/l2tp/l2tp_ppp.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 896bbca9bdaab4..7d0c963680e64f 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -1551,16 +1551,19 @@ struct pppol2tp_seq_data { static void pppol2tp_next_tunnel(struct net *net, struct pppol2tp_seq_data *pd) { + /* Drop reference taken during previous invocation */ + if (pd->tunnel) + l2tp_tunnel_dec_refcount(pd->tunnel); + for (;;) { - pd->tunnel = l2tp_tunnel_find_nth(net, pd->tunnel_idx); + pd->tunnel = l2tp_tunnel_get_nth(net, pd->tunnel_idx); pd->tunnel_idx++; - if (pd->tunnel == NULL) - break; + /* Only accept L2TPv2 tunnels */ + if (!pd->tunnel || pd->tunnel->version == 2) + return; - /* Ignore L2TPv3 tunnels */ - if (pd->tunnel->version < 3) - break; + l2tp_tunnel_dec_refcount(pd->tunnel); } } @@ -1609,7 +1612,14 @@ static void *pppol2tp_seq_next(struct seq_file *m, void *v, loff_t *pos) static void pppol2tp_seq_stop(struct seq_file *p, void *v) { - /* nothing to do */ + struct pppol2tp_seq_data *pd = v; + + if (!pd || pd == SEQ_START_TOKEN) + return; + + /* Drop reference taken by last invocation of pppol2tp_next_tunnel() */ + if (pd->tunnel) + l2tp_tunnel_dec_refcount(pd->tunnel); } static void pppol2tp_seq_tunnel_show(struct seq_file *m, void *v) From f726214d9b23e5fce8c11937577a289a3202498f Mon Sep 17 00:00:00 2001 From: Guillaume Nault Date: Thu, 12 Apr 2018 20:50:35 +0200 Subject: [PATCH 3/3] l2tp: hold reference on tunnels printed in l2tp/tunnels debugfs file Use l2tp_tunnel_get_nth() instead of l2tp_tunnel_find_nth(), to be safe against concurrent tunnel deletion. Use the same mechanism as in l2tp_ppp.c for dropping the reference taken by l2tp_tunnel_get_nth(). That is, drop the reference just before looking up the next tunnel. In case of error, drop the last accessed tunnel in l2tp_dfs_seq_stop(). That was the last use of l2tp_tunnel_find_nth(). Fixes: 0ad6614048cf ("l2tp: Add debugfs files for dumping l2tp debug info") Signed-off-by: Guillaume Nault Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 20 -------------------- net/l2tp/l2tp_core.h | 1 - net/l2tp/l2tp_debugfs.c | 15 +++++++++++++-- 3 files changed, 13 insertions(+), 23 deletions(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index c8c4183f0f379d..40261cb68e8368 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -355,26 +355,6 @@ int l2tp_session_register(struct l2tp_session *session, } EXPORT_SYMBOL_GPL(l2tp_session_register); -struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth) -{ - struct l2tp_net *pn = l2tp_pernet(net); - struct l2tp_tunnel *tunnel; - int count = 0; - - rcu_read_lock_bh(); - list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) { - if (++count > nth) { - rcu_read_unlock_bh(); - return tunnel; - } - } - - rcu_read_unlock_bh(); - - return NULL; -} -EXPORT_SYMBOL_GPL(l2tp_tunnel_find_nth); - /***************************************************************************** * Receive data handling *****************************************************************************/ diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h index e4896413b2b633..c199020f8a8a31 100644 --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -222,7 +222,6 @@ struct l2tp_session *l2tp_session_get(const struct net *net, struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth); struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net, const char *ifname); -struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth); int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c index 72e713da473336..b8f9d45bfeb14c 100644 --- a/net/l2tp/l2tp_debugfs.c +++ b/net/l2tp/l2tp_debugfs.c @@ -47,7 +47,11 @@ struct l2tp_dfs_seq_data { static void l2tp_dfs_next_tunnel(struct l2tp_dfs_seq_data *pd) { - pd->tunnel = l2tp_tunnel_find_nth(pd->net, pd->tunnel_idx); + /* Drop reference taken during previous invocation */ + if (pd->tunnel) + l2tp_tunnel_dec_refcount(pd->tunnel); + + pd->tunnel = l2tp_tunnel_get_nth(pd->net, pd->tunnel_idx); pd->tunnel_idx++; } @@ -96,7 +100,14 @@ static void *l2tp_dfs_seq_next(struct seq_file *m, void *v, loff_t *pos) static void l2tp_dfs_seq_stop(struct seq_file *p, void *v) { - /* nothing to do */ + struct l2tp_dfs_seq_data *pd = v; + + if (!pd || pd == SEQ_START_TOKEN) + return; + + /* Drop reference taken by last invocation of l2tp_dfs_next_tunnel() */ + if (pd->tunnel) + l2tp_tunnel_dec_refcount(pd->tunnel); } static void l2tp_dfs_seq_tunnel_show(struct seq_file *m, void *v)