* [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied
@ 2026-09-19 9:51 Jamal Hadi Salim
2026-09-21 1:11 ` Xin Long
2026-09-22 0:52 ` netdev-bot+sashiko
0 siblings, 2 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2026-09-19 9:51 UTC (permalink / raw)
To: netdev
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Xin Long, stable,
Sashiko, Victor Nogueira, hybris
Commit 292207809486 ("net: sched: fix erspan_opt settings in cls_flower")
removed the unconditional memset of the erspan_metadata mask in
fl_set_erspan_opt(). On the mask-parse pass, when userspace supplies the
encap option key but no mask (TCA_FLOWER_KEY_ENC_OPTS without
TCA_FLOWER_KEY_ENC_OPTS_MASK), the !depth early return leaves the mask
zeroed -- a wildcard. A filter keyed on an ERSPAN index (v1), or dir/hwid
(v2), then matches every ERSPAN packet's values instead of exact-matching.
Mirror the with-mask per-version defaults on the no-mask path, keyed off
the key's own version: v1 index to 0xff, v2 dir and hwid to their
exact-match masks. The mask cannot just memset the whole union -- that
would re-mask the overlapping timestamp/sgt bytes of the v2 header that
the fix (292207809486) deliberately left to 0 in erspan_opt.
This is a follow-up to commit fee10655709c ("net/sched: cls_flower:
validate mask pointer after nla_next()").
Conditions to recreate the bug: add an ERSPAN encap-opts flower filter
with a key but no mask (raw netlink, TCA_FLOWER_KEY_ENC_OPTS present,
TCA_FLOWER_KEY_ENC_OPTS_MASK absent); dump the filter and observe the
match mask is zero (wildcard) instead of exact for the supplied fields.
Fixes: 292207809486 ("net: sched: fix erspan_opt settings in cls_flower")
Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826025123.62758-1-ljp1205831794@gmail.com
Link: https://lore.kernel.org/netdev/20260826025123.62758-1-ljp1205831794@gmail.com/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Tested-by: hybris <hybris@mojatatu.ai>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/cls_flower.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 1cefea571efd..f5acdeb0e2a6 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1358,17 +1358,29 @@ static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key,
static int fl_set_erspan_opt(const struct nlattr *nla, struct fl_flow_key *key,
int depth, int option_len,
- struct netlink_ext_ack *extack)
+ struct netlink_ext_ack *extack, u8 key_ver)
{
struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_MAX + 1];
struct erspan_metadata *md;
int err;
md = (struct erspan_metadata *)&key->enc_opts.data[key->enc_opts.len];
- md->version = 1;
+ md->version = key_ver;
- if (!depth)
+ if (!depth) {
+ /* No mask attribute supplied: exact-match every field the
+ * key set, keyed off the key's own version so the v2 dir/hwid
+ * bytes are masked without touching the overlapping
+ * timestamp/sgt bytes of the union (see 292207809486).
+ */
+ if (md->version == 1) {
+ memset(&md->u.index, 0xff, sizeof(md->u.index));
+ } else if (md->version == 2) {
+ md->u.md2.dir = 1;
+ set_hwid(&md->u.md2, 0xff);
+ }
return sizeof(*md);
+ }
if (nla_type(nla) != TCA_FLOWER_KEY_ENC_OPTS_ERSPAN) {
NL_SET_ERR_MSG(extack, "Non-erspan option type for mask");
@@ -1509,6 +1521,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
{
const struct nlattr *nla_enc_key, *nla_opt_key, *nla_opt_msk = NULL;
int err, option_len, key_depth, msk_depth = 0;
+ u8 key_ver = 1;
err = nla_validate_nested_deprecated(tb[TCA_FLOWER_KEY_ENC_OPTS],
TCA_FLOWER_KEY_ENC_OPTS_MAX,
@@ -1607,9 +1620,11 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
key->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT;
option_len = fl_set_erspan_opt(nla_opt_key, key,
key_depth, option_len,
- extack);
+ extack, 1);
if (option_len < 0)
return option_len;
+ key_ver = ((struct erspan_metadata *)
+ &key->enc_opts.data[key->enc_opts.len])->version;
key->enc_opts.len += option_len;
/* At the same time we need to parse through the mask
@@ -1618,7 +1633,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key,
mask->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT;
option_len = fl_set_erspan_opt(nla_opt_msk, mask,
msk_depth, option_len,
- extack);
+ extack, key_ver);
if (option_len < 0)
return option_len;
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied 2026-09-19 9:51 [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied Jamal Hadi Salim @ 2026-09-21 1:11 ` Xin Long 2026-09-21 17:58 ` Jamal Hadi Salim 2026-09-22 0:52 ` netdev-bot+sashiko 1 sibling, 1 reply; 5+ messages in thread From: Xin Long @ 2026-09-21 1:11 UTC (permalink / raw) To: Jamal Hadi Salim Cc: netdev, Jiri Pirko, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, stable, Sashiko, Victor Nogueira, hybris On Sat, Sep 19, 2026 at 5:51 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > Commit 292207809486 ("net: sched: fix erspan_opt settings in cls_flower") > removed the unconditional memset of the erspan_metadata mask in > fl_set_erspan_opt(). On the mask-parse pass, when userspace supplies the > encap option key but no mask (TCA_FLOWER_KEY_ENC_OPTS without > TCA_FLOWER_KEY_ENC_OPTS_MASK), the !depth early return leaves the mask > zeroed -- a wildcard. A filter keyed on an ERSPAN index (v1), or dir/hwid > (v2), then matches every ERSPAN packet's values instead of exact-matching. > > Mirror the with-mask per-version defaults on the no-mask path, keyed off > the key's own version: v1 index to 0xff, v2 dir and hwid to their > exact-match masks. The mask cannot just memset the whole union -- that > would re-mask the overlapping timestamp/sgt bytes of the v2 header that > the fix (292207809486) deliberately left to 0 in erspan_opt. > > This is a follow-up to commit fee10655709c ("net/sched: cls_flower: > validate mask pointer after nla_next()"). > > Conditions to recreate the bug: add an ERSPAN encap-opts flower filter > with a key but no mask (raw netlink, TCA_FLOWER_KEY_ENC_OPTS present, > TCA_FLOWER_KEY_ENC_OPTS_MASK absent); dump the filter and observe the > match mask is zero (wildcard) instead of exact for the supplied fields. > > Fixes: 292207809486 ("net: sched: fix erspan_opt settings in cls_flower") > Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org> > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826025123.62758-1-ljp1205831794@gmail.com > Link: https://lore.kernel.org/netdev/20260826025123.62758-1-ljp1205831794@gmail.com/ > Reviewed-by: Victor Nogueira <victor@mojatatu.com> > Tested-by: hybris <hybris@mojatatu.ai> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> > --- > net/sched/cls_flower.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index 1cefea571efd..f5acdeb0e2a6 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -1358,17 +1358,29 @@ static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > static int fl_set_erspan_opt(const struct nlattr *nla, struct fl_flow_key *key, > int depth, int option_len, > - struct netlink_ext_ack *extack) > + struct netlink_ext_ack *extack, u8 key_ver) > { > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_MAX + 1]; > struct erspan_metadata *md; > int err; > > md = (struct erspan_metadata *)&key->enc_opts.data[key->enc_opts.len]; > - md->version = 1; > + md->version = key_ver; > > - if (!depth) > + if (!depth) { > + /* No mask attribute supplied: exact-match every field the > + * key set, keyed off the key's own version so the v2 dir/hwid > + * bytes are masked without touching the overlapping > + * timestamp/sgt bytes of the union (see 292207809486). > + */ > + if (md->version == 1) { > + memset(&md->u.index, 0xff, sizeof(md->u.index)); > + } else if (md->version == 2) { > + md->u.md2.dir = 1; > + set_hwid(&md->u.md2, 0xff); > + } > return sizeof(*md); > + } > > if (nla_type(nla) != TCA_FLOWER_KEY_ENC_OPTS_ERSPAN) { > NL_SET_ERR_MSG(extack, "Non-erspan option type for mask"); > @@ -1509,6 +1521,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > { > const struct nlattr *nla_enc_key, *nla_opt_key, *nla_opt_msk = NULL; > int err, option_len, key_depth, msk_depth = 0; > + u8 key_ver = 1; > > err = nla_validate_nested_deprecated(tb[TCA_FLOWER_KEY_ENC_OPTS], > TCA_FLOWER_KEY_ENC_OPTS_MAX, > @@ -1607,9 +1620,11 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > key->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > option_len = fl_set_erspan_opt(nla_opt_key, key, > key_depth, option_len, > - extack); > + extack, 1); > if (option_len < 0) > return option_len; > + key_ver = ((struct erspan_metadata *) > + &key->enc_opts.data[key->enc_opts.len])->version; Do you think it would be better to pass &key_ver to fl_set_erspan_opt() and set it there, avoiding the dereference of erspan_metadata here? Thanks. > > key->enc_opts.len += option_len; > /* At the same time we need to parse through the mask > @@ -1618,7 +1633,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > mask->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > option_len = fl_set_erspan_opt(nla_opt_msk, mask, > msk_depth, option_len, > - extack); > + extack, key_ver); > if (option_len < 0) > return option_len; > > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied 2026-09-21 1:11 ` Xin Long @ 2026-09-21 17:58 ` Jamal Hadi Salim 0 siblings, 0 replies; 5+ messages in thread From: Jamal Hadi Salim @ 2026-09-21 17:58 UTC (permalink / raw) To: Xin Long Cc: netdev, Jiri Pirko, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, stable, Sashiko, Victor Nogueira, hybris On Sun, Sep 20, 2026 at 9:11 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Sat, Sep 19, 2026 at 5:51 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > > Commit 292207809486 ("net: sched: fix erspan_opt settings in cls_flower") > > removed the unconditional memset of the erspan_metadata mask in > > fl_set_erspan_opt(). On the mask-parse pass, when userspace supplies the > > encap option key but no mask (TCA_FLOWER_KEY_ENC_OPTS without > > TCA_FLOWER_KEY_ENC_OPTS_MASK), the !depth early return leaves the mask > > zeroed -- a wildcard. A filter keyed on an ERSPAN index (v1), or dir/hwid > > (v2), then matches every ERSPAN packet's values instead of exact-matching. > > > > Mirror the with-mask per-version defaults on the no-mask path, keyed off > > the key's own version: v1 index to 0xff, v2 dir and hwid to their > > exact-match masks. The mask cannot just memset the whole union -- that > > would re-mask the overlapping timestamp/sgt bytes of the v2 header that > > the fix (292207809486) deliberately left to 0 in erspan_opt. > > > > This is a follow-up to commit fee10655709c ("net/sched: cls_flower: > > validate mask pointer after nla_next()"). > > > > Conditions to recreate the bug: add an ERSPAN encap-opts flower filter > > with a key but no mask (raw netlink, TCA_FLOWER_KEY_ENC_OPTS present, > > TCA_FLOWER_KEY_ENC_OPTS_MASK absent); dump the filter and observe the > > match mask is zero (wildcard) instead of exact for the supplied fields. > > > > Fixes: 292207809486 ("net: sched: fix erspan_opt settings in cls_flower") > > Reported-by: Sashiko (gemini) <sashiko-bot@kernel.org> > > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826025123.62758-1-ljp1205831794@gmail.com > > Link: https://lore.kernel.org/netdev/20260826025123.62758-1-ljp1205831794@gmail.com/ > > Reviewed-by: Victor Nogueira <victor@mojatatu.com> > > Tested-by: hybris <hybris@mojatatu.ai> > > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com> > > --- > > net/sched/cls_flower.c | 25 ++++++++++++++++++++----- > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > > index 1cefea571efd..f5acdeb0e2a6 100644 > > --- a/net/sched/cls_flower.c > > +++ b/net/sched/cls_flower.c > > @@ -1358,17 +1358,29 @@ static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > > > static int fl_set_erspan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > int depth, int option_len, > > - struct netlink_ext_ack *extack) > > + struct netlink_ext_ack *extack, u8 key_ver) > > { > > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_MAX + 1]; > > struct erspan_metadata *md; > > int err; > > > > md = (struct erspan_metadata *)&key->enc_opts.data[key->enc_opts.len]; > > - md->version = 1; > > + md->version = key_ver; > > > > - if (!depth) > > + if (!depth) { > > + /* No mask attribute supplied: exact-match every field the > > + * key set, keyed off the key's own version so the v2 dir/hwid > > + * bytes are masked without touching the overlapping > > + * timestamp/sgt bytes of the union (see 292207809486). > > + */ > > + if (md->version == 1) { > > + memset(&md->u.index, 0xff, sizeof(md->u.index)); > > + } else if (md->version == 2) { > > + md->u.md2.dir = 1; > > + set_hwid(&md->u.md2, 0xff); > > + } > > return sizeof(*md); > > + } > > > > if (nla_type(nla) != TCA_FLOWER_KEY_ENC_OPTS_ERSPAN) { > > NL_SET_ERR_MSG(extack, "Non-erspan option type for mask"); > > @@ -1509,6 +1521,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > > { > > const struct nlattr *nla_enc_key, *nla_opt_key, *nla_opt_msk = NULL; > > int err, option_len, key_depth, msk_depth = 0; > > + u8 key_ver = 1; > > > > err = nla_validate_nested_deprecated(tb[TCA_FLOWER_KEY_ENC_OPTS], > > TCA_FLOWER_KEY_ENC_OPTS_MAX, > > @@ -1607,9 +1620,11 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > > key->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > > option_len = fl_set_erspan_opt(nla_opt_key, key, > > key_depth, option_len, > > - extack); > > + extack, 1); > > if (option_len < 0) > > return option_len; > > + key_ver = ((struct erspan_metadata *) > > + &key->enc_opts.data[key->enc_opts.len])->version; > Do you think it would be better to pass &key_ver to fl_set_erspan_opt() and > set it there, avoiding the dereference of erspan_metadata here? > Sure - that will avoid some casting. I could change it if i have to resend. Let's wait to hear from sashiko-nipa. I have gotten sashiko-gemini feedback but it was spewing some new (meaningless) stuff about things it was claiming then dismissing ;-> So nothing useful there. cheers, jamal > Thanks. > > > > > key->enc_opts.len += option_len; > > /* At the same time we need to parse through the mask > > @@ -1618,7 +1633,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > > mask->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > > option_len = fl_set_erspan_opt(nla_opt_msk, mask, > > msk_depth, option_len, > > - extack); > > + extack, key_ver); > > if (option_len < 0) > > return option_len; > > > > -- > > 2.43.0 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied 2026-09-19 9:51 [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied Jamal Hadi Salim 2026-09-21 1:11 ` Xin Long @ 2026-09-22 0:52 ` netdev-bot+sashiko 2026-09-24 11:34 ` Jamal Hadi Salim 1 sibling, 1 reply; 5+ messages in thread From: netdev-bot+sashiko @ 2026-09-22 0:52 UTC (permalink / raw) To: jhs Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, lucien.xin, stable, sashiko-bot, victor, hybris Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 2 · Low: 0 (2) If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] Undisclosed behaviour change on the *with-mask* ERSPAN parse path. - [Medium] The patch threads the key's ERSPAN version into the mask parse as a *default* only and never validates it, so a mask nest may still… Pre-existing issues: - [Low] The selftest match_erspan_opts_test() in tools/testing/selftests/net/forwarding/tc_flower.sh (lines 679-726)… - [Low] The flower ERSPAN index attribute is consumed as big-endian but declared as a host-endian u32 in both of its declarations. -- net/sched: cls_flower: exact-match ERSPAN key when no mask supplied This patch makes fl_set_erspan_opt() fill in per-version exact-match mask defaults when userspace supplies TCA_FLOWER_KEY_ENC_OPTS without TCA_FLOWER_KEY_ENC_OPTS_MASK, instead of leaving the mask blob zeroed. To do so it threads the key blob's ERSPAN version into the mask parse pass via a new key_ver argument. Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826025123.62758-1-ljp1205831794@gmail.com Link: https://lore.kernel.org/netdev/20260826025123.62758-1-ljp1205831794@gmail.com/ A few questions below. > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > index 1cefea571efd..f5acdeb0e2a6 100644 > --- a/net/sched/cls_flower.c > +++ b/net/sched/cls_flower.c > @@ -1358,17 +1358,29 @@ static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > static int fl_set_erspan_opt(const struct nlattr *nla, struct fl_flow_key *key, > int depth, int option_len, > - struct netlink_ext_ack *extack) > + struct netlink_ext_ack *extack, u8 key_ver) > { > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_MAX + 1]; > struct erspan_metadata *md; > int err; > > md = (struct erspan_metadata *)&key->enc_opts.data[key->enc_opts.len]; > - md->version = 1; > + md->version = key_ver; [Severity: Medium] Is the with-mask path also meant to change here? The assignment sits above the if (!depth) block, so it runs on the mask pass too, not only on the no-mask path the changelog and the new comment describe. On the mask pass option_len is the non-zero value returned by the key pass (sizeof(struct erspan_metadata)), so in fl_set_erspan_opt() this guard never fires: if (!option_len && !tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER]) { NL_SET_ERR_MSG(extack, "Missing tunnel key erspan option ver"); return -EINVAL; } A mask nest that omits TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER is therefore accepted. Before this patch such a mask always defaulted to version 1 and took the v1 branch (index bytes memset to 0xff, dir/hwid left wildcarded, mask blob version 1). With a v2 key it now defaults to 2 and takes the v2 branch instead (md2.dir = 1, set_hwid(0xff), index bytes left at 0). That changes which packets match, what fl_dump_key_erspan_opt() emits for TCA_FLOWER_KEY_ENC_OPTS_MASK (it selects the union member from the mask blob's own md->version), and the mask handed to hardware offload, for requests that installed fine before. Would it make sense to restrict the new default to the !depth path, or alternatively to describe this second affected path in the changelog and move the comment so its scope matches the code? > - if (!depth) > + if (!depth) { > + /* No mask attribute supplied: exact-match every field the > + * key set, keyed off the key's own version so the v2 dir/hwid > + * bytes are masked without touching the overlapping > + * timestamp/sgt bytes of the union (see 292207809486). > + */ > + if (md->version == 1) { > + memset(&md->u.index, 0xff, sizeof(md->u.index)); > + } else if (md->version == 2) { > + md->u.md2.dir = 1; > + set_hwid(&md->u.md2, 0xff); > + } > return sizeof(*md); > + } > > if (nla_type(nla) != TCA_FLOWER_KEY_ENC_OPTS_ERSPAN) { > NL_SET_ERR_MSG(extack, "Non-erspan option type for mask"); [ ... ] > @@ -1607,9 +1620,11 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > key->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > option_len = fl_set_erspan_opt(nla_opt_key, key, > key_depth, option_len, > - extack); > + extack, 1); > if (option_len < 0) > return option_len; > + key_ver = ((struct erspan_metadata *) > + &key->enc_opts.data[key->enc_opts.len])->version; > > key->enc_opts.len += option_len; > /* At the same time we need to parse through the mask > @@ -1618,7 +1633,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > mask->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > option_len = fl_set_erspan_opt(nla_opt_msk, mask, > msk_depth, option_len, > - extack); > + extack, key_ver); > if (option_len < 0) > return option_len; [Severity: Medium] Now that key_ver is available at the call site, should a mask nest that declares a different version than the key be rejected? key_ver only supplies the default; a few lines further down in fl_set_erspan_opt() it is overwritten with no comparison against the key's version: if (tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER]) md->version = nla_get_u8(tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER]); The only cross-check in fl_set_enc_opt() is the length one, and both version branches return sizeof(*md): mask->enc_opts.len += option_len; if (key->enc_opts.len != mask->enc_opts.len) { So key nest {VER=1, INDEX=0x64} with mask nest {VER=2} is accepted. On the mask pass option_len is non-zero, so the "Missing tunnel key erspan option dir or hwid" guard is skipped, and the v2 branch writes only md2.dir and the hwid bitfields, leaving the union bytes holding the key's index at 0. cls_flower compares (packet_key & mask) against (filter_key & mask), so doesn't that filter then match every ERSPAN packet regardless of index, which is the wildcard behaviour this patch sets out to remove? The mirror case, key VER=2 with dir/hwid plus mask VER=1, memsets the union bytes that alias md2.timestamp to 0xff while dir/hwid stay wildcarded, so the filter would never match packets carrying a non-zero ERSPAN timestamp. The pair is also not round-trippable, since fl_dump_key_erspan_opt() picks the union member from each blob's own md->version: the key dumps as VER=1 plus INDEX while the mask dumps as VER=2 plus DIR/HWID. Two remaining observations that are not about this diff: [Severity: Low] This isn't a bug introduced by this patch, but match_erspan_opts_test() in tools/testing/selftests/net/forwarding/tc_flower.sh does not appear to verify that the ERSPAN option values take part in matching, so it passes with or without the wildcard mask this patch fixes. The Type II filter (handle 101) is installed with enc_key_id 1001 plus erspan_opts 1:6789:0:0, and the Type III filter (handle 102) with enc_key_id 1002 plus erspan_opts 2:0:1:63. The distinct tunnel key IDs already select the respective filter, and only one matching packet is sent through each tunnel: $MZ erspan1 -c 1 -p 64 -a $ep1mac -b $h2mac -t ip -q tc_check_packets "dev ep-ex ingress" 101 1 check_err $? "ERSPAN Type II" Both assertions still hold when the ERSPAN index/dir/hwid mask bytes are zero. Could a negative packet with the same enc_key_id but different ERSPAN metadata be added, so the new no-mask defaults in fl_set_erspan_opt() are covered? [Severity: Low] This is a pre-existing issue, but the ERSPAN index attribute is consumed as big-endian while both of its declarations call it a host-endian u32. include/uapi/linux/pkt_cls.h documents TCA_FLOWER_KEY_ENC_OPT_ERSPAN_INDEX as be32, fl_set_erspan_opt() reads it with nla_get_be32() into the __be32 union member and fl_dump_key_erspan_opt() emits it with nla_put_be32(). But erspan_opt_policy has: [TCA_FLOWER_KEY_ENC_OPT_ERSPAN_INDEX] = { .type = NLA_U32 }, and the flower-key-enc-opt-erspan-attrs entry in Documentation/netlink/specs/tc.yaml has: - name: index type: u32 with no byte-order: big-endian. Should the policy use NLA_BE32 and the spec entry declare big-endian byte order so both match the ABI? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-4CSL.v1.20260917072652%40mojatatu.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied 2026-09-22 0:52 ` netdev-bot+sashiko @ 2026-09-24 11:34 ` Jamal Hadi Salim 0 siblings, 0 replies; 5+ messages in thread From: Jamal Hadi Salim @ 2026-09-24 11:34 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, lucien.xin, stable, sashiko-bot, victor, hybris On Mon, Sep 21, 2026 at 8:52 PM <netdev-bot+sashiko@kernel.org> wrote: > > Thank you for your contribution! Sashiko AI review found 2 potential > issue(s) to consider. > > Critical: 0 · High: 0 · Medium: 2 · Low: 0 (2) > > If you believe none of the issues are legitimate, please reply to each > issue of Medium severity or higher and explain why the AI is wrong. > If any of them are legitimate and you plan to respin, please let patchwork > know by including "pw-bot: cr" as a separate line at the end of your reply > (one such reply per series is enough). > > - [Medium] Undisclosed behaviour change on the *with-mask* ERSPAN parse > path. > - [Medium] The patch threads the key's ERSPAN version into the mask parse > as a *default* only and never validates it, so a mask nest may still… > > Pre-existing issues: > - [Low] The selftest match_erspan_opts_test() in > tools/testing/selftests/net/forwarding/tc_flower.sh (lines 679-726)… > - [Low] The flower ERSPAN index attribute is consumed as big-endian but > declared as a host-endian u32 in both of its declarations. > pw-bot: cr cheers, jamal > -- > > net/sched: cls_flower: exact-match ERSPAN key when no mask supplied > > This patch makes fl_set_erspan_opt() fill in per-version exact-match mask > defaults when userspace supplies TCA_FLOWER_KEY_ENC_OPTS without > TCA_FLOWER_KEY_ENC_OPTS_MASK, instead of leaving the mask blob zeroed. > To do so it threads the key blob's ERSPAN version into the mask parse pass > via a new key_ver argument. > > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260826025123.62758-1-ljp1205831794@gmail.com > Link: https://lore.kernel.org/netdev/20260826025123.62758-1-ljp1205831794@gmail.com/ > > A few questions below. > > > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c > > index 1cefea571efd..f5acdeb0e2a6 100644 > > --- a/net/sched/cls_flower.c > > +++ b/net/sched/cls_flower.c > > @@ -1358,17 +1358,29 @@ static int fl_set_vxlan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > > > static int fl_set_erspan_opt(const struct nlattr *nla, struct fl_flow_key *key, > > int depth, int option_len, > > - struct netlink_ext_ack *extack) > > + struct netlink_ext_ack *extack, u8 key_ver) > > { > > struct nlattr *tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_MAX + 1]; > > struct erspan_metadata *md; > > int err; > > > > md = (struct erspan_metadata *)&key->enc_opts.data[key->enc_opts.len]; > > - md->version = 1; > > + md->version = key_ver; > > [Severity: Medium] > Is the with-mask path also meant to change here? The assignment sits > above the if (!depth) block, so it runs on the mask pass too, not only > on the no-mask path the changelog and the new comment describe. > > On the mask pass option_len is the non-zero value returned by the key > pass (sizeof(struct erspan_metadata)), so in fl_set_erspan_opt() this > guard never fires: > > if (!option_len && !tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER]) { > NL_SET_ERR_MSG(extack, "Missing tunnel key erspan option ver"); > return -EINVAL; > } > > A mask nest that omits TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER is therefore > accepted. Before this patch such a mask always defaulted to version 1 > and took the v1 branch (index bytes memset to 0xff, dir/hwid left > wildcarded, mask blob version 1). With a v2 key it now defaults to 2 > and takes the v2 branch instead (md2.dir = 1, set_hwid(0xff), index > bytes left at 0). > > That changes which packets match, what fl_dump_key_erspan_opt() emits > for TCA_FLOWER_KEY_ENC_OPTS_MASK (it selects the union member from the > mask blob's own md->version), and the mask handed to hardware offload, > for requests that installed fine before. > > Would it make sense to restrict the new default to the !depth path, or > alternatively to describe this second affected path in the changelog and > move the comment so its scope matches the code? > > > - if (!depth) > > + if (!depth) { > > + /* No mask attribute supplied: exact-match every field the > > + * key set, keyed off the key's own version so the v2 dir/hwid > > + * bytes are masked without touching the overlapping > > + * timestamp/sgt bytes of the union (see 292207809486). > > + */ > > + if (md->version == 1) { > > + memset(&md->u.index, 0xff, sizeof(md->u.index)); > > + } else if (md->version == 2) { > > + md->u.md2.dir = 1; > > + set_hwid(&md->u.md2, 0xff); > > + } > > return sizeof(*md); > > + } > > > > if (nla_type(nla) != TCA_FLOWER_KEY_ENC_OPTS_ERSPAN) { > > NL_SET_ERR_MSG(extack, "Non-erspan option type for mask"); > > [ ... ] > > > @@ -1607,9 +1620,11 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > > key->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > > option_len = fl_set_erspan_opt(nla_opt_key, key, > > key_depth, option_len, > > - extack); > > + extack, 1); > > if (option_len < 0) > > return option_len; > > + key_ver = ((struct erspan_metadata *) > > + &key->enc_opts.data[key->enc_opts.len])->version; > > > > key->enc_opts.len += option_len; > > /* At the same time we need to parse through the mask > > @@ -1618,7 +1633,7 @@ static int fl_set_enc_opt(struct nlattr **tb, struct fl_flow_key *key, > > mask->enc_opts.dst_opt_type = IP_TUNNEL_ERSPAN_OPT_BIT; > > option_len = fl_set_erspan_opt(nla_opt_msk, mask, > > msk_depth, option_len, > > - extack); > > + extack, key_ver); > > if (option_len < 0) > > return option_len; > > [Severity: Medium] > Now that key_ver is available at the call site, should a mask nest that > declares a different version than the key be rejected? > > key_ver only supplies the default; a few lines further down in > fl_set_erspan_opt() it is overwritten with no comparison against the > key's version: > > if (tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER]) > md->version = nla_get_u8(tb[TCA_FLOWER_KEY_ENC_OPT_ERSPAN_VER]); > > The only cross-check in fl_set_enc_opt() is the length one, and both > version branches return sizeof(*md): > > mask->enc_opts.len += option_len; > if (key->enc_opts.len != mask->enc_opts.len) { > > So key nest {VER=1, INDEX=0x64} with mask nest {VER=2} is accepted. On > the mask pass option_len is non-zero, so the "Missing tunnel key erspan > option dir or hwid" guard is skipped, and the v2 branch writes only > md2.dir and the hwid bitfields, leaving the union bytes holding the > key's index at 0. cls_flower compares (packet_key & mask) against > (filter_key & mask), so doesn't that filter then match every ERSPAN > packet regardless of index, which is the wildcard behaviour this patch > sets out to remove? > > The mirror case, key VER=2 with dir/hwid plus mask VER=1, memsets the > union bytes that alias md2.timestamp to 0xff while dir/hwid stay > wildcarded, so the filter would never match packets carrying a non-zero > ERSPAN timestamp. > > The pair is also not round-trippable, since fl_dump_key_erspan_opt() > picks the union member from each blob's own md->version: the key dumps > as VER=1 plus INDEX while the mask dumps as VER=2 plus DIR/HWID. > > Two remaining observations that are not about this diff: > > [Severity: Low] > This isn't a bug introduced by this patch, but match_erspan_opts_test() > in tools/testing/selftests/net/forwarding/tc_flower.sh does not appear > to verify that the ERSPAN option values take part in matching, so it > passes with or without the wildcard mask this patch fixes. > > The Type II filter (handle 101) is installed with enc_key_id 1001 plus > erspan_opts 1:6789:0:0, and the Type III filter (handle 102) with > enc_key_id 1002 plus erspan_opts 2:0:1:63. The distinct tunnel key IDs > already select the respective filter, and only one matching packet is > sent through each tunnel: > > $MZ erspan1 -c 1 -p 64 -a $ep1mac -b $h2mac -t ip -q > tc_check_packets "dev ep-ex ingress" 101 1 > check_err $? "ERSPAN Type II" > > Both assertions still hold when the ERSPAN index/dir/hwid mask bytes are > zero. Could a negative packet with the same enc_key_id but different > ERSPAN metadata be added, so the new no-mask defaults in > fl_set_erspan_opt() are covered? > > [Severity: Low] > This is a pre-existing issue, but the ERSPAN index attribute is > consumed as big-endian while both of its declarations call it a > host-endian u32. > > include/uapi/linux/pkt_cls.h documents > TCA_FLOWER_KEY_ENC_OPT_ERSPAN_INDEX as be32, > fl_set_erspan_opt() reads it with nla_get_be32() into the __be32 union > member and fl_dump_key_erspan_opt() emits it with nla_put_be32(). But > erspan_opt_policy has: > > [TCA_FLOWER_KEY_ENC_OPT_ERSPAN_INDEX] = { .type = NLA_U32 }, > > and the flower-key-enc-opt-erspan-attrs entry in > Documentation/netlink/specs/tc.yaml has: > > - > name: index > type: u32 > > with no byte-order: big-endian. Should the policy use NLA_BE32 and the > spec entry declare big-endian byte order so both match the ABI? > > -- > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-4CSL.v1.20260917072652%40mojatatu.com ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-24 11:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-19 9:51 [PATCH net] net/sched: cls_flower: exact-match ERSPAN key when no mask supplied Jamal Hadi Salim 2026-09-21 1:11 ` Xin Long 2026-09-21 17:58 ` Jamal Hadi Salim 2026-09-22 0:52 ` netdev-bot+sashiko 2026-09-24 11:34 ` Jamal Hadi Salim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox