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 <[email protected]>
---
.../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 */
+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);
--
2.43.0
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
> [email protected]. 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 <[email protected]>
> ---
> .../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 <[email protected]>
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 <[email protected]>