[PATCH net 0/3] net/sched: fix action bind logic

[PATCH net 0/3] net/sched: fix action bind logic

[Pedro Tammela]

Some actions are not handling the case where an action can be created and bound to a
filter independently. These actions are checking for parameters only passed
in the netlink message for create/change/replace, which then errors out
for valid uses like:
tc filter ... action pedit index 1

In the iproute2 side, we saw a couple of actions with their parsers
broken when passing "index 1" as the only action argument, while the kernel
side accepted it correctly. We fixed those as well.

Pedro Tammela (3):
  net/sched: act_pedit: fix action bind logic
  net/sched: act_mpls: fix action bind logic
  net/sched: act_sample: fix action bind logic

 net/sched/act_mpls.c   | 66 +++++++++++++++++++++++-------------------
 net/sched/act_pedit.c  | 58 ++++++++++++++++++++-----------------
 net/sched/act_sample.c | 11 +++++--
 3 files changed, 77 insertions(+), 58 deletions(-)

-- 
2.34.1

[PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Pedro Tammela]

The TC architecture allows filters and actions to be created independently.
In filters the user can reference action objects using:
tc action add action pedit ... index 1
tc filter add ... action pedit index 1

In the current code for act_pedit this is broken as it checks netlink
attributes for create/update before actually checking if we are binding to an
existing action.

tdc results:
1..69
ok 1 319a - Add pedit action that mangles IP TTL
ok 2 7e67 - Replace pedit action with invalid goto chain
ok 3 377e - Add pedit action with RAW_OP offset u32
ok 4 a0ca - Add pedit action with RAW_OP offset u32 (INVALID)
ok 5 dd8a - Add pedit action with RAW_OP offset u16 u16
ok 6 53db - Add pedit action with RAW_OP offset u16 (INVALID)
ok 7 5c7e - Add pedit action with RAW_OP offset u8 add value
ok 8 2893 - Add pedit action with RAW_OP offset u8 quad
ok 9 3a07 - Add pedit action with RAW_OP offset u8-u16-u8
ok 10 ab0f - Add pedit action with RAW_OP offset u16-u8-u8
ok 11 9d12 - Add pedit action with RAW_OP offset u32 set u16 clear u8 invert
ok 12 ebfa - Add pedit action with RAW_OP offset overflow u32 (INVALID)
ok 13 f512 - Add pedit action with RAW_OP offset u16 at offmask shift set
ok 14 c2cb - Add pedit action with RAW_OP offset u32 retain value
ok 15 1762 - Add pedit action with RAW_OP offset u8 clear value
ok 16 bcee - Add pedit action with RAW_OP offset u8 retain value
ok 17 e89f - Add pedit action with RAW_OP offset u16 retain value
ok 18 c282 - Add pedit action with RAW_OP offset u32 clear value
ok 19 c422 - Add pedit action with RAW_OP offset u16 invert value
ok 20 d3d3 - Add pedit action with RAW_OP offset u32 invert value
ok 21 57e5 - Add pedit action with RAW_OP offset u8 preserve value
ok 22 99e0 - Add pedit action with RAW_OP offset u16 preserve value
ok 23 1892 - Add pedit action with RAW_OP offset u32 preserve value
ok 24 4b60 - Add pedit action with RAW_OP negative offset u16/u32 set value
ok 25 a5a7 - Add pedit action with LAYERED_OP eth set src
ok 26 86d4 - Add pedit action with LAYERED_OP eth set src & dst
ok 27 f8a9 - Add pedit action with LAYERED_OP eth set dst
ok 28 c715 - Add pedit action with LAYERED_OP eth set src (INVALID)
ok 29 8131 - Add pedit action with LAYERED_OP eth set dst (INVALID)
ok 30 ba22 - Add pedit action with LAYERED_OP eth type set/clear sequence
ok 31 dec4 - Add pedit action with LAYERED_OP eth set type (INVALID)
ok 32 ab06 - Add pedit action with LAYERED_OP eth add type
ok 33 918d - Add pedit action with LAYERED_OP eth invert src
ok 34 a8d4 - Add pedit action with LAYERED_OP eth invert dst
ok 35 ee13 - Add pedit action with LAYERED_OP eth invert type
ok 36 7588 - Add pedit action with LAYERED_OP ip set src
ok 37 0fa7 - Add pedit action with LAYERED_OP ip set dst
ok 38 5810 - Add pedit action with LAYERED_OP ip set src & dst
ok 39 1092 - Add pedit action with LAYERED_OP ip set ihl & dsfield
ok 40 02d8 - Add pedit action with LAYERED_OP ip set ttl & protocol
ok 41 3e2d - Add pedit action with LAYERED_OP ip set ttl (INVALID)
ok 42 31ae - Add pedit action with LAYERED_OP ip ttl clear/set
ok 43 486f - Add pedit action with LAYERED_OP ip set duplicate fields
ok 44 e790 - Add pedit action with LAYERED_OP ip set ce, df, mf, firstfrag, nofrag fields
ok 45 cc8a - Add pedit action with LAYERED_OP ip set tos
ok 46 7a17 - Add pedit action with LAYERED_OP ip set precedence
ok 47 c3b6 - Add pedit action with LAYERED_OP ip add tos
ok 48 43d3 - Add pedit action with LAYERED_OP ip add precedence
ok 49 438e - Add pedit action with LAYERED_OP ip clear tos
ok 50 6b1b - Add pedit action with LAYERED_OP ip clear precedence
ok 51 824a - Add pedit action with LAYERED_OP ip invert tos
ok 52 106f - Add pedit action with LAYERED_OP ip invert precedence
ok 53 6829 - Add pedit action with LAYERED_OP beyond ip set dport & sport
ok 54 afd8 - Add pedit action with LAYERED_OP beyond ip set icmp_type & icmp_code
ok 55 3143 - Add pedit action with LAYERED_OP beyond ip set dport (INVALID)
ok 56 815c - Add pedit action with LAYERED_OP ip6 set src
ok 57 4dae - Add pedit action with LAYERED_OP ip6 set dst
ok 58 fc1f - Add pedit action with LAYERED_OP ip6 set src & dst
ok 59 6d34 - Add pedit action with LAYERED_OP ip6 dst retain value (INVALID)
ok 60 94bb - Add pedit action with LAYERED_OP ip6 traffic_class
ok 61 6f5e - Add pedit action with LAYERED_OP ip6 flow_lbl
ok 62 6795 - Add pedit action with LAYERED_OP ip6 set payload_len, nexthdr, hoplimit
ok 63 1442 - Add pedit action with LAYERED_OP tcp set dport & sport
ok 64 b7ac - Add pedit action with LAYERED_OP tcp sport set (INVALID)
ok 65 cfcc - Add pedit action with LAYERED_OP tcp flags set
ok 66 3bc4 - Add pedit action with LAYERED_OP tcp set dport, sport & flags fields
ok 67 f1c8 - Add pedit action with LAYERED_OP udp set dport & sport
ok 68 d784 - Add pedit action with mixed RAW/LAYERED_OP #1
ok 69 70ca - Add pedit action with mixed RAW/LAYERED_OP #2

Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers")
Fixes: f67169fef8db ("net/sched: act_pedit: fix WARN() in the traffic path")
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 58 +++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 77d288d384ae..4559a1507ea5 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -181,26 +181,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	}
 
 	parm = nla_data(pattr);
-	if (!parm->nkeys) {
-		NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be passed");
-		return -EINVAL;
-	}
-	ksize = parm->nkeys * sizeof(struct tc_pedit_key);
-	if (nla_len(pattr) < sizeof(*parm) + ksize) {
-		NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid");
-		return -EINVAL;
-	}
-
-	nparms = kzalloc(sizeof(*nparms), GFP_KERNEL);
-	if (!nparms)
-		return -ENOMEM;
-
-	nparms->tcfp_keys_ex =
-		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
-	if (IS_ERR(nparms->tcfp_keys_ex)) {
-		ret = PTR_ERR(nparms->tcfp_keys_ex);
-		goto out_free;
-	}
 
 	index = parm->index;
 	err = tcf_idr_check_alloc(tn, &index, a, bind);
@@ -209,25 +189,49 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 						&act_pedit_ops, bind, flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
-			goto out_free_ex;
+			return ret;
 		}
 		ret = ACT_P_CREATED;
 	} else if (err > 0) {
 		if (bind)
-			goto out_free;
+			return 0;
 		if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
 			ret = -EEXIST;
 			goto out_release;
 		}
 	} else {
-		ret = err;
-		goto out_free_ex;
+		return err;
+	}
+
+	if (!parm->nkeys) {
+		NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be passed");
+		ret = -EINVAL;
+		goto out_release;
+	}
+	ksize = parm->nkeys * sizeof(struct tc_pedit_key);
+	if (nla_len(pattr) < sizeof(*parm) + ksize) {
+		NL_SET_ERR_MSG_ATTR(extack, pattr, "Length of TCA_PEDIT_PARMS or TCA_PEDIT_PARMS_EX pedit attribute is invalid");
+		ret = -EINVAL;
+		goto out_release;
+	}
+
+	nparms = kzalloc(sizeof(*nparms), GFP_KERNEL);
+	if (!nparms) {
+		ret = -ENOMEM;
+		goto out_release;
+	}
+
+	nparms->tcfp_keys_ex =
+		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
+	if (IS_ERR(nparms->tcfp_keys_ex)) {
+		ret = PTR_ERR(nparms->tcfp_keys_ex);
+		goto out_free;
 	}
 
 	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
 	if (err < 0) {
 		ret = err;
-		goto out_release;
+		goto out_free_ex;
 	}
 
 	nparms->tcfp_off_max_hint = 0;
@@ -278,12 +282,12 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 put_chain:
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
-out_release:
-	tcf_idr_release(*a, bind);
 out_free_ex:
 	kfree(nparms->tcfp_keys_ex);
 out_free:
 	kfree(nparms);
+out_release:
+	tcf_idr_release(*a, bind);
 	return ret;
 }
 
-- 
2.34.1

[PATCH net 2/3] net/sched: act_mpls: fix action bind logic

[Pedro Tammela]

The TC architecture allows filters and actions to be created independently.
In filters the user can reference action objects using:
tc action add action mpls ... index 1
tc filter add ... action mpls index 1

In the current code for act_mpls this is broken as it checks netlink
attributes for create/update before actually checking if we are binding to an
existing action.

tdc results:
1..53
ok 1 a933 - Add MPLS dec_ttl action with pipe opcode
ok 2 08d1 - Add mpls dec_ttl action with pass opcode
ok 3 d786 - Add mpls dec_ttl action with drop opcode
ok 4 f334 - Add mpls dec_ttl action with reclassify opcode
ok 5 29bd - Add mpls dec_ttl action with continue opcode
ok 6 48df - Add mpls dec_ttl action with jump opcode
ok 7 62eb - Add mpls dec_ttl action with trap opcode
ok 8 09d2 - Add mpls dec_ttl action with opcode and cookie
ok 9 c170 - Add mpls dec_ttl action with opcode and cookie of max length
ok 10 9118 - Add mpls dec_ttl action with invalid opcode
ok 11 6ce1 - Add mpls dec_ttl action with label (invalid)
ok 12 352f - Add mpls dec_ttl action with tc (invalid)
ok 13 fa1c - Add mpls dec_ttl action with ttl (invalid)
ok 14 6b79 - Add mpls dec_ttl action with bos (invalid)
ok 15 d4c4 - Add mpls pop action with ip proto
ok 16 91fb - Add mpls pop action with ip proto and cookie
ok 17 92fe - Add mpls pop action with mpls proto
ok 18 7e23 - Add mpls pop action with no protocol (invalid)
ok 19 6182 - Add mpls pop action with label (invalid)
ok 20 6475 - Add mpls pop action with tc (invalid)
ok 21 067b - Add mpls pop action with ttl (invalid)
ok 22 7316 - Add mpls pop action with bos (invalid)
ok 23 38cc - Add mpls push action with label
ok 24 c281 - Add mpls push action with mpls_mc protocol
ok 25 5db4 - Add mpls push action with label, tc and ttl
ok 26 7c34 - Add mpls push action with label, tc ttl and cookie of max length
ok 27 16eb - Add mpls push action with label and bos
ok 28 d69d - Add mpls push action with no label (invalid)
ok 29 e8e4 - Add mpls push action with ipv4 protocol (invalid)
ok 30 ecd0 - Add mpls push action with out of range label (invalid)
ok 31 d303 - Add mpls push action with out of range tc (invalid)
ok 32 fd6e - Add mpls push action with ttl of 0 (invalid)
ok 33 19e9 - Add mpls mod action with mpls label
ok 34 1fde - Add mpls mod action with max mpls label
ok 35 0c50 - Add mpls mod action with mpls label exceeding max (invalid)
ok 36 10b6 - Add mpls mod action with mpls label of MPLS_LABEL_IMPLNULL (invalid)
ok 37 57c9 - Add mpls mod action with mpls min tc
ok 38 6872 - Add mpls mod action with mpls max tc
ok 39 a70a - Add mpls mod action with mpls tc exceeding max (invalid)
ok 40 6ed5 - Add mpls mod action with mpls ttl
ok 41 77c1 - Add mpls mod action with mpls ttl and cookie
ok 42 b80f - Add mpls mod action with mpls max ttl
ok 43 8864 - Add mpls mod action with mpls min ttl
ok 44 6c06 - Add mpls mod action with mpls ttl of 0 (invalid)
ok 45 b5d8 - Add mpls mod action with mpls ttl exceeding max (invalid)
ok 46 451f - Add mpls mod action with mpls max bos
ok 47 a1ed - Add mpls mod action with mpls min bos
ok 48 3dcf - Add mpls mod action with mpls bos exceeding max (invalid)
ok 49 db7c - Add mpls mod action with protocol (invalid)
ok 50 b070 - Replace existing mpls push action with new ID
ok 51 95a9 - Replace existing mpls push action with new label, tc, ttl and cookie
ok 52 6cce - Delete mpls pop action
ok 53 d138 - Flush mpls actions

Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_mpls.c | 66 +++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 29 deletions(-)

diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 6b26bdb999d7..809f7928a1be 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -190,40 +190,67 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
 	parm = nla_data(tb[TCA_MPLS_PARMS]);
 	index = parm->index;
 
+	err = tcf_idr_check_alloc(tn, &index, a, bind);
+	if (err < 0)
+		return err;
+	exists = err;
+	if (exists && bind)
+		return 0;
+
+	if (!exists) {
+		ret = tcf_idr_create(tn, index, est, a, &act_mpls_ops, bind,
+				     true, flags);
+		if (ret) {
+			tcf_idr_cleanup(tn, index);
+			return ret;
+		}
+
+		ret = ACT_P_CREATED;
+	} else if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
+		tcf_idr_release(*a, bind);
+		return -EEXIST;
+	}
+
 	/* Verify parameters against action type. */
 	switch (parm->m_action) {
 	case TCA_MPLS_ACT_POP:
 		if (!tb[TCA_MPLS_PROTO]) {
 			NL_SET_ERR_MSG_MOD(extack, "Protocol must be set for MPLS pop");
-			return -EINVAL;
+			err = -EINVAL;
+			goto release_idr;
 		}
 		if (!eth_proto_is_802_3(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
 			NL_SET_ERR_MSG_MOD(extack, "Invalid protocol type for MPLS pop");
-			return -EINVAL;
+			err = -EINVAL;
+			goto release_idr;
 		}
 		if (tb[TCA_MPLS_LABEL] || tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] ||
 		    tb[TCA_MPLS_BOS]) {
 			NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC or BOS cannot be used with MPLS pop");
-			return -EINVAL;
+			err = -EINVAL;
+			goto release_idr;
 		}
 		break;
 	case TCA_MPLS_ACT_DEC_TTL:
 		if (tb[TCA_MPLS_PROTO] || tb[TCA_MPLS_LABEL] ||
 		    tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || tb[TCA_MPLS_BOS]) {
 			NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC, BOS or protocol cannot be used with MPLS dec_ttl");
-			return -EINVAL;
+			err = -EINVAL;
+			goto release_idr;
 		}
 		break;
 	case TCA_MPLS_ACT_PUSH:
 	case TCA_MPLS_ACT_MAC_PUSH:
 		if (!tb[TCA_MPLS_LABEL]) {
 			NL_SET_ERR_MSG_MOD(extack, "Label is required for MPLS push");
-			return -EINVAL;
+			err = -EINVAL;
+			goto release_idr;
 		}
 		if (tb[TCA_MPLS_PROTO] &&
 		    !eth_p_mpls(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
 			NL_SET_ERR_MSG_MOD(extack, "Protocol must be an MPLS type for MPLS push");
-			return -EPROTONOSUPPORT;
+			err = -EPROTONOSUPPORT;
+			goto release_idr;
 		}
 		/* Push needs a TTL - if not specified, set a default value. */
 		if (!tb[TCA_MPLS_TTL]) {
@@ -238,33 +265,14 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
 	case TCA_MPLS_ACT_MODIFY:
 		if (tb[TCA_MPLS_PROTO]) {
 			NL_SET_ERR_MSG_MOD(extack, "Protocol cannot be used with MPLS modify");
-			return -EINVAL;
+			err = -EINVAL;
+			goto release_idr;
 		}
 		break;
 	default:
 		NL_SET_ERR_MSG_MOD(extack, "Unknown MPLS action");
-		return -EINVAL;
-	}
-
-	err = tcf_idr_check_alloc(tn, &index, a, bind);
-	if (err < 0)
-		return err;
-	exists = err;
-	if (exists && bind)
-		return 0;
-
-	if (!exists) {
-		ret = tcf_idr_create(tn, index, est, a,
-				     &act_mpls_ops, bind, true, flags);
-		if (ret) {
-			tcf_idr_cleanup(tn, index);
-			return ret;
-		}
-
-		ret = ACT_P_CREATED;
-	} else if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
-		tcf_idr_release(*a, bind);
-		return -EEXIST;
+		err = -EINVAL;
+		goto release_idr;
 	}
 
 	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
-- 
2.34.1

[PATCH net 3/3] net/sched: act_sample: fix action bind logic

[Pedro Tammela]

The TC architecture allows filters and actions to be created independently.
In filters the user can reference action objects using:
tc action add action sample ... index 1
tc filter add ... action pedit index 1

In the current code for act_sample this is broken as it checks netlink
attributes for create/update before actually checking if we are binding to an
existing action.

tdc results:
1..29
ok 1 9784 - Add valid sample action with mandatory arguments
ok 2 5c91 - Add valid sample action with mandatory arguments and continue control action
ok 3 334b - Add valid sample action with mandatory arguments and drop control action
ok 4 da69 - Add valid sample action with mandatory arguments and reclassify control action
ok 5 13ce - Add valid sample action with mandatory arguments and pipe control action
ok 6 1886 - Add valid sample action with mandatory arguments and jump control action
ok 7 7571 - Add sample action with invalid rate
ok 8 b6d4 - Add sample action with mandatory arguments and invalid control action
ok 9 a874 - Add invalid sample action without mandatory arguments
ok 10 ac01 - Add invalid sample action without mandatory argument rate
ok 11 4203 - Add invalid sample action without mandatory argument group
ok 12 14a7 - Add invalid sample action without mandatory argument group
ok 13 8f2e - Add valid sample action with trunc argument
ok 14 45f8 - Add sample action with maximum rate argument
ok 15 ad0c - Add sample action with maximum trunc argument
ok 16 83a9 - Add sample action with maximum group argument
ok 17 ed27 - Add sample action with invalid rate argument
ok 18 2eae - Add sample action with invalid group argument
ok 19 6ff3 - Add sample action with invalid trunc size
ok 20 2b2a - Add sample action with invalid index
ok 21 dee2 - Add sample action with maximum allowed index
ok 22 560e - Add sample action with cookie
ok 23 704a - Replace existing sample action with new rate argument
ok 24 60eb - Replace existing sample action with new group argument
ok 25 2cce - Replace existing sample action with new trunc argument
ok 26 59d1 - Replace existing sample action with new control argument
ok 27 0a6e - Replace sample action with invalid goto chain control
ok 28 3872 - Delete sample action with valid index
ok 29 a394 - Delete sample action with invalid index

Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_sample.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index f7416b5598e0..4c670e7568dc 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -55,8 +55,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
 					  sample_policy, NULL);
 	if (ret < 0)
 		return ret;
-	if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
-	    !tb[TCA_SAMPLE_PSAMPLE_GROUP])
+
+	if (!tb[TCA_SAMPLE_PARMS])
 		return -EINVAL;
 
 	parm = nla_data(tb[TCA_SAMPLE_PARMS]);
@@ -80,6 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
 		tcf_idr_release(*a, bind);
 		return -EEXIST;
 	}
+
+	if (!tb[TCA_SAMPLE_RATE] || !tb[TCA_SAMPLE_PSAMPLE_GROUP]) {
+		NL_SET_ERR_MSG(extack, "sample rate and group are required");
+		err = -EINVAL;
+		goto release_idr;
+	}
+
 	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
 	if (err < 0)
 		goto release_idr;
-- 
2.34.1

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Simon Horman]

On Fri, Feb 24, 2023 at 12:00:56PM -0300, Pedro Tammela wrote:
> The TC architecture allows filters and actions to be created independently.
> In filters the user can reference action objects using:
> tc action add action pedit ... index 1
> tc filter add ... action pedit index 1
> 
> In the current code for act_pedit this is broken as it checks netlink
> attributes for create/update before actually checking if we are binding to an
> existing action.
> 
> tdc results:
> 1..69

...

Hi Pedro,

Thanks for running the tests :)

I think this patch looks good - though I am still digesting it.
But I do wonder if you considered adding a test for this condition.

Also, what is the failure mode?

If it is that user's can't bind actions to filters,  but the kernel behaves
correctly with configurations it accepts. If so, then perhaps this is more
of a feature than a fix. OTOH, perhaps it's a regression wrt the oldest of
the two patches references below.

I've haven't looked at the other patches in this series yet.
But I expect my comments apply to them too.

> Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers")
> Fixes: f67169fef8db ("net/sched: act_pedit: fix WARN() in the traffic path")
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

...

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Pedro Tammela]

On 25/02/2023 10:08, Simon Horman wrote:
> On Fri, Feb 24, 2023 at 12:00:56PM -0300, Pedro Tammela wrote:
>> The TC architecture allows filters and actions to be created independently.
>> In filters the user can reference action objects using:
>> tc action add action pedit ... index 1
>> tc filter add ... action pedit index 1
>>
>> In the current code for act_pedit this is broken as it checks netlink
>> attributes for create/update before actually checking if we are binding to an
>> existing action.
>>
>> tdc results:
>> 1..69
> 
> ...
> 
> Hi Pedro,
> 
> Thanks for running the tests :)
> 
> I think this patch looks good - though I am still digesting it.
> But I do wonder if you considered adding a test for this condition.

Yes, they are in my backlog to post when net-next reopens.

> 
> Also, what is the failure mode?

When referencing actions via its indexes on filters there would be three 
outcomes:
1 - Action binds to filter (expected)
2 - Action fails netlink parsing in kernel
3 - Action fails parsing in iproute2

I also posted complementary iproute2 patches.

> 
> If it is that user's can't bind actions to filters,  but the kernel behaves
> correctly with configurations it accepts. If so, then perhaps this is more
> of a feature than a fix.

I would argue it's a fix...

> OTOH, perhaps it's a regression wrt the oldest of
> the two patches references below.

...because filters and actions are completely separate TC objects.
There shouldn't be actions that can be created independently but can't 
be really used.

> 
> I've haven't looked at the other patches in this series yet.
> But I expect my comments apply to them too.
> 
>> Fixes: 71d0ed7079df ("net/act_pedit: Support using offset relative to the conventional network headers")
>> Fixes: f67169fef8db ("net/sched: act_pedit: fix WARN() in the traffic path")
>> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> 
> ...

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Simon Horman]

On Sat, Feb 25, 2023 at 10:38:31AM -0300, Pedro Tammela wrote:
> On 25/02/2023 10:08, Simon Horman wrote:
> > On Fri, Feb 24, 2023 at 12:00:56PM -0300, Pedro Tammela wrote:
> > > The TC architecture allows filters and actions to be created independently.
> > > In filters the user can reference action objects using:
> > > tc action add action pedit ... index 1
> > > tc filter add ... action pedit index 1
> > > 
> > > In the current code for act_pedit this is broken as it checks netlink
> > > attributes for create/update before actually checking if we are binding to an
> > > existing action.
> > > 
> > > tdc results:
> > > 1..69
> > 
> > ...
> > 
> > Hi Pedro,
> > 
> > Thanks for running the tests :)
> > 
> > I think this patch looks good - though I am still digesting it.
> > But I do wonder if you considered adding a test for this condition.
> 
> Yes, they are in my backlog to post when net-next reopens.

Excellent, thanks.

> > Also, what is the failure mode?
> 
> When referencing actions via its indexes on filters there would be three
> outcomes:
> 1 - Action binds to filter (expected)
> 2 - Action fails netlink parsing in kernel
> 3 - Action fails parsing in iproute2
> 
> I also posted complementary iproute2 patches.
> 
> > 
> > If it is that user's can't bind actions to filters,  but the kernel behaves
> > correctly with configurations it accepts. If so, then perhaps this is more
> > of a feature than a fix.
> 
> I would argue it's a fix...
> 
> > OTOH, perhaps it's a regression wrt the oldest of
> > the two patches references below.
> 
> ...because filters and actions are completely separate TC objects.
> There shouldn't be actions that can be created independently but can't be
> really used.

I agree that shouldn't be the case.
For me that doesn't make it a bug, but I don't feel strongly about it.

In any case, I'm now happy with this patch.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

...

Re: [PATCH net 2/3] net/sched: act_mpls: fix action bind logic

[Simon Horman]

On Fri, Feb 24, 2023 at 12:00:57PM -0300, Pedro Tammela wrote:
> The TC architecture allows filters and actions to be created independently.
> In filters the user can reference action objects using:
> tc action add action mpls ... index 1
> tc filter add ... action mpls index 1
> 
> In the current code for act_mpls this is broken as it checks netlink
> attributes for create/update before actually checking if we are binding to an
> existing action.
> 
> tdc results:
> 1..53
> ok 1 a933 - Add MPLS dec_ttl action with pipe opcode
> ok 2 08d1 - Add mpls dec_ttl action with pass opcode
> ok 3 d786 - Add mpls dec_ttl action with drop opcode
> ok 4 f334 - Add mpls dec_ttl action with reclassify opcode
> ok 5 29bd - Add mpls dec_ttl action with continue opcode
> ok 6 48df - Add mpls dec_ttl action with jump opcode
> ok 7 62eb - Add mpls dec_ttl action with trap opcode
> ok 8 09d2 - Add mpls dec_ttl action with opcode and cookie
> ok 9 c170 - Add mpls dec_ttl action with opcode and cookie of max length
> ok 10 9118 - Add mpls dec_ttl action with invalid opcode
> ok 11 6ce1 - Add mpls dec_ttl action with label (invalid)
> ok 12 352f - Add mpls dec_ttl action with tc (invalid)
> ok 13 fa1c - Add mpls dec_ttl action with ttl (invalid)
> ok 14 6b79 - Add mpls dec_ttl action with bos (invalid)
> ok 15 d4c4 - Add mpls pop action with ip proto
> ok 16 91fb - Add mpls pop action with ip proto and cookie
> ok 17 92fe - Add mpls pop action with mpls proto
> ok 18 7e23 - Add mpls pop action with no protocol (invalid)
> ok 19 6182 - Add mpls pop action with label (invalid)
> ok 20 6475 - Add mpls pop action with tc (invalid)
> ok 21 067b - Add mpls pop action with ttl (invalid)
> ok 22 7316 - Add mpls pop action with bos (invalid)
> ok 23 38cc - Add mpls push action with label
> ok 24 c281 - Add mpls push action with mpls_mc protocol
> ok 25 5db4 - Add mpls push action with label, tc and ttl
> ok 26 7c34 - Add mpls push action with label, tc ttl and cookie of max length
> ok 27 16eb - Add mpls push action with label and bos
> ok 28 d69d - Add mpls push action with no label (invalid)
> ok 29 e8e4 - Add mpls push action with ipv4 protocol (invalid)
> ok 30 ecd0 - Add mpls push action with out of range label (invalid)
> ok 31 d303 - Add mpls push action with out of range tc (invalid)
> ok 32 fd6e - Add mpls push action with ttl of 0 (invalid)
> ok 33 19e9 - Add mpls mod action with mpls label
> ok 34 1fde - Add mpls mod action with max mpls label
> ok 35 0c50 - Add mpls mod action with mpls label exceeding max (invalid)
> ok 36 10b6 - Add mpls mod action with mpls label of MPLS_LABEL_IMPLNULL (invalid)
> ok 37 57c9 - Add mpls mod action with mpls min tc
> ok 38 6872 - Add mpls mod action with mpls max tc
> ok 39 a70a - Add mpls mod action with mpls tc exceeding max (invalid)
> ok 40 6ed5 - Add mpls mod action with mpls ttl
> ok 41 77c1 - Add mpls mod action with mpls ttl and cookie
> ok 42 b80f - Add mpls mod action with mpls max ttl
> ok 43 8864 - Add mpls mod action with mpls min ttl
> ok 44 6c06 - Add mpls mod action with mpls ttl of 0 (invalid)
> ok 45 b5d8 - Add mpls mod action with mpls ttl exceeding max (invalid)
> ok 46 451f - Add mpls mod action with mpls max bos
> ok 47 a1ed - Add mpls mod action with mpls min bos
> ok 48 3dcf - Add mpls mod action with mpls bos exceeding max (invalid)
> ok 49 db7c - Add mpls mod action with protocol (invalid)
> ok 50 b070 - Replace existing mpls push action with new ID
> ok 51 95a9 - Replace existing mpls push action with new label, tc, ttl and cookie
> ok 52 6cce - Delete mpls pop action
> ok 53 d138 - Flush mpls actions
> 
> Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Minor not notwithstanding,

Reviewed-by: Simon Horman <simon.horman@corigine.com>


> ---
>  net/sched/act_mpls.c | 66 +++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> index 6b26bdb999d7..809f7928a1be 100644
> --- a/net/sched/act_mpls.c
> +++ b/net/sched/act_mpls.c
> @@ -190,40 +190,67 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
>  	parm = nla_data(tb[TCA_MPLS_PARMS]);
>  	index = parm->index;
>  
> +	err = tcf_idr_check_alloc(tn, &index, a, bind);
> +	if (err < 0)
> +		return err;
> +	exists = err;
> +	if (exists && bind)
> +		return 0;
> +
> +	if (!exists) {
> +		ret = tcf_idr_create(tn, index, est, a, &act_mpls_ops, bind,
> +				     true, flags);
> +		if (ret) {
> +			tcf_idr_cleanup(tn, index);
> +			return ret;
> +		}
> +
> +		ret = ACT_P_CREATED;
> +	} else if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
> +		tcf_idr_release(*a, bind);
> +		return -EEXIST;

nit: I think it would be more consistent to goto release_idr here.

> +	}
> +
>  	/* Verify parameters against action type. */
>  	switch (parm->m_action) {
>  	case TCA_MPLS_ACT_POP:
>  		if (!tb[TCA_MPLS_PROTO]) {
>  			NL_SET_ERR_MSG_MOD(extack, "Protocol must be set for MPLS pop");
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto release_idr;
>  		}
>  		if (!eth_proto_is_802_3(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
>  			NL_SET_ERR_MSG_MOD(extack, "Invalid protocol type for MPLS pop");
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto release_idr;
>  		}
>  		if (tb[TCA_MPLS_LABEL] || tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] ||
>  		    tb[TCA_MPLS_BOS]) {
>  			NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC or BOS cannot be used with MPLS pop");
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto release_idr;
>  		}
>  		break;
>  	case TCA_MPLS_ACT_DEC_TTL:
>  		if (tb[TCA_MPLS_PROTO] || tb[TCA_MPLS_LABEL] ||
>  		    tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || tb[TCA_MPLS_BOS]) {
>  			NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC, BOS or protocol cannot be used with MPLS dec_ttl");
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto release_idr;
>  		}
>  		break;
>  	case TCA_MPLS_ACT_PUSH:
>  	case TCA_MPLS_ACT_MAC_PUSH:
>  		if (!tb[TCA_MPLS_LABEL]) {
>  			NL_SET_ERR_MSG_MOD(extack, "Label is required for MPLS push");
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto release_idr;
>  		}
>  		if (tb[TCA_MPLS_PROTO] &&
>  		    !eth_p_mpls(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
>  			NL_SET_ERR_MSG_MOD(extack, "Protocol must be an MPLS type for MPLS push");
> -			return -EPROTONOSUPPORT;
> +			err = -EPROTONOSUPPORT;
> +			goto release_idr;
>  		}
>  		/* Push needs a TTL - if not specified, set a default value. */
>  		if (!tb[TCA_MPLS_TTL]) {
> @@ -238,33 +265,14 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
>  	case TCA_MPLS_ACT_MODIFY:
>  		if (tb[TCA_MPLS_PROTO]) {
>  			NL_SET_ERR_MSG_MOD(extack, "Protocol cannot be used with MPLS modify");
> -			return -EINVAL;
> +			err = -EINVAL;
> +			goto release_idr;
>  		}
>  		break;
>  	default:
>  		NL_SET_ERR_MSG_MOD(extack, "Unknown MPLS action");
> -		return -EINVAL;
> -	}
> -
> -	err = tcf_idr_check_alloc(tn, &index, a, bind);
> -	if (err < 0)
> -		return err;
> -	exists = err;
> -	if (exists && bind)
> -		return 0;
> -
> -	if (!exists) {
> -		ret = tcf_idr_create(tn, index, est, a,
> -				     &act_mpls_ops, bind, true, flags);
> -		if (ret) {
> -			tcf_idr_cleanup(tn, index);
> -			return ret;
> -		}
> -
> -		ret = ACT_P_CREATED;
> -	} else if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
> -		tcf_idr_release(*a, bind);
> -		return -EEXIST;
> +		err = -EINVAL;
> +		goto release_idr;
>  	}
>  
>  	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> -- 
> 2.34.1
> 

Re: [PATCH net 3/3] net/sched: act_sample: fix action bind logic

[Simon Horman]

On Fri, Feb 24, 2023 at 12:00:58PM -0300, Pedro Tammela wrote:
> The TC architecture allows filters and actions to be created independently.
> In filters the user can reference action objects using:
> tc action add action sample ... index 1
> tc filter add ... action pedit index 1
> 
> In the current code for act_sample this is broken as it checks netlink
> attributes for create/update before actually checking if we are binding to an
> existing action.
> 
> tdc results:
> 1..29
> ok 1 9784 - Add valid sample action with mandatory arguments
> ok 2 5c91 - Add valid sample action with mandatory arguments and continue control action
> ok 3 334b - Add valid sample action with mandatory arguments and drop control action
> ok 4 da69 - Add valid sample action with mandatory arguments and reclassify control action
> ok 5 13ce - Add valid sample action with mandatory arguments and pipe control action
> ok 6 1886 - Add valid sample action with mandatory arguments and jump control action
> ok 7 7571 - Add sample action with invalid rate
> ok 8 b6d4 - Add sample action with mandatory arguments and invalid control action
> ok 9 a874 - Add invalid sample action without mandatory arguments
> ok 10 ac01 - Add invalid sample action without mandatory argument rate
> ok 11 4203 - Add invalid sample action without mandatory argument group
> ok 12 14a7 - Add invalid sample action without mandatory argument group
> ok 13 8f2e - Add valid sample action with trunc argument
> ok 14 45f8 - Add sample action with maximum rate argument
> ok 15 ad0c - Add sample action with maximum trunc argument
> ok 16 83a9 - Add sample action with maximum group argument
> ok 17 ed27 - Add sample action with invalid rate argument
> ok 18 2eae - Add sample action with invalid group argument
> ok 19 6ff3 - Add sample action with invalid trunc size
> ok 20 2b2a - Add sample action with invalid index
> ok 21 dee2 - Add sample action with maximum allowed index
> ok 22 560e - Add sample action with cookie
> ok 23 704a - Replace existing sample action with new rate argument
> ok 24 60eb - Replace existing sample action with new group argument
> ok 25 2cce - Replace existing sample action with new trunc argument
> ok 26 59d1 - Replace existing sample action with new control argument
> ok 27 0a6e - Replace sample action with invalid goto chain control
> ok 28 3872 - Delete sample action with valid index
> ok 29 a394 - Delete sample action with invalid index
> 
> Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

I have a minor comment below.
But in the context of this series, this patch looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> ---
>  net/sched/act_sample.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index f7416b5598e0..4c670e7568dc 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -55,8 +55,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
>  					  sample_policy, NULL);
>  	if (ret < 0)
>  		return ret;
> -	if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
> -	    !tb[TCA_SAMPLE_PSAMPLE_GROUP])
> +
> +	if (!tb[TCA_SAMPLE_PARMS])
>  		return -EINVAL;
>  
>  	parm = nla_data(tb[TCA_SAMPLE_PARMS]);
> @@ -80,6 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
>  		tcf_idr_release(*a, bind);
>  		return -EEXIST;

