[PATCH net v2 0/5] net: sched: Undo tcf_bind_filter in case of errors in set callbacks

[PATCH net v2 0/5] net: sched: Undo tcf_bind_filter in case of errors in set callbacks

[Victor Nogueira]

Five different classifier (fw, bpf, u32, matchall, and flower) are
calling tcf_bind_filter in their callbacks, but weren't undoing it by
calling tcf_unbind_filter if their was an error after binding.

This patch set fixes all this by calling tcf_unbind_filter in such
cases.

This set also undoes a refcount decrement in cls_u32 when an update
fails under specific conditions which are described in patch #4.

v1 -> v2:
* Remove blank line after fixes tag
* Fix reverse xmas tree issues pointed out by Simon

Victor Nogueira (5):
  net: sched: cls_bpf: Undo tcf_bind_filter in case of an error
  net: sched: cls_matchall: Undo tcf_bind_filter in case of failure
    after mall_set_parms
  net: sched: cls_u32: Undo tcf_bind_filter if u32_replace_hw_knode
  net: sched: cls_u32: Undo refcount decrement in case update failed
  net: sched: cls_flower: Undo tcf_bind_filter if fl_set_key fails

 net/sched/cls_bpf.c      |  8 ++++++--
 net/sched/cls_flower.c   | 29 +++++++++++++++++++++++++----
 net/sched/cls_matchall.c |  8 ++++++--
 net/sched/cls_u32.c      | 32 ++++++++++++++++++++++++++------
 4 files changed, 63 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH net v2 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Victor Nogueira]

If cls_bpf_offload errors out, we must also undo tcf_bind_filter that
was done in cls_bpf_set_parms.

Fix that by calling tcf_unbind_filter in errout_parms.

Fixes: eadb41489fd2 ("net: cls_bpf: add support for marking filters as hardware-only")
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_bpf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 466c26df853a..c45321fb3a5e 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -409,7 +409,7 @@ static int cls_bpf_prog_from_efd(struct nlattr **tb, struct cls_bpf_prog *prog,
 static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 			     struct cls_bpf_prog *prog, unsigned long base,
 			     struct nlattr **tb, struct nlattr *est, u32 flags,
-			     struct netlink_ext_ack *extack)
+			     bool *bound_to_filter, struct netlink_ext_ack *extack)
 {
 	bool is_bpf, is_ebpf, have_exts = false;
 	u32 gen_flags = 0;
@@ -451,6 +451,7 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
 	if (tb[TCA_BPF_CLASSID]) {
 		prog->res.classid = nla_get_u32(tb[TCA_BPF_CLASSID]);
 		tcf_bind_filter(tp, &prog->res, base);
+		*bound_to_filter = true;
 	}
 
 	return 0;
@@ -465,6 +466,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	struct cls_bpf_head *head = rtnl_dereference(tp->root);
 	struct cls_bpf_prog *oldprog = *arg;
 	struct nlattr *tb[TCA_BPF_MAX + 1];
+	bool bound_to_filter = false;
 	struct cls_bpf_prog *prog;
 	int ret;
 
@@ -505,7 +507,7 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	prog->handle = handle;
 
 	ret = cls_bpf_set_parms(net, tp, prog, base, tb, tca[TCA_RATE], flags,
-				extack);
+				&bound_to_filter, extack);
 	if (ret < 0)
 		goto errout_idr;
 
@@ -530,6 +532,8 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
 	return 0;
 
 errout_parms:
+	if (bound_to_filter)
+		tcf_unbind_filter(tp, &prog->res);
 	cls_bpf_free_parms(prog);
 errout_idr:
 	if (!oldprog)
-- 
2.25.1

[PATCH net v2 2/5] net: sched: cls_matchall: Undo tcf_bind_filter in case of failure after mall_set_parms

[Victor Nogueira]

In case an error occurred after mall_set_parms executed successfully, we
must undo the tcf_bind_filter call it issues.

Fix that by calling tcf_unbind_filter in err_replace_hw_filter label.

Fixes: ec2507d2a306 ("net/sched: cls_matchall: Fix error path")
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_matchall.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index fa3bbd187eb9..e4b649669835 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -163,7 +163,7 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
 			  struct cls_mall_head *head,
 			  unsigned long base, struct nlattr **tb,
 			  struct nlattr *est, u32 flags, u32 fl_flags,
-			  struct netlink_ext_ack *extack)
+			  bool *bound_to_filter, struct netlink_ext_ack *extack)
 {
 	int err;
 
@@ -175,6 +175,7 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
 	if (tb[TCA_MATCHALL_CLASSID]) {
 		head->res.classid = nla_get_u32(tb[TCA_MATCHALL_CLASSID]);
 		tcf_bind_filter(tp, &head->res, base);
+		*bound_to_filter = true;
 	}
 	return 0;
 }
@@ -187,6 +188,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 {
 	struct cls_mall_head *head = rtnl_dereference(tp->root);
 	struct nlattr *tb[TCA_MATCHALL_MAX + 1];
+	bool bound_to_filter = false;
 	struct cls_mall_head *new;
 	u32 userflags = 0;
 	int err;
@@ -227,7 +229,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	}
 
 	err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE],
-			     flags, new->flags, extack);
+			     flags, new->flags, &bound_to_filter, extack);
 	if (err)
 		goto err_set_parms;
 
@@ -246,6 +248,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 	return 0;
 
 err_replace_hw_filter:
+	if (bound_to_filter)
+		tcf_unbind_filter(tp, &new->res);
 err_set_parms:
 	free_percpu(new->pf);
 err_alloc_percpu:
-- 
2.25.1

[PATCH net v2 3/5] net: sched: cls_u32: Undo tcf_bind_filter if u32_replace_hw_knode

[Victor Nogueira]

When u32_replace_hw_knode fails, we need to undo the tcf_bind_filter
operation done at u32_set_parms.

Fixes: d34e3e181395 ("net: cls_u32: Add support for skip-sw flag to tc u32 classifier.")
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index d15d50de7980..7e32c018941f 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -712,11 +712,13 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 	[TCA_U32_FLAGS]		= { .type = NLA_U32 },
 };
 
+#define U32_SET_FLAGS_BOUND 0x1
+
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 			 unsigned long base,
 			 struct tc_u_knode *n, struct nlattr **tb,
 			 struct nlattr *est, u32 flags, u32 fl_flags,
-			 struct netlink_ext_ack *extack)
+			 u8 *set_flags, struct netlink_ext_ack *extack)
 {
 	int err, ifindex = -1;
 
@@ -763,6 +765,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 	if (tb[TCA_U32_CLASSID]) {
 		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
 		tcf_bind_filter(tp, &n->res, base);
+		*set_flags |= U32_SET_FLAGS_BOUND;
 	}
 
 	if (ifindex >= 0)
@@ -859,6 +862,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	struct nlattr *opt = tca[TCA_OPTIONS];
 	struct nlattr *tb[TCA_U32_MAX + 1];
 	u32 htid, userflags = 0;
+	u8 set_flags = 0;
 	size_t sel_size;
 	int err;
 
@@ -905,7 +909,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_set_parms(net, tp, base, new, tb,
 				    tca[TCA_RATE], flags, new->flags,
-				    extack);
+				    &set_flags, extack);
 
 		if (err) {
 			__u32_destroy_key(new);
@@ -914,6 +918,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 
 		err = u32_replace_hw_knode(tp, new, flags, extack);
 		if (err) {
+			if (set_flags & U32_SET_FLAGS_BOUND)
+				tcf_unbind_filter(tp, &new->res);
+
 			__u32_destroy_key(new);
 			return err;
 		}
@@ -1075,14 +1082,14 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 #endif
 
 	err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE],
-			    flags, n->flags, extack);
+			    flags, n->flags, &set_flags, extack);
 	if (err == 0) {
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
 
 		err = u32_replace_hw_knode(tp, n, flags, extack);
 		if (err)
-			goto errhw;
+			goto errunbind;
 
 		if (!tc_in_hw(n->flags))
 			n->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
@@ -1100,7 +1107,10 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		return 0;
 	}
 
-errhw:
+errunbind:
+	if (set_flags & U32_SET_FLAGS_BOUND)
+		tcf_unbind_filter(tp, &n->res);
+
 #ifdef CONFIG_CLS_U32_MARK
 	free_percpu(n->pcpu_success);
 #endif
-- 
2.25.1

[PATCH net v2 4/5] net: sched: cls_u32: Undo refcount decrement in case update failed

[Victor Nogueira]

In the case of an update, when TCA_U32_LINK is set, u32_set_parms will
decrement the refcount of the ht_down (struct tc_u_hnode) pointer
present in the older u32 filter which we are replacing. However, if
u32_replace_hw_knode errors out, the update command fails and that
ht_down pointer continues decremented. To fix that, when
u32_replace_hw_knode fails, check if ht_down's refcount was decremented
and undo the decrement.

Fixes: d34e3e181395 ("net: cls_u32: Add support for skip-sw flag to tc u32 classifier.")
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_u32.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 7e32c018941f..12e78f81a259 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -713,6 +713,7 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 };
 
 #define U32_SET_FLAGS_BOUND 0x1
+#define U32_SET_FLAGS_DECR_HTDOWN 0x2
 
 static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 			 unsigned long base,
@@ -759,8 +760,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 		ht_old = rtnl_dereference(n->ht_down);
 		rcu_assign_pointer(n->ht_down, ht_down);
 
