Re: [PATCH net] net: sparx5: flower: fix fragment flags handling

[Steen Hegelund <steen.hegelund@microchip.com>]

Hi Asbjørn,


On Mon, 2024-04-08 at 17:27 +0000, Asbjørn Sloth Tønnesen wrote:
> [Some people who received this message don't often get email from
> ast@fiberby.net. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> I noticed that only 3 out of the 4 input bits were used,
> mt.key->flags & FLOW_DIS_IS_FRAGMENT was never checked.
> 
> In order to avoid a complicated maze, I converted it to
> use a 16 byte mapping table.
> 
> As shown in the table below the old heuristics doesn't
> always do the right thing, ie. when FLOW_DIS_IS_FRAGMENT=1/1
> then it used to only match follow-up fragment packets.
> 
> Here are all the combinations, and their resulting new/old
> VCAP key/mask filter:
> 
>   /- FLOW_DIS_IS_FRAGMENT (key/mask)
>   |    /- FLOW_DIS_FIRST_FRAG (key/mask)
>   |    |    /-- new VCAP fragment (key/mask)
>   v    v    v    v- old VCAP fragment (key/mask)
> 
>  0/0  0/0  -/-  -/-     impossible (due to entry cond. on mask)
>  0/0  0/1  -/-  0/3 !!  invalid (can't match non-fragment + follow-up
> frag)
>  0/0  1/0  -/-  -/-     impossible (key > mask)
>  0/0  1/1  1/3  1/3     first fragment
> 
>  0/1  0/0  0/3  3/3 !!  not fragmented
>  0/1  0/1  0/3  3/3 !!  not fragmented (+ not first fragment)
>  0/1  1/0  -/-  -/-     impossible (key > mask)
>  0/1  1/1  -/-  1/3 !!  invalid (non-fragment and first frag)
> 
>  1/0  0/0  -/-  -/-     impossible (key > mask)
>  1/0  0/1  -/-  -/-     impossible (key > mask)
>  1/0  1/0  -/-  -/-     impossible (key > mask)
>  1/0  1/1  -/-  -/-     impossible (key > mask)
> 
>  1/1  0/0  1/1  3/3 !!  some fragment
>  1/1  0/1  3/3  3/3     follow-up fragment
>  1/1  1/0  -/-  -/-     impossible (key > mask)
>  1/1  1/1  1/3  1/3     first fragment
> 
> In the datasheet the VCAP fragment values are documented as:
>  0 = no fragment
>  1 = initial fragment
>  2 = suspicious fragment
>  3 = valid follow-up fragment
> 
> Result: 3 combinations match the old behavior,
>         3 combinations have been corrected,
>         2 combinations are now invalid, and fail,
>         8 combinations are impossible.

Good work om mapping this out in detail.

> 
> It should now be aligned with how FLOW_DIS_IS_FRAGMENT
> and FLOW_DIS_FIRST_FRAG is set in __skb_flow_dissect() in
> net/core/flow_dissector.c
> 
> Since the VCAP fragment values are not a bitfield, we have
> to ignore the suspicious fragment value, eg. when matching
> on any kind of fragment with FLOW_DIS_IS_FRAGMENT=1/1.
> 
> Only compile tested, and logic tested in userspace, as I
> unfortunately don't have access to this switch chip (yet).
> 
> Fixes: d6c2964db3fe ("net: microchip: sparx5: Adding more tc flower
> keys for the IS2 VCAP")
> Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net>
> ---
>  .../microchip/sparx5/sparx5_tc_flower.c       | 60 ++++++++++++-----
> --
>  1 file changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> index 523e0c470894f..2f87ccb8cf8c8 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> @@ -135,6 +135,26 @@ sparx5_tc_flower_handler_basic_usage(struct
> vcap_tc_flower_parse_usage *st)
>         return err;
>  }
> 
> +/* SparX-5 VCAP fragment types:
> + * 0 = no fragment, 1 = initial fragment,
> + * 2 = suspicious fragment, 3 = valid follow-up fragment
> + */
> +enum {                   /* key / mask */
> +       FRAG_NOT   = 0x03, /* 0 / 3 */
> +       FRAG_SOME  = 0x11, /* 1 / 1 */
> +       FRAG_FIRST = 0x13, /* 1 / 3 */
> +       FRAG_LATER = 0x33, /* 3 / 3 */
> +       FRAG_INVAL = 0xff, /* invalid */
> +};
> +
> +/* Flower fragment flag to VCAP fragment type mapping */

Please add info about the x, y dimensions: (first_frag, fragged)

> +static const u8 sparx5_vcap_frag_map[4][4] = {
> +       { FRAG_INVAL, FRAG_INVAL, FRAG_INVAL, FRAG_FIRST },
> +       { FRAG_NOT,   FRAG_NOT,   FRAG_INVAL, FRAG_INVAL },
> +       { FRAG_INVAL, FRAG_INVAL, FRAG_INVAL, FRAG_INVAL },
> +       { FRAG_SOME,  FRAG_LATER, FRAG_INVAL, FRAG_FIRST }
> +};
> +


I would prefer the enums and table to be at the top of the file instead
of here.

>  static int
>  sparx5_tc_flower_handler_control_usage(struct
> vcap_tc_flower_parse_usage *st)
>  {
> @@ -145,29 +165,27 @@ sparx5_tc_flower_handler_control_usage(struct
> vcap_tc_flower_parse_usage *st)
>         flow_rule_match_control(st->frule, &mt);
> 
>         if (mt.mask->flags) {
> -               if (mt.mask->flags & FLOW_DIS_FIRST_FRAG) {
> -                       if (mt.key->flags & FLOW_DIS_FIRST_FRAG) {
> -                               value = 1; /* initial fragment */
> -                               mask = 0x3;
> -                       } else {
> -                               if (mt.mask->flags &
> FLOW_DIS_IS_FRAGMENT) {
> -                                       value = 3; /* follow up
> fragment */
> -                                       mask = 0x3;
> -                               } else {
> -                                       value = 0; /* no fragment */
> -                                       mask = 0x3;
> -                               }
> -                       }
> -               } else {
> -                       if (mt.mask->flags & FLOW_DIS_IS_FRAGMENT) {
> -                               value = 3; /* follow up fragment */
> -                               mask = 0x3;
> -                       } else {
> -                               value = 0; /* no fragment */
> -                               mask = 0x3;
> -                       }
> +               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;
> +
> +               u8 first_frag_key = !!(mt.key->flags &
> FLOW_DIS_FIRST_FRAG);
> +               u8 first_frag_mask = !!(mt.mask->flags &
> FLOW_DIS_FIRST_FRAG);
> +               u8 first_frag_idx = (first_frag_key << 1) |
> first_frag_mask;
> +
> +               /* lookup verdict based on the 2 + 2 input bits */
> +               u8 vdt =
> sparx5_vcap_frag_map[is_frag_idx][first_frag_idx];
> +
> +               if (vdt == FRAG_INVAL) {
> +                       NL_SET_ERR_MSG_MOD(st->fco->common.extack,
> +                                          "match on invalid fragment
> flag combination");
> +                       return -EINVAL;
>                 }
> 
> +               /* extract VCAP fragment key and mask from verdict */
> +               value = (vdt >> 4) & 0x3;
> +               mask = vdt & 0x3;
> +
>                 err = vcap_rule_add_key_u32(st->vrule,
>                                             VCAP_KF_L3_FRAGMENT_TYPE,
>                                             value, mask);
> --
> 2.43.0
> 

Thanks for the patch!

Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>

BR
Steen