[PATCH net-next v2 0/4] net/sched: act_pedit: minor improvements

[PATCH net-next v2 0/4] net/sched: act_pedit: minor improvements

[Pedro Tammela]

This series aims to improve the code and usability of act_pedit for
netlink users.

Patch 1 improves error reporting for extended keys parsing with extack.
While at it, do a minor refactor on error handling.

Patch 2 checks the static offsets a priori on create/update. Currently,
this is done at the datapath for both static and runtime offsets.

Patch 3 removes a check from the datapath which is redundant since the
netlink parsing validates the key types.

Patch 4 changes the 'pr_info()' calls in the datapath to rate limited
versions.

v1->v2: Added patch 3 to the series as discussed with Simon.

Pedro Tammela (4):
  net/sched: act_pedit: use extack in 'ex' parsing errors
  net/sched: act_pedit: check static offsets a priori
  net/sched: act_pedit: remove extra check for key type
  net/sched: act_pedit: rate limit datapath messages

 net/sched/act_pedit.c | 77 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/4] net/sched: act_pedit: use extack in 'ex' parsing errors

[Pedro Tammela]

We have extack available when parsing 'ex' keys, so pass it to
tcf_pedit_keys_ex_parse and add more detailed error messages.
While at it, remove redundant code from the 'err_out' label code path.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 4559a1507ea5..be9e7e565551 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -35,7 +35,7 @@ static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = {
 };
 
 static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
-							u8 n)
+							u8 n, struct netlink_ext_ack *extack)
 {
 	struct tcf_pedit_key_ex *keys_ex;
 	struct tcf_pedit_key_ex *k;
@@ -56,25 +56,25 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
 
 		if (!n) {
-			err = -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "Can't parse more extended keys than requested");
 			goto err_out;
 		}
+
 		n--;
 
 		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
-			err = -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "Unknown attribute, expected extended key");
 			goto err_out;
 		}
 
-		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX,
-						  ka, pedit_key_ex_policy,
-						  NULL);
+		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX, ka,
+						  pedit_key_ex_policy, extack);
 		if (err)
 			goto err_out;
 
 		if (!tb[TCA_PEDIT_KEY_EX_HTYPE] ||
 		    !tb[TCA_PEDIT_KEY_EX_CMD]) {
-			err = -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "Extended Pedit missing required attributes");
 			goto err_out;
 		}
 
@@ -83,7 +83,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 
 		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
 		    k->cmd > TCA_PEDIT_CMD_MAX) {
-			err = -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "Extended Pedit key is malformed");
 			goto err_out;
 		}
 
@@ -91,7 +91,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 	}
 
 	if (n) {
-		err = -EINVAL;
+		NL_SET_ERR_MSG_MOD(extack, "Not enough extended keys to parse");
 		goto err_out;
 	}
 
@@ -99,7 +99,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 
 err_out:
 	kfree(keys_ex);
-	return ERR_PTR(err);
+	return ERR_PTR(-EINVAL);
 }
 
 static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
@@ -222,7 +222,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	}
 
 	nparms->tcfp_keys_ex =
