[PATCH net-next v4 0/5] net/sched: act_pedit: minor improvements

[PATCH net-next v4 0/5] net/sched: act_pedit: minor improvements

[Pedro Tammela]

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

Patches 1-2 improves error reporting for extended keys parsing with extack.

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

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

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

v3->v4: Break the old patch 1 into two patches.
v2->v3: Propagate nl_parse errors in patch 1 like the original version.
v1->v2: Added patch 3 to the series as discussed with Simon.

Pedro Tammela (5):
  net/sched: act_pedit: simplify 'ex' key parsing error propagation
  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 | 88 +++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH net-next v4 1/5] net/sched: act_pedit: simplify 'ex' key parsing error propagation

[Pedro Tammela]

'err' is returned -EINVAL most of the time.
Make the exception be the netlink parsing and remove the
redundant error assignments in the other code paths.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 4559a1507ea5..90f5214e679e 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -54,46 +54,39 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 
 	nla_for_each_nested(ka, nla, rem) {
 		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
+		int ret;
 
-		if (!n) {
-			err = -EINVAL;
+		if (!n)
 			goto err_out;
-		}
 		n--;
 
-		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
-			err = -EINVAL;
+		if (nla_type(ka) != TCA_PEDIT_KEY_EX)
 			goto err_out;
-		}
 
-		err = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX,
+		ret = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX,
 						  ka, pedit_key_ex_policy,
 						  NULL);
-		if (err)
+		if (ret) {
+			err = ret;
 			goto err_out;
+		}
 
 		if (!tb[TCA_PEDIT_KEY_EX_HTYPE] ||
-		    !tb[TCA_PEDIT_KEY_EX_CMD]) {
-			err = -EINVAL;
+		    !tb[TCA_PEDIT_KEY_EX_CMD])
 			goto err_out;
-		}
 
 		k->htype = nla_get_u16(tb[TCA_PEDIT_KEY_EX_HTYPE]);
 		k->cmd = nla_get_u16(tb[TCA_PEDIT_KEY_EX_CMD]);
 
 		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
-		    k->cmd > TCA_PEDIT_CMD_MAX) {
-			err = -EINVAL;
+		    k->cmd > TCA_PEDIT_CMD_MAX)
 			goto err_out;
-		}
 
 		k++;
 	}
 
-	if (n) {
-		err = -EINVAL;
+	if (n)
 		goto err_out;
-	}
 
 	return keys_ex;
 
-- 
2.34.1

[PATCH net-next v4 2/5] 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.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/act_pedit.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 90f5214e679e..c8f27a384800 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,37 +56,51 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
 		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
 		int ret;
 
-		if (!n)
+		if (!n) {
+			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)
+		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
+			NL_SET_ERR_MSG_MOD(extack, "Unknown attribute, expected extended key");
 			goto err_out;
+		}
 
-		ret = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX,
-						  ka, pedit_key_ex_policy,
-						  NULL);
+		ret = nla_parse_nested_deprecated(tb, TCA_PEDIT_KEY_EX_MAX, ka,
+						  pedit_key_ex_policy, extack);
 		if (ret) {
 			err = ret;
 			goto err_out;
 		}
 
-		if (!tb[TCA_PEDIT_KEY_EX_HTYPE] ||
-		    !tb[TCA_PEDIT_KEY_EX_CMD])
+		if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PEDIT_KEY_EX_HTYPE)) {
+			NL_SET_ERR_MSG(extack, "Missing required attribute");
 			goto err_out;
+		}
+
+		if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PEDIT_KEY_EX_CMD)) {
+			NL_SET_ERR_MSG(extack, "Missing required attribute");
+			goto err_out;
+		}
 
 		k->htype = nla_get_u16(tb[TCA_PEDIT_KEY_EX_HTYPE]);
 		k->cmd = nla_get_u16(tb[TCA_PEDIT_KEY_EX_CMD]);
 
 		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
-		    k->cmd > TCA_PEDIT_CMD_MAX)
+		    k->cmd > TCA_PEDIT_CMD_MAX) {
+			NL_SET_ERR_MSG_MOD(extack, "Extended key is malformed");
 			goto err_out;
+		}
 
 		k++;
 	}
 
-	if (n)
+	if (n) {
+		NL_SET_ERR_MSG_MOD(extack, "Not enough extended keys to parse");
 		goto err_out;
+	}
 
 	return keys_ex;
 
@@ -215,7 +229,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 v4 3/5] 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>
Reviewed-by: Simon Horman <simon.horman@corigine.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 c8f27a384800..ef6cdf17743b 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -256,6 +256,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, "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,
@@ -414,12 +420,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 v4 4/5] 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>
Reviewed-by: Simon Horman <simon.horman@corigine.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 ef6cdf17743b..2eacada5dcbb 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -326,37 +326,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,
@@ -389,10 +380,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;
@@ -401,12 +391,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 v4 5/5] 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>
Reviewed-by: Simon Horman <simon.horman@corigine.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 2eacada5dcbb..791144012c91 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -397,8 +397,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,
@@ -408,14 +408,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;
 		}
 
@@ -432,8 +431,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 v4 1/5] net/sched: act_pedit: simplify 'ex' key parsing error propagation

[Simon Horman]

On Tue, Apr 18, 2023 at 08:43:50PM -0300, Pedro Tammela wrote:
> 'err' is returned -EINVAL most of the time.
> Make the exception be the netlink parsing and remove the
> redundant error assignments in the other code paths.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

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

Re: [PATCH net-next v4 2/5] net/sched: act_pedit: use extack in 'ex' parsing errors

[Simon Horman]

On Tue, Apr 18, 2023 at 08:43:51PM -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.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

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

Re: [PATCH net-next v4 1/5] net/sched: act_pedit: simplify 'ex' key parsing error propagation

[Jakub Kicinski]

On Tue, 18 Apr 2023 20:43:50 -0300 Pedro Tammela wrote:
> 'err' is returned -EINVAL most of the time.
> Make the exception be the netlink parsing and remove the
> redundant error assignments in the other code paths.

> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 4559a1507ea5..90f5214e679e 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -54,46 +54,39 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla,
>  
>  	nla_for_each_nested(ka, nla, rem) {
>  		struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1];
> +		int ret;
>  
> -		if (!n) {
> -			err = -EINVAL;
> +		if (!n)
>  			goto err_out;
> -		}
>  		n--;

IMHO this is not worth doing. Setting the error value before the jump
is more idiomatic. If anything I'd remove the unnecessary init of err
to EINVAL at the start of the function.

Re: [PATCH net-next v4 2/5] net/sched: act_pedit: use extack in 'ex' parsing errors

[Jakub Kicinski]

On Tue, 18 Apr 2023 20:43:51 -0300 Pedro Tammela wrote:
> -		if (nla_type(ka) != TCA_PEDIT_KEY_EX)
> +		if (nla_type(ka) != TCA_PEDIT_KEY_EX) {
> +			NL_SET_ERR_MSG_MOD(extack, "Unknown attribute, expected extended key");
>  			goto err_out;
> +		}

This is a check on ka, we should use NL_SET_ERR_MSG_ATTR()

