From 95075150d0bdaa78cc350efc606e6ed02fa2a991 Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:49 +0100 Subject: [PATCH 1/9] l2tp: avoid multiple assignments checkpatch warns about multiple assignments. Update l2tp accordingly. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 6 +++--- net/l2tp/l2tp_ip.c | 12 ++++++++---- net/l2tp/l2tp_ip6.c | 6 ++++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 7e3523015d6f..b871cceeff7c 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -621,8 +621,8 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb, int length) { struct l2tp_tunnel *tunnel = session->tunnel; + u32 ns = 0, nr = 0; int offset; - u32 ns, nr; /* Parse and check optional cookie */ if (session->peer_cookie_len > 0) { @@ -644,7 +644,6 @@ void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb, * the control of the LNS. If no sequence numbers present but * we were expecting them, discard frame. */ - ns = nr = 0; L2TP_SKB_CB(skb)->has_seq = 0; if (tunnel->version == L2TP_HDR_VER_2) { if (hdrflags & L2TP_HDRFLAG_S) { @@ -826,7 +825,8 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb) } /* Point to L2TP header */ - optr = ptr = skb->data; + optr = skb->data; + ptr = skb->data; /* Get L2TP header flags */ hdrflags = ntohs(*(__be16 *)ptr); diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c index d81564cf1e7f..a159cb2bf0f4 100644 --- a/net/l2tp/l2tp_ip.c +++ b/net/l2tp/l2tp_ip.c @@ -124,7 +124,8 @@ static int l2tp_ip_recv(struct sk_buff *skb) goto discard; /* Point to L2TP header */ - optr = ptr = skb->data; + optr = skb->data; + ptr = skb->data; session_id = ntohl(*((__be32 *)ptr)); ptr += 4; @@ -153,7 +154,8 @@ static int l2tp_ip_recv(struct sk_buff *skb) goto discard_sess; /* Point to L2TP header */ - optr = ptr = skb->data; + optr = skb->data; + ptr = skb->data; ptr += 4; pr_debug("%s: ip recv\n", tunnel->name); print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length); @@ -284,8 +286,10 @@ static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len) chk_addr_ret != RTN_MULTICAST && chk_addr_ret != RTN_BROADCAST) goto out; - if (addr->l2tp_addr.s_addr) - inet->inet_rcv_saddr = inet->inet_saddr = addr->l2tp_addr.s_addr; + if (addr->l2tp_addr.s_addr) { + inet->inet_rcv_saddr = addr->l2tp_addr.s_addr; + inet->inet_saddr = addr->l2tp_addr.s_addr; + } if (chk_addr_ret == RTN_MULTICAST || chk_addr_ret == RTN_BROADCAST) inet->inet_saddr = 0; /* Use device */ diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c index 614febf8dd0d..bc757bc7e264 100644 --- a/net/l2tp/l2tp_ip6.c +++ b/net/l2tp/l2tp_ip6.c @@ -137,7 +137,8 @@ static int l2tp_ip6_recv(struct sk_buff *skb) goto discard; /* Point to L2TP header */ - optr = ptr = skb->data; + optr = skb->data; + ptr = skb->data; session_id = ntohl(*((__be32 *)ptr)); ptr += 4; @@ -166,7 +167,8 @@ static int l2tp_ip6_recv(struct sk_buff *skb) goto discard_sess; /* Point to L2TP header */ - optr = ptr = skb->data; + optr = skb->data; + ptr = skb->data; ptr += 4; pr_debug("%s: ip recv\n", tunnel->name); print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, ptr, length); From 7a379558c28c435681221aa5d84ead8ff19211be Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:50 +0100 Subject: [PATCH 2/9] l2tp: WARN_ON rather than BUG_ON in l2tp_dfs_seq_start l2tp_dfs_seq_start had a BUG_ON to catch a possible programming error in l2tp_dfs_seq_open. Since we can easily bail out of l2tp_dfs_seq_start, prefer to do that and flag the error with a WARN_ON rather than crashing the kernel. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_debugfs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/l2tp/l2tp_debugfs.c b/net/l2tp/l2tp_debugfs.c index 72ba83aa0eaf..96cb9601c21b 100644 --- a/net/l2tp/l2tp_debugfs.c +++ b/net/l2tp/l2tp_debugfs.c @@ -72,7 +72,10 @@ static void *l2tp_dfs_seq_start(struct seq_file *m, loff_t *offs) if (!pos) goto out; - BUG_ON(!m->private); + if (WARN_ON(!m->private)) { + pd = NULL; + goto out; + } pd = m->private; if (!pd->tunnel) From ce2f86ae253de3f3e961d4f91438604a97c29752 Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:51 +0100 Subject: [PATCH 3/9] l2tp: remove BUG_ON in l2tp_session_queue_purge l2tp_session_queue_purge is only called from l2tp_core.c, and it's easy to statically analyse the code paths calling it to validate that it should never be passed a NULL session pointer. Having a BUG_ON checking the session pointer triggers a checkpatch warning. Since the BUG_ON is of no value, remove it to avoid the warning. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 1 - 1 file changed, 1 deletion(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index b871cceeff7c..a1ed8baa5aaa 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -777,7 +777,6 @@ static int l2tp_session_queue_purge(struct l2tp_session *session) { struct sk_buff *skb = NULL; - BUG_ON(!session); BUG_ON(session->magic != L2TP_SESSION_MAGIC); while ((skb = skb_dequeue(&session->reorder_q))) { atomic_long_inc(&session->stats.rx_errors); From cd3e29b333cc2571a5875d1b45c460dc948a2f95 Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:52 +0100 Subject: [PATCH 4/9] l2tp: remove BUG_ON in l2tp_tunnel_closeall l2tp_tunnel_closeall is only called from l2tp_core.c, and it's easy to statically analyse the code path calling it to validate that it should never be passed a NULL tunnel pointer. Having a BUG_ON checking the tunnel pointer triggers a checkpatch warning. Since the BUG_ON is of no value, remove it to avoid the warning. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index a1ed8baa5aaa..6be3f2e69efd 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1188,8 +1188,6 @@ static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel) struct hlist_node *tmp; struct l2tp_session *session; - BUG_ON(!tunnel); - l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing all sessions...\n", tunnel->name); From 1aa646ac71feb987a9571e1d70980d10ff495fbc Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:53 +0100 Subject: [PATCH 5/9] l2tp: don't BUG_ON session magic checks in l2tp_ppp checkpatch advises that WARN_ON and recovery code are preferred over BUG_ON which crashes the kernel. l2tp_ppp.c's BUG_ON checks of the l2tp session structure's "magic" field occur in code paths where it's reasonably easy to recover: * In the case of pppol2tp_sock_to_session, we can return NULL and the caller will bail out appropriately. There is no change required to any of the callsites of this function since they already handle pppol2tp_sock_to_session returning NULL. * In the case of pppol2tp_session_destruct we can just avoid decrementing the reference count on the suspect session structure. In the worst case scenario this results in a memory leak, which is preferable to a crash. Convert these uses of BUG_ON to WARN_ON accordingly. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_ppp.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 4389df66af35..2aeee648c080 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -163,8 +163,11 @@ static inline struct l2tp_session *pppol2tp_sock_to_session(struct sock *sk) sock_put(sk); goto out; } - - BUG_ON(session->magic != L2TP_SESSION_MAGIC); + if (WARN_ON(session->magic != L2TP_SESSION_MAGIC)) { + session = NULL; + sock_put(sk); + goto out; + } out: return session; @@ -419,7 +422,8 @@ static void pppol2tp_session_destruct(struct sock *sk) if (session) { sk->sk_user_data = NULL; - BUG_ON(session->magic != L2TP_SESSION_MAGIC); + if (WARN_ON(session->magic != L2TP_SESSION_MAGIC)) + return; l2tp_session_dec_refcount(session); } } From ebb4f5e6e4cd68a856aeea5d319a76f47543c3a9 Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:54 +0100 Subject: [PATCH 6/9] l2tp: don't BUG_ON seqfile checks in l2tp_ppp checkpatch advises that WARN_ON and recovery code are preferred over BUG_ON which crashes the kernel. l2tp_ppp has a BUG_ON check of struct seq_file's private pointer in pppol2tp_seq_start prior to accessing data through that pointer. Rather than crashing, we can simply bail out early and return NULL in order to terminate the seq file processing in much the same way as we do when reaching the end of tunnel/session instances to render. Retain a WARN_ON to help trace possible bugs in this area. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_ppp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 2aeee648c080..13c3153b40d6 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -1478,7 +1478,11 @@ static void *pppol2tp_seq_start(struct seq_file *m, loff_t *offs) if (!pos) goto out; - BUG_ON(!m->private); + if (WARN_ON(!m->private)) { + pd = NULL; + goto out; + } + pd = m->private; net = seq_file_net(m); From 493048f5dfcd0093880414928717f68613ba9157 Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:55 +0100 Subject: [PATCH 7/9] l2tp: WARN_ON rather than BUG_ON in l2tp_session_queue_purge l2tp_session_queue_purge is used during session shutdown to drop any skbs queued for reordering purposes according to L2TP dataplane rules. The BUG_ON in this function checks the session magic feather in an attempt to catch lifetime bugs. Rather than crashing the kernel with a BUG_ON, we can simply WARN_ON and refuse to do anything more -- in the worst case this could result in a leak. However this is highly unlikely given that the session purge only occurs from codepaths which have obtained the session by means of a lookup via. the parent tunnel and which check the session "dead" flag to protect against shutdown races. While we're here, have l2tp_session_queue_purge return void rather than an integer, since neither of the callsites checked the return value. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 6be3f2e69efd..e228480fa529 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -773,16 +773,17 @@ EXPORT_SYMBOL(l2tp_recv_common); /* Drop skbs from the session's reorder_q */ -static int l2tp_session_queue_purge(struct l2tp_session *session) +static void l2tp_session_queue_purge(struct l2tp_session *session) { struct sk_buff *skb = NULL; - BUG_ON(session->magic != L2TP_SESSION_MAGIC); + if (WARN_ON(session->magic != L2TP_SESSION_MAGIC)) + return; + while ((skb = skb_dequeue(&session->reorder_q))) { atomic_long_inc(&session->stats.rx_errors); kfree_skb(skb); } - return 0; } /* Internal UDP receive frame. Do the real work of receiving an L2TP data frame From 0dd62f69d898de3cf31a2f8b59e9a62bb5448457 Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:56 +0100 Subject: [PATCH 8/9] l2tp: remove BUG_ON refcount value in l2tp_session_free l2tp_session_free is only called by l2tp_session_dec_refcount when the reference count reaches zero, so it's of limited value to validate the reference count value in l2tp_session_free itself. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index e228480fa529..50548c61b91e 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1563,8 +1563,6 @@ void l2tp_session_free(struct l2tp_session *session) { struct l2tp_tunnel *tunnel = session->tunnel; - BUG_ON(refcount_read(&session->ref_count) != 0); - if (tunnel) { BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC); l2tp_tunnel_dec_refcount(tunnel); From ab6934e084e5eee665adf6e834e5096ebae4a95f Mon Sep 17 00:00:00 2001 From: Tom Parkin Date: Fri, 24 Jul 2020 16:31:57 +0100 Subject: [PATCH 9/9] l2tp: WARN_ON rather than BUG_ON in l2tp_session_free l2tp_session_free called BUG_ON if the tunnel magic feather value wasn't correct. The intent of this was to catch lifetime bugs; for example early tunnel free due to incorrect use of reference counts. Since the tunnel magic feather being wrong indicates either early free or structure corruption, we can avoid doing more damage by simply leaving the tunnel structure alone. If the tunnel refcount isn't dropped when it should be, the tunnel instance will remain in the kernel, resulting in the tunnel structure and socket leaking. Signed-off-by: Tom Parkin Signed-off-by: David S. Miller --- net/l2tp/l2tp_core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 50548c61b91e..e723828e458b 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1564,10 +1564,12 @@ void l2tp_session_free(struct l2tp_session *session) struct l2tp_tunnel *tunnel = session->tunnel; if (tunnel) { - BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC); + if (WARN_ON(tunnel->magic != L2TP_TUNNEL_MAGIC)) + goto out; l2tp_tunnel_dec_refcount(tunnel); } +out: kfree(session); } EXPORT_SYMBOL_GPL(l2tp_session_free);