-		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
+		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys, extack);
 	if (IS_ERR(nparms->tcfp_keys_ex)) {
 		ret = PTR_ERR(nparms->tcfp_keys_ex);
 		goto out_free;
-- 
2.34.1

[PATCH net-next v2 2/4] net/sched: act_pedit: check static offsets a priori

[Pedro Tammela]

Static key offsets should always be on 32 bit boundaries. Validate them on
create/update time for static offsets and move the datapath validation
for runtime offsets only.

iproute2 already errors out if a given offset and data size cannot be packed
to a 32 bit boundary. This change will make sure users which create/update pedit
instances directly via netlink also error out, instead of finding out
when packets are traversing.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index be9e7e565551..e6725e329ec1 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -249,6 +249,12 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	for (i = 0; i < nparms->tcfp_nkeys; ++i) {
 		u32 cur = nparms->tcfp_keys[i].off;
 
+		if (cur % 4) {
+			NL_SET_ERR_MSG_MOD(extack, "Pedit offsets must be on 32bit boundaries");
+			ret = -EINVAL;
+			goto put_chain;
+		}
+
 		/* sanitize the shift value for any later use */
 		nparms->tcfp_keys[i].shift = min_t(size_t,
 						   BITS_PER_TYPE(int) - 1,
@@ -407,12 +413,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 					       sizeof(_d), &_d);
 			if (!d)
 				goto bad;
-			offset += (*d & tkey->offmask) >> tkey->shift;
-		}
 
-		if (offset % 4) {
-			pr_info("tc action pedit offset must be on 32 bit boundaries\n");
-			goto bad;
+			offset += (*d & tkey->offmask) >> tkey->shift;
+			if (offset % 4) {
+				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
+				goto bad;
+			}
 		}
 
 		if (!offset_valid(skb, hoffset + offset)) {
-- 
2.34.1

[PATCH net-next v2 3/4] net/sched: act_pedit: remove extra check for key type

[Pedro Tammela]

The netlink parsing already validates the key 'htype'.
Remove the datapath check as it's redundant.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index e6725e329ec1..b09e931f23d5 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -319,37 +319,28 @@ static bool offset_valid(struct sk_buff *skb, int offset)
 	return true;
 }
 
-static int pedit_skb_hdr_offset(struct sk_buff *skb,
-				enum pedit_header_type htype, int *hoffset)
+static void pedit_skb_hdr_offset(struct sk_buff *skb,
+				 enum pedit_header_type htype, int *hoffset)
 {
-	int ret = -EINVAL;
-
+	/* 'htype' is validated in the netlink parsing */
 	switch (htype) {
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH:
-		if (skb_mac_header_was_set(skb)) {
+		if (skb_mac_header_was_set(skb))
 			*hoffset = skb_mac_offset(skb);
-			ret = 0;
-		}
 		break;
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6:
 		*hoffset = skb_network_offset(skb);
-		ret = 0;
 		break;
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP:
 	case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP:
-		if (skb_transport_header_was_set(skb)) {
+		if (skb_transport_header_was_set(skb))
 			*hoffset = skb_transport_offset(skb);
-			ret = 0;
-		}
 		break;
 	default:
-		ret = -EINVAL;
 		break;
 	}
-
-	return ret;
 }
 
 TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
@@ -382,10 +373,9 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 
 	for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) {
 		int offset = tkey->off;
+		int hoffset = 0;
 		u32 *ptr, hdata;
-		int hoffset;
 		u32 val;
-		int rc;
 
 		if (tkey_ex) {
 			htype = tkey_ex->htype;
@@ -394,12 +384,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			tkey_ex++;
 		}
 
-		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
-		if (rc) {
-			pr_info("tc action pedit bad header type specified (0x%x)\n",
-				htype);
-			goto bad;
-		}
+		pedit_skb_hdr_offset(skb, htype, &hoffset);
 
 		if (tkey->offmask) {
 			u8 *d, _d;
-- 
2.34.1

[PATCH net-next v2 4/4] net/sched: act_pedit: rate limit datapath messages

[Pedro Tammela]

Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily
depending on the action created. As these messages are informational, usually printing
some, not all, is enough to bring attention to the real issue.

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index b09e931f23d5..ffcbc83dd5c5 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -390,8 +390,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			u8 *d, _d;
 
 			if (!offset_valid(skb, hoffset + tkey->at)) {
-				pr_info("tc action pedit 'at' offset %d out of bounds\n",
-					hoffset + tkey->at);
+				pr_info_ratelimited("tc action pedit 'at' offset %d out of bounds\n",
+						    hoffset + tkey->at);
 				goto bad;
 			}
 			d = skb_header_pointer(skb, hoffset + tkey->at,
@@ -401,14 +401,13 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 
 			offset += (*d & tkey->offmask) >> tkey->shift;
 			if (offset % 4) {
-				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
+				pr_info_ratelimited("tc action pedit offset must be on 32 bit boundaries\n");
 				goto bad;
 			}
 		}
 
 		if (!offset_valid(skb, hoffset + offset)) {
-			pr_info("tc action pedit offset %d out of bounds\n",
-				hoffset + offset);
+			pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset);
 			goto bad;
 		}
 
@@ -425,8 +424,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 			val = (*ptr + tkey->val) & ~tkey->mask;
 			break;
 		default:
-			pr_info("tc action pedit bad command (%d)\n",
-				cmd);
+			pr_info_ratelimited("tc action pedit bad command (%d)\n", cmd);
 			goto bad;
 		}
 
-- 
2.34.1

Re: [PATCH net-next v2 0/4] net/sched: act_pedit: minor improvements

[Pedro Tammela]

