| Message ID | 20240408172738.96447-1-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: sparx5: flower: fix fragment flags handling | expand |
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
> 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). Ran VCAP test-suites - seems good. Thanks! Tested-by: Daniel Machon <daniel.machon@microchip.com>
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 */ +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 } +}; + 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);
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). 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(-)