* [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
@ 2017-09-15 9:08 Sabrina Dubroca
2017-09-15 9:42 ` Tom Parkin
0 siblings, 1 reply; 4+ messages in thread
From: Sabrina Dubroca @ 2017-09-15 9:08 UTC (permalink / raw)
To: netdev; +Cc: Sabrina Dubroca, Guillaume Nault, Xin Long
The tunnel is currently removed from the list during destruction. This
can lead to a double-free of the struct sock if we try to delete the tunnel
twice fast enough.
The first delete operation does a lookup (l2tp_tunnel_get), finds the
tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
l2tp_tunnel_del_work.
The second delete operation also finds the tunnel and calls
l2tp_tunnel_delete. If the workqueue has already fired and started
running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
same tunnel a second time, and try to free the socket again.
Add a dead flag and remove tunnel from its list earlier. Then we can
remove the check of queue_work's result that was meant to prevent that
race but doesn't.
Reproducer:
ip l2tp add tunnel tunnel_id 3000 peer_tunnel_id 4000 local 192.168.0.2 remote 192.168.0.1 encap udp udp_sport 5000 udp_dport 6000
ip l2tp add session name l2tp1 tunnel_id 3000 session_id 1000 peer_session_id 2000
ip link set l2tp1 up
ip l2tp del tunnel tunnel_id 3000
ip l2tp del tunnel tunnel_id 3000
Fixes: f8ccac0e4493 ("l2tp: put tunnel socket release on a workqueue")
Reported-by: Jianlin Shi <jishi@redhat.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
net/l2tp/l2tp_core.c | 30 ++++++++++++++++--------------
net/l2tp/l2tp_core.h | 4 +++-
2 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ee485df73ccd..26553d3c4f8f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1245,7 +1245,6 @@ EXPORT_SYMBOL_GPL(l2tp_xmit_skb);
static void l2tp_tunnel_destruct(struct sock *sk)
{
struct l2tp_tunnel *tunnel = l2tp_tunnel(sk);
- struct l2tp_net *pn;
if (tunnel == NULL)
goto end;
@@ -1269,13 +1268,6 @@ static void l2tp_tunnel_destruct(struct sock *sk)
sk->sk_destruct = tunnel->old_sk_destruct;
sk->sk_user_data = NULL;
- /* Remove the tunnel struct from the tunnel list */
- pn = l2tp_pernet(tunnel->l2tp_net);
- spin_lock_bh(&pn->l2tp_tunnel_list_lock);
- list_del_rcu(&tunnel->list);
- spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
- atomic_dec(&l2tp_tunnel_count);
-
l2tp_tunnel_closeall(tunnel);
tunnel->sock = NULL;
@@ -1685,14 +1677,24 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
/* This function is used by the netlink TUNNEL_DELETE command.
*/
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
{
+ struct l2tp_net *pn;
+
+ if (tunnel->dead)
+ return;
+
l2tp_tunnel_inc_refcount(tunnel);
- if (false == queue_work(l2tp_wq, &tunnel->del_work)) {
- l2tp_tunnel_dec_refcount(tunnel);
- return 1;
- }
- return 0;
+
+ /* Remove the tunnel struct from the tunnel list */
+ pn = l2tp_pernet(tunnel->l2tp_net);
+ spin_lock_bh(&pn->l2tp_tunnel_list_lock);
+ tunnel->dead = 1;
+ list_del_rcu(&tunnel->list);
+ spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
+ atomic_dec(&l2tp_tunnel_count);
+
+ queue_work(l2tp_wq, &tunnel->del_work);
}
EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a305e0c5925a..173e68bb8119 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -160,6 +160,8 @@ struct l2tp_tunnel_cfg {
struct l2tp_tunnel {
int magic; /* Should be L2TP_TUNNEL_MAGIC */
+ int dead;
+
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
bool acpt_newsess; /* Indicates whether this
@@ -254,7 +256,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
struct l2tp_tunnel **tunnelp);
void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
struct l2tp_session *l2tp_session_create(int priv_size,
struct l2tp_tunnel *tunnel,
u32 session_id, u32 peer_session_id,
--
2.14.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
2017-09-15 9:08 [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete Sabrina Dubroca
@ 2017-09-15 9:42 ` Tom Parkin
2017-09-15 14:55 ` Sabrina Dubroca
0 siblings, 1 reply; 4+ messages in thread
From: Tom Parkin @ 2017-09-15 9:42 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: netdev, Guillaume Nault, Xin Long
[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]
On Fri, Sep 15, 2017 at 11:08:07AM +0200, Sabrina Dubroca wrote:
> The tunnel is currently removed from the list during destruction. This
> can lead to a double-free of the struct sock if we try to delete the tunnel
> twice fast enough.
>
> The first delete operation does a lookup (l2tp_tunnel_get), finds the
> tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
> l2tp_tunnel_del_work.
>
> The second delete operation also finds the tunnel and calls
> l2tp_tunnel_delete. If the workqueue has already fired and started
> running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> same tunnel a second time, and try to free the socket again.
>
> Add a dead flag and remove tunnel from its list earlier. Then we can
> remove the check of queue_work's result that was meant to prevent that
> race but doesn't.
How do we avoid leaving stale information on the tunnel list for
use-cases which don't delete tunnels using netlink? For example the
L2TPv2 ppp/socket API depends on sk_destruct to clean up the kernel
context on socket destruction. Similarly, userspace may just close
the tunnel socket without first making netlink calls to delete the
tunnel.
By moving the tunnel list removal from l2tp_tunnel_destruct to
l2tp_tunnel_delete I can't see how codepaths which don't involve
l2tp_tunnel_delete don't end up with a corrupted tunnel list.
Tom
--
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
2017-09-15 9:42 ` Tom Parkin
@ 2017-09-15 14:55 ` Sabrina Dubroca
2017-09-15 20:38 ` Guillaume Nault
0 siblings, 1 reply; 4+ messages in thread
From: Sabrina Dubroca @ 2017-09-15 14:55 UTC (permalink / raw)
To: Tom Parkin; +Cc: netdev, Guillaume Nault, Xin Long
2017-09-15, 10:42:59 +0100, Tom Parkin wrote:
> On Fri, Sep 15, 2017 at 11:08:07AM +0200, Sabrina Dubroca wrote:
> > The tunnel is currently removed from the list during destruction. This
> > can lead to a double-free of the struct sock if we try to delete the tunnel
> > twice fast enough.
> >
> > The first delete operation does a lookup (l2tp_tunnel_get), finds the
> > tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
> > l2tp_tunnel_del_work.
> >
> > The second delete operation also finds the tunnel and calls
> > l2tp_tunnel_delete. If the workqueue has already fired and started
> > running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> > same tunnel a second time, and try to free the socket again.
> >
> > Add a dead flag and remove tunnel from its list earlier. Then we can
> > remove the check of queue_work's result that was meant to prevent that
> > race but doesn't.
>
> How do we avoid leaving stale information on the tunnel list for
> use-cases which don't delete tunnels using netlink? For example the
> L2TPv2 ppp/socket API depends on sk_destruct to clean up the kernel
> context on socket destruction. Similarly, userspace may just close
> the tunnel socket without first making netlink calls to delete the
> tunnel.
>
> By moving the tunnel list removal from l2tp_tunnel_destruct to
> l2tp_tunnel_delete I can't see how codepaths which don't involve
> l2tp_tunnel_delete don't end up with a corrupted tunnel list.
Ok, thanks for pointing that out. We could go with just the ->dead
flag then. I'm not sure whether we need to set it in
l2tp_tunnel_destruct as well.
-------- 8< --------
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ee485df73ccd..e74596418169 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1685,14 +1685,12 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
/* This function is used by the netlink TUNNEL_DELETE command.
*/
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
{
- l2tp_tunnel_inc_refcount(tunnel);
- if (false == queue_work(l2tp_wq, &tunnel->del_work)) {
- l2tp_tunnel_dec_refcount(tunnel);
- return 1;
+ if (!test_and_set_bit(1, &tunnel->dead)) {
+ l2tp_tunnel_inc_refcount(tunnel);
+ queue_work(l2tp_wq, &tunnel->del_work);
}
- return 0;
}
EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a305e0c5925a..deda869504d0 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -160,6 +160,9 @@ struct l2tp_tunnel_cfg {
struct l2tp_tunnel {
int magic; /* Should be L2TP_TUNNEL_MAGIC */
+
+ unsigned long dead;
+
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
bool acpt_newsess; /* Indicates whether this
@@ -254,7 +257,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
struct l2tp_tunnel **tunnelp);
void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
struct l2tp_session *l2tp_session_create(int priv_size,
struct l2tp_tunnel *tunnel,
u32 session_id, u32 peer_session_id,
--
Sabrina
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
2017-09-15 14:55 ` Sabrina Dubroca
@ 2017-09-15 20:38 ` Guillaume Nault
0 siblings, 0 replies; 4+ messages in thread
From: Guillaume Nault @ 2017-09-15 20:38 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: Tom Parkin, netdev, Xin Long
On Fri, Sep 15, 2017 at 04:55:02PM +0200, Sabrina Dubroca wrote:
> 2017-09-15, 10:42:59 +0100, Tom Parkin wrote:
> > On Fri, Sep 15, 2017 at 11:08:07AM +0200, Sabrina Dubroca wrote:
> > > The tunnel is currently removed from the list during destruction. This
> > > can lead to a double-free of the struct sock if we try to delete the tunnel
> > > twice fast enough.
> > >
> > > The first delete operation does a lookup (l2tp_tunnel_get), finds the
> > > tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
> > > l2tp_tunnel_del_work.
> > >
> > > The second delete operation also finds the tunnel and calls
> > > l2tp_tunnel_delete. If the workqueue has already fired and started
> > > running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> > > same tunnel a second time, and try to free the socket again.
> > >
> > > Add a dead flag and remove tunnel from its list earlier. Then we can
> > > remove the check of queue_work's result that was meant to prevent that
> > > race but doesn't.
> >
> > How do we avoid leaving stale information on the tunnel list for
> > use-cases which don't delete tunnels using netlink? For example the
> > L2TPv2 ppp/socket API depends on sk_destruct to clean up the kernel
> > context on socket destruction. Similarly, userspace may just close
> > the tunnel socket without first making netlink calls to delete the
> > tunnel.
> >
> > By moving the tunnel list removal from l2tp_tunnel_destruct to
> > l2tp_tunnel_delete I can't see how codepaths which don't involve
> > l2tp_tunnel_delete don't end up with a corrupted tunnel list.
>
> Ok, thanks for pointing that out. We could go with just the ->dead
> flag then.
>
Yes, I guess that's the least instrusive way to fix this issue.
> I'm not sure whether we need to set it in l2tp_tunnel_destruct as
> well.
>
It shouldn't be necessary. If the socket is managed by userspace, then
l2tp_tunnel_del_work() gets it using sockfd_lookup(). Therefore it
shouldn't find it if userspace is in the process of releasing it.
Also l2tp_tunnel_del_work() doesn't try to release sockets handled by
userspace, so not checking ->dead in l2tp_tunnel_destruct() should be
safe.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-15 20:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-15 9:08 [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete Sabrina Dubroca
2017-09-15 9:42 ` Tom Parkin
2017-09-15 14:55 ` Sabrina Dubroca
2017-09-15 20:38 ` 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).