From: Tom Parkin <tparkin@katalix.com>
To: Sabrina Dubroca <sd@queasysnail.net>
Cc: netdev@vger.kernel.org, Guillaume Nault <g.nault@alphalink.fr>,
Xin Long <lucien.xin@gmail.com>
Subject: Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
Date: Fri, 15 Sep 2017 10:42:59 +0100 [thread overview]
Message-ID: <20170915094259.GA3209@jackdaw> (raw)
In-Reply-To: <f07e7ba88b9b4651900a888796e3f8816d06a8a8.1505396134.git.sd@queasysnail.net>
[-- 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 --]
next prev parent reply other threads:[~2017-09-15 9:49 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-15 9:08 [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete Sabrina Dubroca
2017-09-15 9:42 ` Tom Parkin [this message]
2017-09-15 14:55 ` Sabrina Dubroca
2017-09-15 20:38 ` Guillaume Nault
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170915094259.GA3209@jackdaw \
--to=tparkin@katalix.com \
--cc=g.nault@alphalink.fr \
--cc=lucien.xin@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=sd@queasysnail.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).