-		if (ht_old)
+		if (ht_old) {
+			*set_flags |= U32_SET_FLAGS_DECR_HTDOWN;
 			ht_old->refcnt--;
+		}
 	}
 	if (tb[TCA_U32_CLASSID]) {
 		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
@@ -921,6 +924,13 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			if (set_flags & U32_SET_FLAGS_BOUND)
 				tcf_unbind_filter(tp, &new->res);
 
+			if (set_flags & U32_SET_FLAGS_DECR_HTDOWN) {
+				struct tc_u_hnode *ht_old;
+
+				ht_old = rtnl_dereference(n->ht_down);
+				if (ht_old)
+					ht_old->refcnt++;
+			}
 			__u32_destroy_key(new);
 			return err;
 		}
-- 
2.25.1

[PATCH net v2 5/5] net: sched: cls_flower: Undo tcf_bind_filter if fl_set_key fails

[Victor Nogueira]

if TCA_FLOWER_CLASSID is specified in the netlink message, the code will
call tcf_bind_filter. However, if any error occurs after that, the code
should undo this by calling tcf_unbind_filter.

When checking for TCA_FLOWER_CLASSID attribute, the code is calling for
tcf_bind_fitler.

Fixes: 77b9900ef53a ("tc: introduce Flower classifier")
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_flower.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 56065cc5a661..644b0097e6ae 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2169,7 +2169,7 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 			struct nlattr *est,
 			struct fl_flow_tmplt *tmplt,
 			u32 flags, u32 fl_flags,
-			struct netlink_ext_ack *extack)
+			bool *bound_to_filter, struct netlink_ext_ack *extack)
 {
 	int err;
 
@@ -2185,18 +2185,20 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 		tcf_bind_filter(tp, &f->res, base);
 		if (flags & TCA_ACT_FLAGS_NO_RTNL)
 			rtnl_unlock();
+		*bound_to_filter = true;
 	}
 
 	err = fl_set_key(net, tb, &f->key, &mask->key, extack);
 	if (err)
-		return err;
+		goto unbind_filter;
 
 	fl_mask_update_range(mask);
 	fl_set_masked_key(&f->mkey, &f->key, mask);
 
 	if (!fl_mask_fits_tmplt(tmplt, mask)) {
 		NL_SET_ERR_MSG_MOD(extack, "Mask does not fit the template");
-		return -EINVAL;
+		err = -EINVAL;
+		goto unbind_filter;
 	}
 
 	/* Enable tc skb extension if filter matches on data extracted from
@@ -2208,6 +2210,17 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
 	}
 
 	return 0;
+
+unbind_filter:
+	if (*bound_to_filter) {
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_lock();
+		tcf_unbind_filter(tp, &f->res);
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_unlock();
+		*bound_to_filter = false;
+	}
+	return err;
 }
 
 static int fl_ht_insert_unique(struct cls_fl_filter *fnew,
@@ -2241,6 +2254,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 	struct cls_fl_head *head = fl_head_dereference(tp);
 	bool rtnl_held = !(flags & TCA_ACT_FLAGS_NO_RTNL);
 	struct cls_fl_filter *fold = *arg;
+	bool bound_to_filter = false;
 	struct cls_fl_filter *fnew;
 	struct fl_flow_mask *mask;
 	struct nlattr **tb;
@@ -2327,7 +2341,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 
 	err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE],
 			   tp->chain->tmplt_priv, flags, fnew->flags,
-			   extack);
+			   &bound_to_filter, extack);
 	if (err)
 		goto errout_idr;
 
@@ -2425,6 +2439,13 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 errout_mask:
 	fl_mask_put(head, fnew->mask);
 errout_idr:
+	if (bound_to_filter) {
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_lock();
+		tcf_unbind_filter(tp, &fnew->res);
+		if (flags & TCA_ACT_FLAGS_NO_RTNL)
+			rtnl_unlock();
+	}
 	if (!fold)
 		idr_remove(&head->handle_idr, fnew->handle);
 	__fl_put(fnew);
-- 
2.25.1

Re: [PATCH net v2 1/5] net: sched: cls_bpf: Undo tcf_bind_filter in case of an error

[Jakub Kicinski]

On Wed,  5 Jul 2023 10:43:25 -0300 Victor Nogueira wrote:
>  static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
>  			     struct cls_bpf_prog *prog, unsigned long base,
>  			     struct nlattr **tb, struct nlattr *est, u32 flags,
> -			     struct netlink_ext_ack *extack)
> +			     bool *bound_to_filter, struct netlink_ext_ack *extack)

Output argument, and an 8th argument of a function at that is too ugly.
Please find a better way to fix this.
-- 
pw-bot: cr