nit: not for this patch, but I think it would be more consistent
     to use goto release_idr here.

>  	}
> +
> +	if (!tb[TCA_SAMPLE_RATE] || !tb[TCA_SAMPLE_PSAMPLE_GROUP]) {
> +		NL_SET_ERR_MSG(extack, "sample rate and group are required");
> +		err = -EINVAL;
> +		goto release_idr;
> +	}
> +
>  	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
>  	if (err < 0)
>  		goto release_idr;
> -- 
> 2.34.1
> 

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Jakub Kicinski]

On Sat, 25 Feb 2023 17:15:00 +0100 Simon Horman wrote:
> > > OTOH, perhaps it's a regression wrt the oldest of
> > > the two patches references below.  
> > 
> > ...because filters and actions are completely separate TC objects.
> > There shouldn't be actions that can be created independently but can't be
> > really used.  
> 
> I agree that shouldn't be the case.
> For me that doesn't make it a bug, but I don't feel strongly about it.

I'm with Simon - this is a long standing problem, and we weren't getting
any user complaints about this. So I also prefer to route this via
net-next, without the Fixes tags.

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Jamal Hadi Salim]

On Mon, Feb 27, 2023 at 2:36 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 25 Feb 2023 17:15:00 +0100 Simon Horman wrote:
> > > > OTOH, perhaps it's a regression wrt the oldest of
> > > > the two patches references below.
> > >
> > > ...because filters and actions are completely separate TC objects.
> > > There shouldn't be actions that can be created independently but can't be
> > > really used.
> >
> > I agree that shouldn't be the case.
> > For me that doesn't make it a bug, but I don't feel strongly about it.
>
> I'm with Simon - this is a long standing problem, and we weren't getting
> any user complaints about this. So I also prefer to route this via
> net-next, without the Fixes tags.

At minimum the pedit is a fix.
The rest is toss-a-coin and put in net-next or net.

cheers,
jamal

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Jakub Kicinski]

On Mon, 27 Feb 2023 14:51:58 -0500 Jamal Hadi Salim wrote:
> > > I agree that shouldn't be the case.
> > > For me that doesn't make it a bug, but I don't feel strongly about it.  
> >
> > I'm with Simon - this is a long standing problem, and we weren't getting
> > any user complaints about this. So I also prefer to route this via
> > net-next, without the Fixes tags.  
> 
> At minimum the pedit is a fix.

How come? What makes pedit different?
It's kinda hard to parse from the diff and the commit messages
look copy/pasted.

> The rest is toss-a-coin and put in net-next or net.

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Jakub Kicinski]

On Mon, 27 Feb 2023 12:04:20 -0800 Jakub Kicinski wrote:
> > > I'm with Simon - this is a long standing problem, and we weren't getting
> > > any user complaints about this. So I also prefer to route this via
> > > net-next, without the Fixes tags.    
> > 
> > At minimum the pedit is a fix.  
> 
> How come? What makes pedit different?
> It's kinda hard to parse from the diff and the commit messages
> look copy/pasted.

Ah, looks like DaveM already applied this (not v2), so the discussion
is moot.

Re: [PATCH net 1/3] net/sched: act_pedit: fix action bind logic

[Jamal Hadi Salim]

On Mon, Feb 27, 2023 at 4:41 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 27 Feb 2023 12:04:20 -0800 Jakub Kicinski wrote:
> > > > I'm with Simon - this is a long standing problem, and we weren't getting
> > > > any user complaints about this. So I also prefer to route this via
> > > > net-next, without the Fixes tags.
> > >
> > > At minimum the pedit is a fix.
> >
> > How come? What makes pedit different?
> > It's kinda hard to parse from the diff and the commit messages
> > look copy/pasted.
>
> Ah, looks like DaveM already applied this (not v2), so the discussion
> is moot.

for completion's sake:
pedit worked and then got broken at some point. The others, I believe
the review  missed the details unfortunately; those features are a
requirement but someone cutnpasted and missed something.

Note: The only reason we even found this is because some hardware
(nameless at this point) capable of offloading and sharing pedit
actions didnt work. Looking closely we found the other two.

cheers,
jamal