* [PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags
2024-04-23 10:27 [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() Asbjørn Sloth Tønnesen
@ 2024-04-23 10:27 ` Asbjørn Sloth Tønnesen
2024-04-23 11:16 ` Daniel Machon
2024-04-23 11:15 ` [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() Daniel Machon
2024-04-23 11:22 ` Jiri Pirko
2 siblings, 1 reply; 7+ messages in thread
From: Asbjørn Sloth Tønnesen @ 2024-04-23 10:27 UTC (permalink / raw)
To: netdev
Cc: Asbjørn Sloth Tønnesen, Steen Hegelund, Lars Povlsen,
Daniel Machon, UNGLinuxDriver, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-kernel, Jacob Keller
Use flow_rule_is_supp_control_flags() to reject filters with
unsupported control flags.
In case any unsupported control flags are masked,
flow_rule_is_supp_control_flags() sets a NL extended
error message, and we return -EOPNOTSUPP.
Only compile-tested.
Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
---
drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
index d846edd77a01..f81d89f8f620 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
@@ -197,6 +197,11 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
}
}
+ if (!flow_rule_is_supp_control_flags(FLOW_DIS_IS_FRAGMENT |
+ FLOW_DIS_FIRST_FRAG,
+ mt.mask->flags, extack))
+ return -EOPNOTSUPP;
+
st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL);
return err;
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags
2024-04-23 10:27 ` [PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags Asbjørn Sloth Tønnesen
@ 2024-04-23 11:16 ` Daniel Machon
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Machon @ 2024-04-23 11:16 UTC (permalink / raw)
To: Asbjørn Sloth Tønnesen
Cc: netdev, Steen Hegelund, Lars Povlsen, UNGLinuxDriver,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, Jacob Keller
> Use flow_rule_is_supp_control_flags() to reject filters with
> unsupported control flags.
>
> In case any unsupported control flags are masked,
> flow_rule_is_supp_control_flags() sets a NL extended
> error message, and we return -EOPNOTSUPP.
>
> Only compile-tested.
>
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
> drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> index d846edd77a01..f81d89f8f620 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> @@ -197,6 +197,11 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
> }
> }
>
> + if (!flow_rule_is_supp_control_flags(FLOW_DIS_IS_FRAGMENT |
> + FLOW_DIS_FIRST_FRAG,
> + mt.mask->flags, extack))
> + return -EOPNOTSUPP;
> +
> st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL);
>
> return err;
> --
> 2.43.0
As mentioned in patch #1, use supp_flags here. Otherwise looks
good.
/Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()
2024-04-23 10:27 [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() Asbjørn Sloth Tønnesen
2024-04-23 10:27 ` [PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags Asbjørn Sloth Tønnesen
@ 2024-04-23 11:15 ` Daniel Machon
2024-04-23 16:25 ` Asbjørn Sloth Tønnesen
2024-04-23 11:22 ` Jiri Pirko
2 siblings, 1 reply; 7+ messages in thread
From: Daniel Machon @ 2024-04-23 11:15 UTC (permalink / raw)
To: Asbjørn Sloth Tønnesen
Cc: netdev, Steen Hegelund, Lars Povlsen, UNGLinuxDriver,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, Jacob Keller
Hi Asbjørn,
Thank you for your patch!
> Define extack locally, to reduce line lengths and future users.
>
> Only perform fragment handling, when at least one fragment flag is set.
>
> Remove goto, as it's only used once, and the error message is specific
> to that context.
>
> Only compile tested.
>
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
> .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> index 663571fe7b2d..d846edd77a01 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> @@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st)
> static int
> sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
> {
> + struct netlink_ext_ack *extack = st->fco->common.extack;
Could you please update the use of extack in all places inside this
function. You are missing one place.
> struct flow_match_control mt;
> u32 value, mask;
> int err = 0;
>
> flow_rule_match_control(st->frule, &mt);
>
> - if (mt.mask->flags) {
> + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) {
Since these flags are used here and in the next patch, maybe assign them
to a variable:
u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG
And update the use throughout.
> u8 is_frag_key = !!(mt.key->flags & FLOW_DIS_IS_FRAGMENT);
> u8 is_frag_mask = !!(mt.mask->flags & FLOW_DIS_IS_FRAGMENT);
> u8 is_frag_idx = (is_frag_key << 1) | is_frag_mask;
> @@ -190,17 +191,15 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
> err = vcap_rule_add_key_u32(st->vrule,
> VCAP_KF_L3_FRAGMENT_TYPE,
> value, mask);
> - if (err)
> - goto out;
> + if (err) {
> + NL_SET_ERR_MSG_MOD(extack, "ip_frag parse error");
> + return err;
> + }
> }
>
> st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL);
>
> return err;
> -
> -out:
> - NL_SET_ERR_MSG_MOD(st->fco->common.extack, "ip_frag parse error");
> - return err;
> }
>
> static int
> --
> 2.43.0
Also I think you missing a cover letter for this series.
I will run the patches through our tests once the issues are addressed.
/Daniel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()
2024-04-23 11:15 ` [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() Daniel Machon
@ 2024-04-23 16:25 ` Asbjørn Sloth Tønnesen
2024-04-23 19:15 ` Daniel Machon
0 siblings, 1 reply; 7+ messages in thread
From: Asbjørn Sloth Tønnesen @ 2024-04-23 16:25 UTC (permalink / raw)
To: Daniel Machon
Cc: netdev, Steen Hegelund, Lars Povlsen, UNGLinuxDriver,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, Jacob Keller
Hi Daniel,
Thank you for the review.
On 4/23/24 11:15 AM, Daniel Machon wrote:
> Hi Asbjørn,
>
> Thank you for your patch!
>
>> Define extack locally, to reduce line lengths and future users.
>>
>> Only perform fragment handling, when at least one fragment flag is set.
>>
>> Remove goto, as it's only used once, and the error message is specific
>> to that context.
>>
>> Only compile tested.
>>
>> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>> ---
>> .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
>> index 663571fe7b2d..d846edd77a01 100644
>> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
>> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
>> @@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st)
>> static int
>> sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
>> {
>> + struct netlink_ext_ack *extack = st->fco->common.extack;
>
> Could you please update the use of extack in all places inside this
> function. You are missing one place.
Good catch, sure. It must have got lost somewhere along the way. I deliberately kept it out
of the net patch, since it could wait for net-next.
>> struct flow_match_control mt;
>> u32 value, mask;
>> int err = 0;
>>
>> flow_rule_match_control(st->frule, &mt);
>>
>> - if (mt.mask->flags) {
>> + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) {
>
> Since these flags are used here and in the next patch, maybe assign them
> to a variable:
>
> u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG
>
> And update the use throughout.
In an earlier state this patch had a #define SPARX5_FLOWER_SUPPORTED_CTLFLAGS,
in the same style as nfp in drivers/net/ethernet/netronome/nfp/flower/offload.c
Right now, this driver supports all currently defined flags (which are used with mask),
so the point of using flow_rule_is_supp_control_flags() to this dirver, is to
make it possible to introduce new flags in the future, without having to update
all drivers to explicitly not support a new flag.
My problem with using supp_flags in both places is: What happens when support
for a new flag is introduced?
u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG | FLOW_DIS_NEW_FLAG;
if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG))
/* handle fragment flags through lookup table */
if (mt.mask->flags & FLOW_DIS_NEW_FLAG)
/* do something */
if (!flow_rule_is_supp_control_flags(supp_flags, mt.mask->flags, extack))
return -EOPNOTSUPP;
The fragment lookup table code currently requires the above guarding,
as [0][0] in the lookup table is FRAG_INVAL, and not FRAG_SHRUG.
What do you think?
--
Best regards
Asbjørn Sloth Tønnesen
Network Engineer
Fiberby - AS42541
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()
2024-04-23 16:25 ` Asbjørn Sloth Tønnesen
@ 2024-04-23 19:15 ` Daniel Machon
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Machon @ 2024-04-23 19:15 UTC (permalink / raw)
To: Asbjørn Sloth Tønnesen
Cc: netdev, Steen Hegelund, Lars Povlsen, UNGLinuxDriver,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel, Jacob Keller
> > > - if (mt.mask->flags) {
> > > + if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) {
> >
> > Since these flags are used here and in the next patch, maybe assign them
> > to a variable:
> >
> > u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG
> >
> > And update the use throughout.
>
> In an earlier state this patch had a #define SPARX5_FLOWER_SUPPORTED_CTLFLAGS,
> in the same style as nfp in drivers/net/ethernet/netronome/nfp/flower/offload.c
>
> Right now, this driver supports all currently defined flags (which are used with mask),
> so the point of using flow_rule_is_supp_control_flags() to this dirver, is to
> make it possible to introduce new flags in the future, without having to update
> all drivers to explicitly not support a new flag.
>
> My problem with using supp_flags in both places is: What happens when support
> for a new flag is introduced?
>
> u32 supp_flags = FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG | FLOW_DIS_NEW_FLAG;
>
> if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG))
> /* handle fragment flags through lookup table */
>
> if (mt.mask->flags & FLOW_DIS_NEW_FLAG)
> /* do something */
>
> if (!flow_rule_is_supp_control_flags(supp_flags, mt.mask->flags, extack))
> return -EOPNOTSUPP;
>
> The fragment lookup table code currently requires the above guarding,
> as [0][0] in the lookup table is FRAG_INVAL, and not FRAG_SHRUG.
>
> What do you think?
Yes - lets only check for fragment flags when doing the lookup. I am
fine with your original impl.
If you can fix the remaining issue, I will take the patches for a test
spin tomorrow.
Thanks!
>
> --
> Best regards
> Asbjørn Sloth Tønnesen
> Network Engineer
> Fiberby - AS42541
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage()
2024-04-23 10:27 [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() Asbjørn Sloth Tønnesen
2024-04-23 10:27 ` [PATCH net-next 2/2] net: sparx5: flower: check for unsupported control flags Asbjørn Sloth Tønnesen
2024-04-23 11:15 ` [PATCH net-next 1/2] net: sparx5: flower: cleanup sparx5_tc_flower_handler_control_usage() Daniel Machon
@ 2024-04-23 11:22 ` Jiri Pirko
2 siblings, 0 replies; 7+ messages in thread
From: Jiri Pirko @ 2024-04-23 11:22 UTC (permalink / raw)
To: Asbjørn Sloth Tønnesen
Cc: netdev, Steen Hegelund, Lars Povlsen, Daniel Machon,
UNGLinuxDriver, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel, Jacob Keller
Tue, Apr 23, 2024 at 12:27:26PM CEST, ast@fiberby.net wrote:
>Define extack locally, to reduce line lengths and future users.
>
>Only perform fragment handling, when at least one fragment flag is set.
>
>Remove goto, as it's only used once, and the error message is specific
>to that context.
3 changes, 3 patches?
>
>Only compile tested.
>
>Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
>---
> .../ethernet/microchip/sparx5/sparx5_tc_flower.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
>index 663571fe7b2d..d846edd77a01 100644
>--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
>+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
>@@ -159,13 +159,14 @@ sparx5_tc_flower_handler_basic_usage(struct vcap_tc_flower_parse_usage *st)
> static int
> sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
> {
>+ struct netlink_ext_ack *extack = st->fco->common.extack;
> struct flow_match_control mt;
> u32 value, mask;
> int err = 0;
>
> flow_rule_match_control(st->frule, &mt);
>
>- if (mt.mask->flags) {
>+ if (mt.mask->flags & (FLOW_DIS_IS_FRAGMENT | FLOW_DIS_FIRST_FRAG)) {
> u8 is_frag_key = !!(mt.key->flags & FLOW_DIS_IS_FRAGMENT);
> u8 is_frag_mask = !!(mt.mask->flags & FLOW_DIS_IS_FRAGMENT);
> u8 is_frag_idx = (is_frag_key << 1) | is_frag_mask;
>@@ -190,17 +191,15 @@ sparx5_tc_flower_handler_control_usage(struct vcap_tc_flower_parse_usage *st)
> err = vcap_rule_add_key_u32(st->vrule,
> VCAP_KF_L3_FRAGMENT_TYPE,
> value, mask);
>- if (err)
>- goto out;
>+ if (err) {
>+ NL_SET_ERR_MSG_MOD(extack, "ip_frag parse error");
>+ return err;
>+ }
> }
>
> st->used_keys |= BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL);
>
> return err;
>-
>-out:
>- NL_SET_ERR_MSG_MOD(st->fco->common.extack, "ip_frag parse error");
>- return err;
> }
>
> static int
>--
>2.43.0
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread