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

[PATCH net-next 0/3] 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 changes the 'pr_info()' calls in the datapath to rate limited
versions.

Pedro Tammela (3):
  net/sched: act_pedit: use extack in 'ex' parsing errors
  net/sched: act_pedit: check static offsets a priori
  net/sched: act_pedit: rate limit datapath messages

 net/sched/act_pedit.c | 58 ++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH net-next 1/3] 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 | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 4559a1507ea5..f4ebe06aeaf2 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;
@@ -55,26 +55,21 @@ 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];
 
-		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,
-						  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,23 +78,21 @@ 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;
 		}
 
 		k++;
 	}
 
-	if (n) {
-		err = -EINVAL;
+	if (n)
 		goto err_out;
-	}
 
 	return keys_ex;
 
 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 +215,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 1/3] net/sched: act_pedit: use extack in 'ex' parsing errors

[Simon Horman]

On Thu, Mar 09, 2023 at 03:51:56PM -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>

Some errors in tcf_pedit_keys_ex_parse() result in extact being set.
And some don't.

Is that intentional?

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

[Pedro Tammela]

On 10/03/2023 11:19, Simon Horman wrote:
> On Thu, Mar 09, 2023 at 03:51:56PM -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>
> 
> Some errors in tcf_pedit_keys_ex_parse() result in extact being set.
> And some don't.
> 
> Is that intentional?

Good point, I will add more messages.

[PATCH net-next 2/3] 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 f4ebe06aeaf2..e42cbfc369ff 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -242,6 +242,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,
@@ -400,12 +406,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

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

[Simon Horman]

On Thu, Mar 09, 2023 at 03:51:57PM -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>

[PATCH net-next 3/3] 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 | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index e42cbfc369ff..b5a8fc19ee55 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
 		}
 
 		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
-		if (rc) {
-			pr_info("tc action pedit bad header type specified (0x%x)\n",
-				htype);
+		if (unlikely(rc)) {
+			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
 			goto bad;
 		}
 
@@ -398,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,
@@ -409,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;
 		}
 
@@ -433,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 3/3] net/sched: act_pedit: rate limit datapath messages

[Simon Horman]

On Thu, Mar 09, 2023 at 03:51:58PM -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.

Would this reasoning also apply to other TC actions?

> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  net/sched/act_pedit.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index e42cbfc369ff..b5a8fc19ee55 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>  		}
>  
>  		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
> -		if (rc) {
> -			pr_info("tc action pedit bad header type specified (0x%x)\n",
> -				htype);
> +		if (unlikely(rc)) {

Do you really need unlikely() here (and no where else?)

> +			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
>  			goto bad;
>  		}
>  

...

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

[Pedro Tammela]

On 10/03/2023 11:21, Simon Horman wrote:
> On Thu, Mar 09, 2023 at 03:51:58PM -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.
> 
> Would this reasoning also apply to other TC actions?

Hi Simon,

So far, the only action that has datapath pr_info() messages is pedit.
This seems like it comes from the old days, according to git.

> 
>> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>> ---
>>   net/sched/act_pedit.c | 17 +++++++----------
>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
>> index e42cbfc369ff..b5a8fc19ee55 100644
>> --- a/net/sched/act_pedit.c
>> +++ b/net/sched/act_pedit.c
>> @@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
>>   		}
>>   
>>   		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
>> -		if (rc) {
>> -			pr_info("tc action pedit bad header type specified (0x%x)\n",
>> -				htype);
>> +		if (unlikely(rc)) {
> 
> Do you really need unlikely() here (and no where else?)

This case in particular is already checked in the netlink parsing code 
on create/update.
I was gonna delete the condition initially but then thought of hiding it 
under an unlikely branch.
As for the other branches, I didn't see much of a reason.

> 
>> +			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
>>   			goto bad;
>>   		}
>>   
> 
> ...

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

[Simon Horman]

On Mon, Mar 13, 2023 at 03:24:47PM -0300, Pedro Tammela wrote:
> On 10/03/2023 11:21, Simon Horman wrote:
> > On Thu, Mar 09, 2023 at 03:51:58PM -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.
> > 
> > Would this reasoning also apply to other TC actions?
> 
> Hi Simon,
> 
> So far, the only action that has datapath pr_info() messages is pedit.
> This seems like it comes from the old days, according to git.

I'd be in favour of unifying things.
But perhaps that is a topic for another day.

> > > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> > > ---
> > >   net/sched/act_pedit.c | 17 +++++++----------
> > >   1 file changed, 7 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > > index e42cbfc369ff..b5a8fc19ee55 100644
> > > --- a/net/sched/act_pedit.c
> > > +++ b/net/sched/act_pedit.c
> > > @@ -388,9 +388,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb,
> > >   		}
> > >   		rc = pedit_skb_hdr_offset(skb, htype, &hoffset);
> > > -		if (rc) {
> > > -			pr_info("tc action pedit bad header type specified (0x%x)\n",
> > > -				htype);
> > > +		if (unlikely(rc)) {
> > 
> > Do you really need unlikely() here (and no where else?)
> 
> This case in particular is already checked in the netlink parsing code on
> create/update.
> I was gonna delete the condition initially but then thought of hiding it
> under an unlikely branch.
> As for the other branches, I didn't see much of a reason.

TBH, I'd drop the unlikely() unless there is some performance data.
Perhaps you can drop the log entirely, can it occur given the checking
elsewhere?

> > 
> > > +			pr_info_ratelimited("tc action pedit bad header type specified (0x%x)\n", htype);
> > >   			goto bad;
> > >   		}
> > 
> > ...
>