Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Cong Wang <xiyou.wangcong@gmail.com>
To: netdev@vger.kernel.org
Cc: "Cong Wang" <xiyou.wangcong@gmail.com>,
	"Cameron Berkenpas" <cam@neo-zeon.de>,
	"Peter Geis" <pgwipeout@gmail.com>,
	"Lu Fengqi" <lufq.fnst@cn.fujitsu.com>,
	"Daniël Sonck" <dsonck92@gmail.com>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Zefan Li" <lizefan@huawei.com>, "Tejun Heo" <tj@kernel.org>
Subject: [Patch net] cgroup: fix cgroup_sk_alloc() for sk_clone_lock()
Date: Tue, 16 Jun 2020 11:03:52 -0700
Message-ID: <20200616180352.18602-1-xiyou.wangcong@gmail.com> (raw)

When we clone a socket in sk_clone_lock(), its sk_cgrp_data is
copied, so the cgroup refcnt must be taken too. And, unlike the
sk_alloc() path, sock_update_netprioidx() is not called here.
Therefore, it is safe and necessary to grab the cgroup refcnt
even when cgroup_sk_alloc is disabled.

sk_clone_lock() is in BH context anyway, the in_interrupt()
would terminate this function if called there. And for sk_alloc()
skcd->val is always zero. So it's safe to factor out the code
to make it more readable.

Fixes: 090e28b229af92dc5b ("netprio_cgroup: Fix unlimited memory leak of v2 cgroups")
Reported-by: Cameron Berkenpas <cam@neo-zeon.de>
Reported-by: Peter Geis <pgwipeout@gmail.com>
Reported-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
Reported-by: Daniël Sonck <dsonck92@gmail.com>
Tested-by: Cameron Berkenpas <cam@neo-zeon.de>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Zefan Li <lizefan@huawei.com>
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/cgroup.h |  2 ++
 kernel/cgroup/cgroup.c | 26 ++++++++++++++------------
 net/core/sock.c        |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 4598e4da6b1b..818dc7b3ed6c 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -822,6 +822,7 @@ extern spinlock_t cgroup_sk_update_lock;
 
 void cgroup_sk_alloc_disable(void);
 void cgroup_sk_alloc(struct sock_cgroup_data *skcd);
+void cgroup_sk_clone(struct sock_cgroup_data *skcd);
 void cgroup_sk_free(struct sock_cgroup_data *skcd);
 
 static inline struct cgroup *sock_cgroup_ptr(struct sock_cgroup_data *skcd)
@@ -847,6 +848,7 @@ static inline struct cgroup *sock_cgroup_ptr(struct sock_cgroup_data *skcd)
 #else	/* CONFIG_CGROUP_DATA */
 
 static inline void cgroup_sk_alloc(struct sock_cgroup_data *skcd) {}
+static inline void cgroup_sk_clone(struct sock_cgroup_data *skcd) {}
 static inline void cgroup_sk_free(struct sock_cgroup_data *skcd) {}
 
 #endif	/* CONFIG_CGROUP_DATA */
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 1ea181a58465..6377045b7096 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -6442,18 +6442,6 @@ void cgroup_sk_alloc(struct sock_cgroup_data *skcd)
 	if (cgroup_sk_alloc_disabled)
 		return;
 
-	/* Socket clone path */
-	if (skcd->val) {
-		/*
-		 * We might be cloning a socket which is left in an empty
-		 * cgroup and the cgroup might have already been rmdir'd.
-		 * Don't use cgroup_get_live().
-		 */
-		cgroup_get(sock_cgroup_ptr(skcd));
-		cgroup_bpf_get(sock_cgroup_ptr(skcd));
-		return;
-	}
-
 	/* Don't associate the sock with unrelated interrupted task's cgroup. */
 	if (in_interrupt())
 		return;
@@ -6475,6 +6463,20 @@ void cgroup_sk_alloc(struct sock_cgroup_data *skcd)
 	rcu_read_unlock();
 }
 
