Netdev Archive on lore.kernel.org
 help / Atom feed
* [Patch net 0/3] net_sched: some fixes for cls_tcindex
@ 2019-02-11  1:47 Cong Wang
  2019-02-11  1:47 ` [Patch net 1/3] net_sched: fix a race condition in tcindex_destroy() Cong Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Cong Wang @ 2019-02-11  1:47 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang

This patchset contains 3 bug fixes for tcindex filter. Please check
each patch for details.

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Cong Wang (3):
  net_sched: fix a race condition in tcindex_destroy()
  net_sched: fix a memory leak in cls_tcindex
  net_sched: fix two more memory leaks in cls_tcindex

 net/sched/cls_tcindex.c | 104 +++++++++++++++++++++++++++-------------
 1 file changed, 70 insertions(+), 34 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Patch net 1/3] net_sched: fix a race condition in tcindex_destroy()
  2019-02-11  1:47 [Patch net 0/3] net_sched: some fixes for cls_tcindex Cong Wang
@ 2019-02-11  1:47 ` Cong Wang
  2019-02-11  1:47 ` [Patch net 2/3] net_sched: fix a memory leak in cls_tcindex Cong Wang
  2019-02-11  1:47 ` [Patch net 3/3] net_sched: fix two more memory leaks " Cong Wang
  2 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2019-02-11  1:47 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Adrian, Ben Hutchings, Jamal Hadi Salim, Jiri Pirko

tcindex_destroy() invokes tcindex_destroy_element() via
a walker to delete each filter result in its perfect hash
table, and tcindex_destroy_element() calls tcindex_delete()
which schedules tcf RCU works to do the final deletion work.
Unfortunately this races with the RCU callback
__tcindex_destroy(), which could lead to use-after-free as
reported by Adrian.

Fix this by migrating this RCU callback to tcf RCU work too,
as that workqueue is ordered, we will not have use-after-free.

This change requires us to store a net pointer inside struct
tcindex_data, to avoid the known race with tc_action_net_exit().

Fixes: 27ce4f05e2ab ("net_sched: use tcf_queue_work() in tcindex filter")
Reported-by: Adrian <bugs@abtelecom.ro>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 46 ++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 9ccc93f257db..14e6d80dd58e 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -48,7 +48,8 @@ struct tcindex_data {
 	u32 hash;		/* hash table size; 0 if undefined */
 	u32 alloc_hash;		/* allocated size */
 	u32 fall_through;	/* 0: only classify if explicit match */
-	struct rcu_head rcu;
+	struct net *net;
+	struct rcu_work rwork;
 };
 
 static inline int tcindex_filter_is_set(struct tcindex_filter_result *r)
@@ -229,15 +230,23 @@ static int tcindex_destroy_element(struct tcf_proto *tp,
 	return tcindex_delete(tp, arg, &last, NULL);
 }
 
-static void __tcindex_destroy(struct rcu_head *head)
+static void __tcindex_destroy(struct tcindex_data *p)
 {
-	struct tcindex_data *p = container_of(head, struct tcindex_data, rcu);
-
 	kfree(p->perfect);
 	kfree(p->h);
 	kfree(p);
 }
 
