| Message ID | 20240410095224.6372-1-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: sparx5: flower: fix fragment flags handling | expand |
On 4/10/2024 2:52 AM, Asbjørn Sloth Tønnesen wrote: > 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. > Appreciate the detailed analysis with the lookup table. This is a bit more opaque but a maze of combinations would be even less readable and maintainable. Makes sense. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Hi Asbjørn, I know I am nitpicking here, but could you please sneak in below changes. > static int > sparx5_tc_flower_es0_tpid(struct vcap_tc_flower_parse_usage *st) > { > @@ -145,29 +166,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"); Please start this NL msg with a capital letter. All (AFAICS) other places in this file do this - nice to stay consistent. As a matter of fact, also do this to the new comments introduced. > + 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 > Checkpatch is producing a warning about the placement of the version information of the patch. Might as well fix this while at it. Thanks, /Daniel
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c index 523e0c470894f..f986850efdcc7 100644 --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c @@ -36,6 +36,27 @@ struct sparx5_tc_flower_template { u16 l3_proto; /* protocol specified in the template */ }; +/* 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 */ +static const u8 sparx5_vcap_frag_map[4][4] = { /* is_frag */ + { FRAG_INVAL, FRAG_INVAL, FRAG_INVAL, FRAG_FIRST }, /* 0/0 */ + { FRAG_NOT, FRAG_NOT, FRAG_INVAL, FRAG_INVAL }, /* 0/1 */ + { FRAG_INVAL, FRAG_INVAL, FRAG_INVAL, FRAG_INVAL }, /* 1/0 */ + { FRAG_SOME, FRAG_LATER, FRAG_INVAL, FRAG_FIRST } /* 1/1 */ + /* 0/0 0/1 1/0 1/1 <-- first_frag */ +}; + static int sparx5_tc_flower_es0_tpid(struct vcap_tc_flower_parse_usage *st) { @@ -145,29 +166,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);
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. 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). v2: - Improved in-line documentation of mapping table (requested by Steen) - Relocated enum and mapping table (requested by Steen) - Added Reviewed-by from Steen Hegelund - Added Tested-by from Daniel Machon v1: https://lore.kernel.org/netdev/20240408172738.96447-1-ast@fiberby.net/ 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> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com> Tested-by: Daniel Machon <daniel.machon@microchip.com> --- .../microchip/sparx5/sparx5_tc_flower.c | 61 ++++++++++++------- 1 file changed, 40 insertions(+), 21 deletions(-)