+void cgroup_sk_clone(struct sock_cgroup_data *skcd)
+{
+	/* Socket clone path */
+	if (skcd->val) {
+		/*
+		 * We might be cloning a socket which is left in an empty
+		 * cgroup and the cgroup might have already been rmdir'd.
+		 * Don't use cgroup_get_live().
+		 */
+		cgroup_get(sock_cgroup_ptr(skcd));
+		cgroup_bpf_get(sock_cgroup_ptr(skcd));
+	}
+}
+
 void cgroup_sk_free(struct sock_cgroup_data *skcd)
 {
 	struct cgroup *cgrp = sock_cgroup_ptr(skcd);
diff --git a/net/core/sock.c b/net/core/sock.c
index 6c4acf1f0220..b62f06fa5e37 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1925,7 +1925,7 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority)
 		/* sk->sk_memcg will be populated at accept() time */
 		newsk->sk_memcg = NULL;
 
-		cgroup_sk_alloc(&newsk->sk_cgrp_data);
+		cgroup_sk_clone(&newsk->sk_cgrp_data);
 
 		rcu_read_lock();
 		filter = rcu_dereference(sk->sk_filter);
-- 
2.26.2


             reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-16 18:03 Cong Wang [this message]
2020-06-18  1:44 ` Zefan Li
2020-06-18 19:19   ` Cong Wang
2020-06-18 19:36     ` Roman Gushchin
2020-06-18 21:09       ` Cong Wang
2020-06-18 21:26         ` Roman Gushchin
2020-06-18 22:45           ` Peter Geis
2020-06-19  6:40         ` Zefan Li
2020-06-19 19:51           ` Cong Wang
2020-06-20  0:45             ` Zefan Li
2020-06-20  0:51               ` Zefan Li
2020-06-20  3:31                 ` Cong Wang
2020-06-20  7:52                   ` Zefan Li
2020-06-20 16:04                     ` Roman Gushchin
2020-06-23 22:21                   ` Roman Gushchin
2020-06-26  5:23                     ` Cameron Berkenpas
2020-06-26 17:58                       ` Cong Wang
2020-06-26 22:03                         ` Cameron Berkenpas
2020-06-27 22:59                           ` Cameron Berkenpas
2020-06-30 22:16                             ` Cong Wang
2020-06-27 23:41                         ` Roman Gushchin
2020-06-30 22:22                           ` Cong Wang
2020-06-30 22:48                             ` Roman Gushchin
2020-07-01  1:18                               ` Zefan Li
2020-07-02  4:48                               ` Cong Wang
2020-07-02  8:12                                 ` Thomas Lamprecht
2020-07-02 16:02                                 ` Roman Gushchin
2020-07-02 16:24                                   ` Peter Geis
2020-07-03  1:17                                   ` Zefan Li
2020-06-20  0:51           ` Roman Gushchin
2020-06-20  1:00             ` Zefan Li
2020-06-20  1:14               ` Roman Gushchin
2020-06-20  2:48                 ` Zefan Li
2020-06-20  3:00                 ` Cong Wang
2020-06-20 15:57                   ` Roman Gushchin
2020-06-22 18:14                     ` Cong Wang
2020-06-22 20:39                       ` Roman Gushchin
2020-06-23  8:45                         ` Zhang,Qiang
2020-06-23 17:56                           ` Cong Wang
2020-06-23  8:54                         ` Zhang,Qiang
2020-06-23  9:01                         ` Zhang,Qiang

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=20200616180352.18602-1-xiyou.wangcong@gmail.com \
    --to=xiyou.wangcong@gmail.com \
    --cc=cam@neo-zeon.de \
    --cc=daniel@iogearbox.net \
    --cc=dsonck92@gmail.com \
    --cc=lizefan@huawei.com \
    --cc=lufq.fnst@cn.fujitsu.com \
    --cc=netdev@vger.kernel.org \
    --cc=pgwipeout@gmail.com \
    --cc=tj@kernel.org \
    /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

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git