From: Herbert Xu <herbert@gondor.apana.org.au>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Andrey Konovalov <andreyknvl@google.com>,
"David S. Miller" <davem@davemloft.net>,
Johannes Berg <johannes.berg@intel.com>,
Florian Westphal <fw@strlen.de>,
Eric Dumazet <edumazet@google.com>,
Bob Copeland <me@bobcopeland.com>,
Tom Herbert <tom@herbertland.com>,
David Decotigny <decot@googlers.com>,
netdev <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: [v3 PATCH] netlink: Do not schedule work from sk_destruct
Date: Mon, 5 Dec 2016 15:28:21 +0800 [thread overview]
Message-ID: <20161205072820.GB10204@gondor.apana.org.au> (raw)
In-Reply-To: <20161205072600.GA10204@gondor.apana.org.au>
On Mon, Dec 05, 2016 at 03:26:00PM +0800, Herbert Xu wrote:
> On Mon, Dec 05, 2016 at 03:19:46PM +0800, Herbert Xu wrote:
> >
> > Thanks for the patch. It'll obviously work but I wanted avoid that
> > because it penalises the common path for the rare case.
> >
> > Andrey, please try this patch and let me know if it's any better.
> >
> > ---8<---
> > Subject: netlink: Do not schedule work from sk_destruct
>
> Crap, I screwed it up again. Here is a v2 which moves the atomic
> call into the RCU callback as otherwise the socket can be freed from
> another path while we await the RCU callback.
With the move it no longer makes sense to rename deferred_put_nlk_sk
so here is v3 which restores the original name.
---8<---
It is wrong to schedule a work from sk_destruct using the socket
as the memory reserve because the socket will be freed immediately
after the return from sk_destruct.
Instead we should do the deferral prior to sk_free.
This patch does just that.
Fixes: 707693c8a498 ("netlink: Call cb->done from a worker thread")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 602e5eb..246f29d 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -322,11 +322,13 @@ static void netlink_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
sk_mem_charge(sk, skb->truesize);
}
-static void __netlink_sock_destruct(struct sock *sk)
+static void netlink_sock_destruct(struct sock *sk)
{
struct netlink_sock *nlk = nlk_sk(sk);
if (nlk->cb_running) {
+ if (nlk->cb.done)
+ nlk->cb.done(&nlk->cb);
module_put(nlk->cb.module);
kfree_skb(nlk->cb.skb);
}
@@ -348,21 +350,7 @@ static void netlink_sock_destruct_work(struct work_struct *work)
struct netlink_sock *nlk = container_of(work, struct netlink_sock,
work);
- nlk->cb.done(&nlk->cb);
- __netlink_sock_destruct(&nlk->sk);
-}
-
-static void netlink_sock_destruct(struct sock *sk)
-{
- struct netlink_sock *nlk = nlk_sk(sk);
-
- if (nlk->cb_running && nlk->cb.done) {
- INIT_WORK(&nlk->work, netlink_sock_destruct_work);
- schedule_work(&nlk->work);
- return;
- }
-
- __netlink_sock_destruct(sk);
+ sk_free(&nlk->sk);
}
/* This lock without WQ_FLAG_EXCLUSIVE is good on UP and it is _very_ bad on
@@ -667,8 +655,18 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
static void deferred_put_nlk_sk(struct rcu_head *head)
{
struct netlink_sock *nlk = container_of(head, struct netlink_sock, rcu);
+ struct sock *sk = &nlk->sk;
+
+ if (!atomic_dec_and_test(&sk->sk_refcnt))
+ return;
+
+ if (nlk->cb_running && nlk->cb.done) {
+ INIT_WORK(&nlk->work, netlink_sock_destruct_work);
+ schedule_work(&nlk->work);
+ return;
+ }
- sock_put(&nlk->sk);
+ sk_free(sk);
}
static int netlink_release(struct socket *sock)
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
next prev parent reply other threads:[~2016-12-05 8:04 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-03 12:56 net: use-after-free in worker_thread Andrey Konovalov
2016-12-03 12:58 ` Andrey Konovalov
2016-12-03 13:05 ` Andrey Konovalov
2016-12-03 13:49 ` Eric Dumazet
2016-12-03 15:39 ` Andrey Konovalov
2016-12-05 7:21 ` Herbert Xu
2016-12-03 17:41 ` Cong Wang
2016-12-03 18:14 ` Cong Wang
2016-12-05 7:19 ` Herbert Xu
2016-12-05 7:26 ` [v2 PATCH] netlink: Do not schedule work from sk_destruct Herbert Xu
2016-12-05 7:28 ` Herbert Xu [this message]
2016-12-05 11:51 ` [v3 " Andrey Konovalov
2016-12-06 0:44 ` David Miller
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=20161205072820.GB10204@gondor.apana.org.au \
--to=herbert@gondor.apana.org.au \
--cc=andreyknvl@google.com \
--cc=davem@davemloft.net \
--cc=decot@googlers.com \
--cc=edumazet@google.com \
--cc=fw@strlen.de \
--cc=johannes.berg@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=me@bobcopeland.com \
--cc=netdev@vger.kernel.org \
--cc=tom@herbertland.com \
--cc=xiyou.wangcong@gmail.com \
/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).