On 14/03/2023 17:24, Pedro Tammela wrote:
> This series aims to improve the code and usability of act_pedit for
> netlink users.
> 
> Patch 1 improves error reporting for extended keys parsing with extack.
> While at it, do a minor refactor on error handling.
> 
> Patch 2 checks the static offsets a priori on create/update. Currently,
> this is done at the datapath for both static and runtime offsets.
> 
> Patch 3 removes a check from the datapath which is redundant since the
> netlink parsing validates the key types.
> 
> Patch 4 changes the 'pr_info()' calls in the datapath to rate limited
> versions.
> 
> v1->v2: Added patch 3 to the series as discussed with Simon.
> 
> Pedro Tammela (4):
>    net/sched: act_pedit: use extack in 'ex' parsing errors
>    net/sched: act_pedit: check static offsets a priori
>    net/sched: act_pedit: remove extra check for key type
>    net/sched: act_pedit: rate limit datapath messages
> 
>   net/sched/act_pedit.c | 77 +++++++++++++++++++------------------------
>   1 file changed, 33 insertions(+), 44 deletions(-)
> 

I forgot to add this to the cover letter:

1..70
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
ok 70 abdc - Reference pedit action object in filter

Re: [PATCH net-next v2 1/4] net/sched: act_pedit: use extack in 'ex' parsing errors

[Simon Horman]

On Tue, Mar 14, 2023 at 05:24:45PM -0300, Pedro Tammela wrote:
> We have extack available when parsing 'ex' keys, so pass it to
> tcf_pedit_keys_ex_parse and add more detailed error messages.
> While at it, remove redundant code from the 'err_out' label code path.
> 
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  net/sched/act_pedit.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 4559a1507ea5..be9e7e565551 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -35,7 +35,7 @@ static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = {
>  };
>  
>  static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
> -							u8 n)
> +							u8 n, struct netlink_ext_ack *extack)
>  {
>  	struct tcf_pedit_key_ex *keys_ex;
>  	struct tcf_pedit_key_ex *k;
> @@ -56,25 +56,25 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>  		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
>  
>  		if (!n) {
> -			err = -EINVAL;
> +			NL_SET_ERR_MSG_MOD(extack, "Can't parse more extended keys than requested");
>  			goto err_out;
>  		}
> +
>  		n--;
>  
>  		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
> -			err = -EINVAL;
> +			NL_SET_ERR_MSG_MOD(extack, "Unknown attribute, expected extended key");
>  			goto err_out;
>  		}
>  
> -		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX,
> -						  ka, pedit_key_ex_policy,
> -						  NULL);
> +		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX, ka,
> +						  pedit_key_ex_policy, extack);
>  		if (err)
>  			goto err_out;

err_out will return ERR_PTR(-EINVAL).
I.e. the value of is not propagated.
Are you sure it is always -EINVAL?

>  
>  		if (!tb[TCA_PEDIT_KEY_EX_HTYPE] ||
>  		    !tb[TCA_PEDIT_KEY_EX_CMD]) {
> -			err = -EINVAL;
> +			NL_SET_ERR_MSG_MOD(extack, "Extended Pedit missing required attributes");
>  			goto err_out;
>  		}
>  
> @@ -83,7 +83,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>  
>  		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
>  		    k->cmd > TCA_PEDIT_CMD_MAX) {
> -			err = -EINVAL;
> +			NL_SET_ERR_MSG_MOD(extack, "Extended Pedit key is malformed");
>  			goto err_out;
>  		}
>  
> @@ -91,7 +91,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>  	}
>  
>  	if (n) {
> -		err = -EINVAL;
> +		NL_SET_ERR_MSG_MOD(extack, "Not enough extended keys to parse");
>  		goto err_out;
>  	}
>  
> @@ -99,7 +99,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>  
>  err_out:
>  	kfree(keys_ex);
> -	return ERR_PTR(err);
> +	return ERR_PTR(-EINVAL);
>  }
>  
>  static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
> @@ -222,7 +222,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>  	}
>  
>  	nparms->tcfp_keys_ex =
> -		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
> +		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys, extack);
>  	if (IS_ERR(nparms->tcfp_keys_ex)) {
>  		ret = PTR_ERR(nparms->tcfp_keys_ex);
>  		goto out_free;
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 2/4] net/sched: act_pedit: check static offsets a priori

[Simon Horman]

On Tue, Mar 14, 2023 at 05:24:46PM -0300, Pedro Tammela wrote:
> Static key offsets should always be on 32 bit boundaries. Validate them on
> create/update time for static offsets and move the datapath validation
> for runtime offsets only.
> 
> iproute2 already errors out if a given offset and data size cannot be packed
> to a 32 bit boundary. This change will make sure users which create/update pedit
> instances directly via netlink also error out, instead of finding out
> when packets are traversing.
> 
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

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