>  		k->htype = nla_get_u16(tb[TCA_PEDIT_KEY_EX_HTYPE]);
>  		k->cmd = nla_get_u16(tb[TCA_PEDIT_KEY_EX_CMD]);
>  
>  		if (k->htype > TCA_PEDIT_HDR_TYPE_MAX ||
> -		    k->cmd > TCA_PEDIT_CMD_MAX)
> +		    k->cmd > TCA_PEDIT_CMD_MAX) {
> +			NL_SET_ERR_MSG_MOD(extack, "Extended key is malformed");

And these are checks for tb[TCA_PEDIT_KEY_EX_HTYPE] and
tb[TCA_PEDIT_KEY_EX_CMD], should be part of the parsing policy.
-- 
pw-bot: cr

Re: [PATCH net-next v4 3/5] net/sched: act_pedit: check static offsets a priori

[Jakub Kicinski]

On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote:
> @@ -414,12 +420,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;

this line loads part of the offset from packet data, so it's not
exactly equivalent to the init time check. It's unlikely to be used
but I think that rejecting cur % 4 vs data patch check only is
technically a functional change, so needs to be discussed in the commit
msg.

> +			if (offset % 4) {
> +				pr_info("tc action pedit offset must be on 32 bit boundaries\n");
> +				goto bad;
> +			}

Re: [PATCH net-next v4 3/5] net/sched: act_pedit: check static offsets a priori

[Pedro Tammela]

On 20/04/2023 23:41, Jakub Kicinski wrote:
> On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote:
>> @@ -414,12 +420,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;
> 
> this line loads part of the offset from packet data, so it's not
> exactly equivalent to the init time check.

The code uses 'tkey->offmask' as a check for static offsets vs packet 
derived offsets, which have different handling.
By checking the static offsets at init we can move the datapath
'offset % 4' check for the packet derived offsets only.

Note that this change only affects the offsets defined in 'tkey->off', 
the 'at' offset logic stays the same.
My intention was to keep the code semantically the same.
Did I miss anything?

> It's unlikely to be used
> but I think that rejecting cur % 4 vs data patch check only is
> technically a functional change, so needs to be discussed in the commit
> msg.
>

Re: [PATCH net-next v4 3/5] net/sched: act_pedit: check static offsets a priori

[Jakub Kicinski]

On Fri, 21 Apr 2023 12:12:54 -0300 Pedro Tammela wrote:
> On 20/04/2023 23:41, Jakub Kicinski wrote:
> > On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote:  
> >> @@ -414,12 +420,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;  
> > 
> > this line loads part of the offset from packet data, so it's not
> > exactly equivalent to the init time check.  
> 
> The code uses 'tkey->offmask' as a check for static offsets vs packet 
> derived offsets, which have different handling.
> By checking the static offsets at init we can move the datapath
> 'offset % 4' check for the packet derived offsets only.
> 
> Note that this change only affects the offsets defined in 'tkey->off', 
> the 'at' offset logic stays the same.
> My intention was to keep the code semantically the same.
> Did I miss anything?

You are now erroring out if the static offset is not divisible by 4,
and technically it was possible to construct a case where that'd work
previously - if static offset was 2 and we load another 2 from the
packet, no?

If so it needs to be mentioned in the commit msg.

Re: [PATCH net-next v4 3/5] net/sched: act_pedit: check static offsets a priori

[Pedro Tammela]

On 21/04/2023 12:15, Jakub Kicinski wrote:
> On Fri, 21 Apr 2023 12:12:54 -0300 Pedro Tammela wrote:
>> On 20/04/2023 23:41, Jakub Kicinski wrote:
>>> On Tue, 18 Apr 2023 20:43:52 -0300 Pedro Tammela wrote:
>>>> @@ -414,12 +420,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;
>>>
>>> this line loads part of the offset from packet data, so it's not
>>> exactly equivalent to the init time check.
>>
>> The code uses 'tkey->offmask' as a check for static offsets vs packet
>> derived offsets, which have different handling.
>> By checking the static offsets at init we can move the datapath
>> 'offset % 4' check for the packet derived offsets only.
>>
>> Note that this change only affects the offsets defined in 'tkey->off',
>> the 'at' offset logic stays the same.
>> My intention was to keep the code semantically the same.
>> Did I miss anything?
> 
> You are now erroring out if the static offset is not divisible by 4,
> and technically it was possible to construct a case where that'd work
> previously - if static offset was 2 and we load another 2 from the
> packet, no?
> 

True!

It seems though that the iproute2 packing broke this feature:
tc action add action pedit ex munge offset 2 u16 at 128 ff 0 clear

offset will be rounded down to 0 in iproute2, so if in the datapath at 
128 sums 2, it will fail the offset check since 2 % 4 != 0.

Let me check with Jamal what he thinks about this change since netlink 
could do it still.
This slipped under our radar, thanks for catching it!

> If so it needs to be mentioned in the commit msg.