Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 763dbf63 authored by WANG Cong's avatar WANG Cong Committed by David S. Miller
Browse files

net_sched: move the empty tp check from ->destroy() to ->delete() 



We could have a race condition where in ->classify() path we
dereference tp->root and meanwhile a parallel ->destroy() makes it
a NULL. Daniel cured this bug in commit d9363774
("net, sched: respect rcu grace period on cls destruction").

This happens when ->destroy() is called for deleting a filter to
check if we are the last one in tp, this tp is still linked and
visible at that time. The root cause of this problem is the semantic
of ->destroy(), it does two things (for non-force case):

1) check if tp is empty
2) if tp is empty we could really destroy it

and its caller, if cares, needs to check its return value to see if it
is really destroyed. Therefore we can't unlink tp unless we know it is
empty.

As suggested by Daniel, we could actually move the test logic to ->delete()
so that we can safely unlink tp after ->delete() tells us the last one is
just deleted and before ->destroy().

Fixes: 1e052be6 ("net_sched: destroy proto tp when all filters are gone")
Cc: Roi Dayan <roid@mellanox.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: default avatarCong Wang <xiyou.wangcong@gmail.com>
Acked-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent b1d9fc41

include/net/sch_generic.h

+2 −2
Original line number Diff line number Diff line
@@ -204,14 +204,14 @@ struct tcf_proto_ops { 
					    const struct tcf_proto *,

					    struct tcf_result *);

	int			(*init)(struct tcf_proto*);

	bool			(*destroy)(struct tcf_proto*, bool);

	void			(*destroy)(struct tcf_proto*);



	unsigned long		(*get)(struct tcf_proto*, u32 handle);

	int			(*change)(struct net *net, struct sk_buff *,

					struct tcf_proto*, unsigned long,

					u32 handle, struct nlattr **,

					unsigned long *, bool);

	int			(*delete)(struct tcf_proto*, unsigned long);

	int			(*delete)(struct tcf_proto*, unsigned long, bool*);

	void			(*walk)(struct tcf_proto*, struct tcf_walker *arg);



	/* rtnetlink specific */

net/sched/cls_api.c

+14 −13
Original line number Diff line number Diff line
@@ -178,14 +178,11 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol, 
	return ERR_PTR(err);

}



static bool tcf_proto_destroy(struct tcf_proto *tp, bool force)

static void tcf_proto_destroy(struct tcf_proto *tp)

{

	if (tp->ops->destroy(tp, force)) {

	tp->ops->destroy(tp);

	module_put(tp->ops->owner);

	kfree_rcu(tp, rcu);

		return true;

	}

	return false;

}



void tcf_destroy_chain(struct tcf_proto __rcu **fl)
@@ -194,7 +191,7 @@ void tcf_destroy_chain(struct tcf_proto __rcu **fl) 


	while ((tp = rtnl_dereference(*fl)) != NULL) {

		RCU_INIT_POINTER(*fl, tp->next);

		tcf_proto_destroy(tp, true);

		tcf_proto_destroy(tp);

	}

}

EXPORT_SYMBOL(tcf_destroy_chain);
@@ -361,7 +358,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, 
			RCU_INIT_POINTER(*back, next);

			tfilter_notify(net, skb, n, tp, fh,

				       RTM_DELTFILTER, false);

			tcf_proto_destroy(tp, true);

			tcf_proto_destroy(tp);

			err = 0;

			goto errout;

		}
@@ -372,24 +369,28 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, 
			goto errout;

		}

	} else {

		bool last;



		switch (n->nlmsg_type) {

		case RTM_NEWTFILTER:

			if (n->nlmsg_flags & NLM_F_EXCL) {

				if (tp_created)

					tcf_proto_destroy(tp, true);

					tcf_proto_destroy(tp);

				err = -EEXIST;

				goto errout;

			}

			break;

		case RTM_DELTFILTER:

			err = tp->ops->delete(tp, fh);

			err = tp->ops->delete(tp, fh, &last);

			if (err)

				goto errout;

			next = rtnl_dereference(tp->next);

			tfilter_notify(net, skb, n, tp, t->tcm_handle,

				       RTM_DELTFILTER, false);

			if (tcf_proto_destroy(tp, false))

			if (last) {

				RCU_INIT_POINTER(*back, next);

				tcf_proto_destroy(tp);

			}

			goto errout;

		case RTM_GETTFILTER:

			err = tfilter_notify(net, skb, n, tp, fh,
@@ -411,7 +412,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, 
		tfilter_notify(net, skb, n, tp, fh, RTM_NEWTFILTER, false);

	} else {

		if (tp_created)

			tcf_proto_destroy(tp, true);

			tcf_proto_destroy(tp);

	}



errout:

net/sched/cls_basic.c

+4 −6
Original line number Diff line number Diff line
@@ -93,30 +93,28 @@ static void basic_delete_filter(struct rcu_head *head) 
	kfree(f);

}



static bool basic_destroy(struct tcf_proto *tp, bool force)

static void basic_destroy(struct tcf_proto *tp)

{

	struct basic_head *head = rtnl_dereference(tp->root);

	struct basic_filter *f, *n;



	if (!force && !list_empty(&head->flist))

		return false;



	list_for_each_entry_safe(f, n, &head->flist, link) {

		list_del_rcu(&f->link);

		tcf_unbind_filter(tp, &f->res);

		call_rcu(&f->rcu, basic_delete_filter);

	}

	kfree_rcu(head, rcu);

	return true;

}



static int basic_delete(struct tcf_proto *tp, unsigned long arg)

static int basic_delete(struct tcf_proto *tp, unsigned long arg, bool *last)

{

	struct basic_head *head = rtnl_dereference(tp->root);

	struct basic_filter *f = (struct basic_filter *) arg;



	list_del_rcu(&f->link);

	tcf_unbind_filter(tp, &f->res);

	call_rcu(&f->rcu, basic_delete_filter);

	*last = list_empty(&head->flist);

	return 0;

}

net/sched/cls_bpf.c

+5 −6
Original line number Diff line number Diff line
@@ -274,25 +274,24 @@ static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog) 
	call_rcu(&prog->rcu, cls_bpf_delete_prog_rcu);

}



static int cls_bpf_delete(struct tcf_proto *tp, unsigned long arg)

static int cls_bpf_delete(struct tcf_proto *tp, unsigned long arg, bool *last)

{

	struct cls_bpf_head *head = rtnl_dereference(tp->root);



	__cls_bpf_delete(tp, (struct cls_bpf_prog *) arg);

	*last = list_empty(&head->plist);

	return 0;

}



static bool cls_bpf_destroy(struct tcf_proto *tp, bool force)

static void cls_bpf_destroy(struct tcf_proto *tp)

{

	struct cls_bpf_head *head = rtnl_dereference(tp->root);

	struct cls_bpf_prog *prog, *tmp;



	if (!force && !list_empty(&head->plist))

		return false;



	list_for_each_entry_safe(prog, tmp, &head->plist, link)

		__cls_bpf_delete(tp, prog);



	kfree_rcu(head, rcu);

	return true;

}



static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)

net/sched/cls_cgroup.c

+2 −6
Original line number Diff line number Diff line
@@ -131,20 +131,16 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb, 
	return err;

}



static bool cls_cgroup_destroy(struct tcf_proto *tp, bool force)

static void cls_cgroup_destroy(struct tcf_proto *tp)

{

	struct cls_cgroup_head *head = rtnl_dereference(tp->root);



	if (!force)

		return false;

	/* Head can still be NULL due to cls_cgroup_init(). */

	if (head)

		call_rcu(&head->rcu, cls_cgroup_destroy_rcu);



	return true;

}



static int cls_cgroup_delete(struct tcf_proto *tp, unsigned long arg)

static int cls_cgroup_delete(struct tcf_proto *tp, unsigned long arg, bool *last)

{

	return -EOPNOTSUPP;

}