Re: [PATCH net-next v2 3/4] net/sched: act_pedit: remove extra check for key type

[Simon Horman]

On Tue, Mar 14, 2023 at 05:24:47PM -0300, Pedro Tammela wrote:
> The netlink parsing already validates the key 'htype'.
> Remove the datapath check as it's redundant.
> 
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

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

Re: [PATCH net-next v2 4/4] net/sched: act_pedit: rate limit datapath messages

[Simon Horman]

On Tue, Mar 14, 2023 at 05:24:48PM -0300, Pedro Tammela wrote:
> Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily
> depending on the action created. As these messages are informational, usually printing
> some, not all, is enough to bring attention to the real issue.
> 
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

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

Re: [PATCH net-next v2 1/4] net/sched: act_pedit: use extack in 'ex' parsing errors

[Pedro Tammela]

On 15/03/2023 12:51, Simon Horman wrote:
> On Tue, Mar 14, 2023 at 05:24:45PM -0300, Pedro Tammela wrote:
>> We have extack available when parsing 'ex' keys, so pass it to
>> tcf_pedit_keys_ex_parse and add more detailed error messages.
>> While at it, remove redundant code from the 'err_out' label code path.
>>
>> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>> ---
>>   net/sched/act_pedit.c | 22 +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>> index 4559a1507ea5..be9e7e565551 100644
>> --- a/net/sched/act_pedit.c
>> +++ b/net/sched/act_pedit.c
>> @@ -35,7 +35,7 @@ static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = {
>>   };
>>   
>>   static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>> -							u8 n)
>> +							u8 n, struct netlink_ext_ack *extack)
>>   {
>>   	struct tcf_pedit_key_ex *keys_ex;
>>   	struct tcf_pedit_key_ex *k;
>> @@ -56,25 +56,25 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>>   		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
>>   
>>   		if (!n) {
>> -			err = -EINVAL;
>> +			NL_SET_ERR_MSG_MOD(extack, "Can't parse more extended keys than requested");
>>   			goto err_out;
>>   		}
>> +
>>   		n--;
>>   
>>   		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
>> -			err = -EINVAL;
>> +			NL_SET_ERR_MSG_MOD(extack, "Unknown attribute, expected extended key");
>>   			goto err_out;
>>   		}
>>   
>> -		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX,
>> -						  ka, pedit_key_ex_policy,
>> -						  NULL);
>> +		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX, ka,
>> +						  pedit_key_ex_policy, extack);
>>   		if (err)
>>   			goto err_out;
> 
> err_out will return ERR_PTR(-EINVAL).
> I.e. the value of is not propagated.
> Are you sure it is always -EINVAL?

Good catch, I will propagate the error.

> 
>>   
>>   		if (!tb[TCA_PEDIT_KEY_EX_HTYPE] ||
>>   		    !tb[TCA_PEDIT_KEY_EX_CMD]) {
>> -			err = -EINVAL;
>> +			NL_SET_ERR_MSG_MOD(extack, "Extended Pedit missing required attributes");
>>   			goto err_out;
>>   		}
>>   
>> @@ -83,7 +83,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>>   
>>   		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
>>   		    k->cmd > TCA_PEDIT_CMD_MAX) {
>> -			err = -EINVAL;
>> +			NL_SET_ERR_MSG_MOD(extack, "Extended Pedit key is malformed");
>>   			goto err_out;
>>   		}
>>   
>> @@ -91,7 +91,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>>   	}
>>   
>>   	if (n) {
>> -		err = -EINVAL;
>> +		NL_SET_ERR_MSG_MOD(extack, "Not enough extended keys to parse");
>>   		goto err_out;
>>   	}
>>   
>> @@ -99,7 +99,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>>   
>>   err_out:
>>   	kfree(keys_ex);
>> -	return ERR_PTR(err);
>> +	return ERR_PTR(-EINVAL);
>>   }
>>   
>>   static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
>> @@ -222,7 +222,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>>   	}
>>   
>>   	nparms->tcfp_keys_ex =
>> -		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys);
>> +		tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys, extack);
>>   	if (IS_ERR(nparms->tcfp_keys_ex)) {
>>   		ret = PTR_ERR(nparms->tcfp_keys_ex);
>>   		goto out_free;
>> -- 
>> 2.34.1
>>