* [PATCH net] l2tp: purge socket queues in the .destruct() callback
@ 2017-03-29 6:45 Guillaume Nault
2017-03-29 16:27 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Guillaume Nault @ 2017-03-29 6:45 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
The Rx path may grab the socket right before pppol2tp_release(), but
nothing guarantees that it will enqueue packets before
skb_queue_purge(). Therefore, the socket can be destroyed without its
queues fully purged.
Fix this by purging queues in pppol2tp_session_destruct() where we're
guaranteed nothing is still referencing the socket.
Fixes: 9e9cb6221aa7 ("l2tp: fix userspace reception on plain L2TP sockets")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_ppp.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 36cc56fd0418..123b6a2411a0 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -450,6 +450,10 @@ static void pppol2tp_session_close(struct l2tp_session *session)
static void pppol2tp_session_destruct(struct sock *sk)
{
struct l2tp_session *session = sk->sk_user_data;
+
+ skb_queue_purge(&sk->sk_receive_queue);
+ skb_queue_purge(&sk->sk_write_queue);
+
if (session) {
sk->sk_user_data = NULL;
BUG_ON(session->magic != L2TP_SESSION_MAGIC);
@@ -488,9 +492,6 @@ static int pppol2tp_release(struct socket *sock)
l2tp_session_queue_purge(session);
sock_put(sk);
}
- skb_queue_purge(&sk->sk_receive_queue);
- skb_queue_purge(&sk->sk_write_queue);
-
release_sock(sk);
/* This will delete the session context via
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] l2tp: purge socket queues in the .destruct() callback
2017-03-29 6:45 [PATCH net] l2tp: purge socket queues in the .destruct() callback Guillaume Nault
@ 2017-03-29 16:27 ` David Miller
0 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-03-29 16:27 UTC (permalink / raw)
To: g.nault; +Cc: netdev, jchapman
From: Guillaume Nault <g.nault@alphalink.fr>
Date: Wed, 29 Mar 2017 08:45:29 +0200
> The Rx path may grab the socket right before pppol2tp_release(), but
> nothing guarantees that it will enqueue packets before
> skb_queue_purge(). Therefore, the socket can be destroyed without its
> queues fully purged.
>
> Fix this by purging queues in pppol2tp_session_destruct() where we're
> guaranteed nothing is still referencing the socket.
>
> Fixes: 9e9cb6221aa7 ("l2tp: fix userspace reception on plain L2TP sockets")
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Applied.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net] l2tp: hold tunnel socket when handling control frames in l2tp_ip and l2tp_ip6
@ 2017-03-28 13:32 Guillaume Nault
2017-03-28 13:32 ` [PATCH net] l2tp: purge socket queues in the .destruct() callback Guillaume Nault
0 siblings, 1 reply; 5+ messages in thread
From: Guillaume Nault @ 2017-03-28 13:32 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
The code following l2tp_tunnel_find() expects that a new reference is
held on sk. Either sk_receive_skb() or the discard_put error path will
drop a reference from the tunnel's socket.
This issue exists in both l2tp_ip and l2tp_ip6.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_ip.c | 5 +++--
net/l2tp/l2tp_ip6.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index d25038cfd64e..7208fbe5856b 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -178,9 +178,10 @@ static int l2tp_ip_recv(struct sk_buff *skb)
tunnel_id = ntohl(*(__be32 *) &skb->data[4]);
tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel != NULL)
+ if (tunnel) {
sk = tunnel->sock;
- else {
+ sock_hold(sk);
+ } else {
struct iphdr *iph = (struct iphdr *) skb_network_header(skb);
read_lock_bh(&l2tp_ip_lock);
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index a4abcbc4c09a..516d7ce24ba7 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -191,9 +191,10 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
tunnel_id = ntohl(*(__be32 *) &skb->data[4]);
tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel != NULL)
+ if (tunnel) {
sk = tunnel->sock;
- else {
+ sock_hold(sk);
+ } else {
struct ipv6hdr *iph = ipv6_hdr(skb);
read_lock_bh(&l2tp_ip6_lock);
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net] l2tp: purge socket queues in the .destruct() callback
2017-03-28 13:32 [PATCH net] l2tp: hold tunnel socket when handling control frames in l2tp_ip and l2tp_ip6 Guillaume Nault
@ 2017-03-28 13:32 ` Guillaume Nault
2017-03-29 4:39 ` David Miller
0 siblings, 1 reply; 5+ messages in thread
From: Guillaume Nault @ 2017-03-28 13:32 UTC (permalink / raw)
To: netdev; +Cc: James Chapman
The Rx path may grab the socket right before pppol2tp_release(), but
nothing guarantees that it will enqueue packets before
skb_queue_purge(). Therefore, the socket can be destroyed without its
queues fully purged.
Fix this by purging queues in pppol2tp_session_destruct() where we're
guaranteed nothing is still referencing the socket.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
net/l2tp/l2tp_ppp.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 36cc56fd0418..123b6a2411a0 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -450,6 +450,10 @@ static void pppol2tp_session_close(struct l2tp_session *session)
static void pppol2tp_session_destruct(struct sock *sk)
{
struct l2tp_session *session = sk->sk_user_data;
+
+ skb_queue_purge(&sk->sk_receive_queue);
+ skb_queue_purge(&sk->sk_write_queue);
+
if (session) {
sk->sk_user_data = NULL;
BUG_ON(session->magic != L2TP_SESSION_MAGIC);
@@ -488,9 +492,6 @@ static int pppol2tp_release(struct socket *sock)
l2tp_session_queue_purge(session);
sock_put(sk);
}
- skb_queue_purge(&sk->sk_receive_queue);
- skb_queue_purge(&sk->sk_write_queue);
-
release_sock(sk);
/* This will delete the session context via
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] l2tp: purge socket queues in the .destruct() callback
2017-03-28 13:32 ` [PATCH net] l2tp: purge socket queues in the .destruct() callback Guillaume Nault
@ 2017-03-29 4:39 ` David Miller
2017-03-29 6:37 ` Guillaume Nault
0 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2017-03-29 4:39 UTC (permalink / raw)
To: g.nault; +Cc: netdev, jchapman
From: Guillaume Nault <g.nault@alphalink.fr>
Date: Tue, 28 Mar 2017 15:32:41 +0200
> The Rx path may grab the socket right before pppol2tp_release(), but
> nothing guarantees that it will enqueue packets before
> skb_queue_purge(). Therefore, the socket can be destroyed without its
> queues fully purged.
>
> Fix this by purging queues in pppol2tp_session_destruct() where we're
> guaranteed nothing is still referencing the socket.
>
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
As a quick guess, I'm thinking this problem might have been introduced
by:
====================
commit 9e9cb6221aa7cb04765484fe87cc2d1b92edce64
Author: Guillaume Nault <g.nault@alphalink.fr>
Date: Thu Mar 6 11:15:10 2014 +0100
l2tp: fix userspace reception on plain L2TP sockets
====================
Please add an appropriate Fixes: tag for this patch.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] l2tp: purge socket queues in the .destruct() callback
2017-03-29 4:39 ` David Miller
@ 2017-03-29 6:37 ` Guillaume Nault
0 siblings, 0 replies; 5+ messages in thread
From: Guillaume Nault @ 2017-03-29 6:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, jchapman
On Tue, Mar 28, 2017 at 09:39:50PM -0700, David Miller wrote:
> As a quick guess, I'm thinking this problem might have been introduced
> by:
>
> ====================
> commit 9e9cb6221aa7cb04765484fe87cc2d1b92edce64
> Author: Guillaume Nault <g.nault@alphalink.fr>
> Date: Thu Mar 6 11:15:10 2014 +0100
>
> l2tp: fix userspace reception on plain L2TP sockets
> ====================
>
True. Will resubmit.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-29 16:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-29 6:45 [PATCH net] l2tp: purge socket queues in the .destruct() callback Guillaume Nault
2017-03-29 16:27 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2017-03-28 13:32 [PATCH net] l2tp: hold tunnel socket when handling control frames in l2tp_ip and l2tp_ip6 Guillaume Nault
2017-03-28 13:32 ` [PATCH net] l2tp: purge socket queues in the .destruct() callback Guillaume Nault
2017-03-29 4:39 ` David Miller
2017-03-29 6:37 ` Guillaume Nault
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).