+static void tcindex_destroy_work(struct work_struct *work)
+{
+	struct tcindex_data *p = container_of(to_rcu_work(work),
+					      struct tcindex_data,
+					      rwork);
+
+	put_net(p->net);
+	__tcindex_destroy(p);
+}
+
 static inline int
 valid_perfect_hash(struct tcindex_data *p)
 {
@@ -258,14 +267,22 @@ static int tcindex_filter_result_init(struct tcindex_filter_result *r)
 	return tcf_exts_init(&r->exts, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
 }
 
-static void __tcindex_partial_destroy(struct rcu_head *head)
+static void __tcindex_partial_destroy(struct tcindex_data *p)
 {
-	struct tcindex_data *p = container_of(head, struct tcindex_data, rcu);
-
 	kfree(p->perfect);
 	kfree(p);
 }
 
+static void tcindex_partial_destroy_work(struct work_struct *work)
+{
+	struct tcindex_data *p = container_of(to_rcu_work(work),
+					      struct tcindex_data,
+					      rwork);
+
+	put_net(p->net);
+	__tcindex_partial_destroy(p);
+}
+
 static void tcindex_free_perfect_hash(struct tcindex_data *cp)
 {
 	int i;
@@ -333,6 +350,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	cp->alloc_hash = p->alloc_hash;
 	cp->fall_through = p->fall_through;
 	cp->tp = tp;
+	cp->net = net;
 
 	if (p->perfect) {
 		int i;
@@ -477,8 +495,13 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 		rcu_assign_pointer(*fp, f);
 	}
 
-	if (oldp)
-		call_rcu(&oldp->rcu, __tcindex_partial_destroy);
+	if (oldp) {
+		if (oldp->net && maybe_get_net(oldp->net))
+			tcf_queue_work(&oldp->rwork,
+				       tcindex_partial_destroy_work);
+		else
+			__tcindex_partial_destroy(oldp);
+	}
 	return 0;
 
 errout_alloc:
@@ -570,7 +593,10 @@ static void tcindex_destroy(struct tcf_proto *tp,
 	walker.fn = tcindex_destroy_element;
 	tcindex_walk(tp, &walker);
 
-	call_rcu(&p->rcu, __tcindex_destroy);
+	if (maybe_get_net(p->net))
+		tcf_queue_work(&p->rwork, tcindex_destroy_work);
+	else
+		__tcindex_destroy(p);
 }
 
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Patch net 2/3] net_sched: fix a memory leak in cls_tcindex
  2019-02-11  1:47 [Patch net 0/3] net_sched: some fixes for cls_tcindex Cong Wang
  2019-02-11  1:47 ` [Patch net 1/3] net_sched: fix a race condition in tcindex_destroy() Cong Wang
@ 2019-02-11  1:47 ` Cong Wang
       [not found]   ` <201902111051.xRYMeLJl%fengguang.wu@intel.com>
  2019-02-11  1:47 ` [Patch net 3/3] net_sched: fix two more memory leaks " Cong Wang
  2 siblings, 1 reply; 5+ messages in thread
From: Cong Wang @ 2019-02-11  1:47 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko

When tcindex_destroy() destroys all the filter results in
the perfect hash table, it invokes the walker to delete
each of them. However, results with class==0 are skipped
in either tcindex_walk() or tcindex_delete(), which causes
a memory leak reported by kmemleak.

This patch fixes it by skipping the walker and directly
deleting these filter results so we don't miss any filter
result.

As a result of this change, we have to initialize exts->net
properly in tcindex_alloc_perfect_hash(). In net-next, we
need to evaluate whether we should initialize ->net in
tcf_exts_init() instead.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 44 ++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 14e6d80dd58e..6c3436e8436c 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -222,14 +222,6 @@ static int tcindex_delete(struct tcf_proto *tp, void *arg, bool *last,
 	return 0;
 }
 
-static int tcindex_destroy_element(struct tcf_proto *tp,
-				   void *arg, struct tcf_walker *walker)
-{
-	bool last;
-
-	return tcindex_delete(tp, arg, &last, NULL);
-}
-
 static void __tcindex_destroy(struct tcindex_data *p)
 {
 	kfree(p->perfect);
@@ -292,7 +284,7 @@ static void tcindex_free_perfect_hash(struct tcindex_data *cp)
 	kfree(cp->perfect);
 }
 
-static int tcindex_alloc_perfect_hash(struct tcindex_data *cp)
+static int tcindex_alloc_perfect_hash(struct net *net, struct tcindex_data *cp)
 {
 	int i, err = 0;
 
@@ -306,6 +298,7 @@ static int tcindex_alloc_perfect_hash(struct tcindex_data *cp)
 				    TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
 		if (err < 0)
 			goto errout;
+		cp->perfect[i].exts.net = net;
 	}
 
 	return 0;
@@ -355,7 +348,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	if (p->perfect) {
 		int i;
 
-		if (tcindex_alloc_perfect_hash(cp) < 0)
+		if (tcindex_alloc_perfect_hash(net, cp) < 0)
 			goto errout;
 		for (i = 0; i < cp->hash; i++)
 			cp->perfect[i].res = p->perfect[i].res;
@@ -424,7 +417,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	err = -ENOMEM;
 	if (!cp->perfect && !cp->h) {
 		if (valid_perfect_hash(cp)) {
-			if (tcindex_alloc_perfect_hash(cp) < 0)
+			if (tcindex_alloc_perfect_hash(net, cp) < 0)
 				goto errout_alloc;
 			balloc = 1;
 		} else {
@@ -585,13 +578,32 @@ static void tcindex_destroy(struct tcf_proto *tp,
 			    struct netlink_ext_ack *extack)
 {
 	struct tcindex_data *p = rtnl_dereference(tp->root);
-	struct tcf_walker walker;
+	int i;
 
 	pr_debug("tcindex_destroy(tp %p),p %p\n", tp, p);
-	walker.count = 0;
-	walker.skip = 0;
-	walker.fn = tcindex_destroy_element;
-	tcindex_walk(tp, &walker);
+
+	if (p->perfect) {
+		for (i = 0; i < p->hash; i++) {
+			struct tcindex_filter_result *r = p->perfect + i;
+
+			tcf_unbind_filter(tp, &r->res);
+			if (tcf_exts_get_net(&r->exts))
+				tcf_queue_work(&r->rwork,
+					       tcindex_destroy_rexts_work);
+			else
+				__tcindex_destroy_rexts(r);
+		}
+	}
+
+	for (i = 0; p->h && i < p->hash; i++) {
+		struct tcindex_filter *f, *next;
+		bool last;
+
+		for (f = rtnl_dereference(p->h[i]); f; f = next) {
+			next = rtnl_dereference(f->next);
+			tcindex_delete(tp, &f->result, &last, NULL);
+		}
+	}
 
 	if (maybe_get_net(p->net))
 		tcf_queue_work(&p->rwork, tcindex_destroy_work);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [Patch net 3/3] net_sched: fix two more memory leaks in cls_tcindex
  2019-02-11  1:47 [Patch net 0/3] net_sched: some fixes for cls_tcindex Cong Wang
  2019-02-11  1:47 ` [Patch net 1/3] net_sched: fix a race condition in tcindex_destroy() Cong Wang
  2019-02-11  1:47 ` [Patch net 2/3] net_sched: fix a memory leak in cls_tcindex Cong Wang
@ 2019-02-11  1:47 ` " Cong Wang
  2 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2019-02-11  1:47 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko

struct tcindex_filter_result contains two parts:
struct tcf_exts and struct tcf_result.

For the local variable 'cr', its exts part is never used but
initialized without being released properly on success path. So
just completely remove the exts part to fix this leak.

For the local variable 'new_filter_result', it is never properly
released if not used by 'r' on success path.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 6c3436e8436c..297cb6e98c44 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -315,9 +315,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 		  struct nlattr *est, bool ovr, struct netlink_ext_ack *extack)
 {
 	struct tcindex_filter_result new_filter_result, *old_r = r;
-	struct tcindex_filter_result cr;
 	struct tcindex_data *cp = NULL, *oldp;
 	struct tcindex_filter *f = NULL; /* make gcc behave */
+	struct tcf_result cr = {};
 	int err, balloc = 0;
 	struct tcf_exts e;
 
@@ -357,13 +357,10 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	cp->h = p->h;
 
 	err = tcindex_filter_result_init(&new_filter_result);
-	if (err < 0)
-		goto errout1;
-	err = tcindex_filter_result_init(&cr);
 	if (err < 0)
 		goto errout1;
 	if (old_r)
-		cr.res = r->res;
+		cr = r->res;
 
 	if (tb[TCA_TCINDEX_HASH])
 		cp->hash = nla_get_u32(tb[TCA_TCINDEX_HASH]);
@@ -454,8 +451,8 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	}
 
 	if (tb[TCA_TCINDEX_CLASSID]) {
-		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
-		tcf_bind_filter(tp, &cr.res, base);
+		cr.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
+		tcf_bind_filter(tp, &cr, base);
 	}
 
 	if (old_r && old_r != r) {
@@ -467,7 +464,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	}
 
 	oldp = p;
-	r->res = cr.res;
+	r->res = cr;
 	tcf_exts_change(&r->exts, &e);
 
 	rcu_assign_pointer(tp->root, cp);
@@ -486,6 +483,8 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 				; /* nothing */
 
 		rcu_assign_pointer(*fp, f);
+	} else {
+		tcf_exts_destroy(&new_filter_result.exts);
 	}
 
 	if (oldp) {
@@ -503,7 +502,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	else if (balloc == 2)
 		kfree(cp->h);
 errout1:
-	tcf_exts_destroy(&cr.exts);
 	tcf_exts_destroy(&new_filter_result.exts);
 errout:
 	kfree(cp);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Patch net 2/3] net_sched: fix a memory leak in cls_tcindex
       [not found]   ` <201902111051.xRYMeLJl%fengguang.wu@intel.com>
@ 2019-02-11  7:19     ` Cong Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Cong Wang @ 2019-02-11  7:19 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, Linux Kernel Network Developers, Jamal Hadi Salim,
	Jiri Pirko

On Sun, Feb 10, 2019 at 6:15 PM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Cong,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on net/master]
>
> url:    https://github.com/0day-ci/linux/commits/Cong-Wang/net_sched-some-fixes-for-cls_tcindex/20190211-095057
> config: i386-randconfig-x002-201906 (attached as .config)
> compiler: gcc-8 (Debian 8.2.0-14) 8.2.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
>    net/sched/cls_tcindex.c: In function 'tcindex_alloc_perfect_hash':
> >> net/sched/cls_tcindex.c:301:22: error: 'struct tcf_exts' has no member named 'net'
>       cp->perfect[i].exts.net = net;

Yeah, looks like I missed the CONFIG_CLS_ACT=n case.

Let me think about how to fix it properly.

Thanks!

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11  1:47 [Patch net 0/3] net_sched: some fixes for cls_tcindex Cong Wang
2019-02-11  1:47 ` [Patch net 1/3] net_sched: fix a race condition in tcindex_destroy() Cong Wang
2019-02-11  1:47 ` [Patch net 2/3] net_sched: fix a memory leak in cls_tcindex Cong Wang
     [not found]   ` <201902111051.xRYMeLJl%fengguang.wu@intel.com>
2019-02-11  7:19     ` Cong Wang
2019-02-11  1:47 ` [Patch net 3/3] net_sched: fix two more memory leaks " Cong Wang

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 netdev@archiver.kernel.org
	public-inbox-index netdev


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