[Patch net 0/3] net_sched: some fixes for cls_tcindex

[Patch net 0/3] net_sched: some fixes for cls_tcindex

[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

[Patch net 1/3] net_sched: fix a race condition in tcindex_destroy()

[Cong Wang]

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

[Patch net 2/3] net_sched: fix a memory leak in cls_tcindex

[Cong Wang]

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

[Patch net 3/3] net_sched: fix two more memory leaks in cls_tcindex

[Cong Wang]

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

Re: [Patch net 2/3] net_sched: fix a memory leak in cls_tcindex

[Cong Wang]

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!