| Message ID | 20240424134250.465904-1-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: qede: flower: validate control flags | expand |
Wed, Apr 24, 2024 at 03:42:48PM CEST, ast@fiberby.net wrote: >This driver currently doesn't support any flower control flags. > >Implement check for control flags, such as can be set through >`tc flower ... ip_flags frag`. > >Since qede_parse_flow_attr() are called by both qede_add_tc_flower_fltr() >and qede_flow_spec_to_rule(), as the latter doesn't having access to >extack, then flow_rule_*_control_flags() can't be used in this driver. Why? You can pass null. > >This patch therefore re-implements flow_rule_match_has_control_flags(), >but with error messaging via DP_NOTICE, instead of NL_SET_ERR_MSG_FMT_MOD. > >So in case any control flags are masked, we call DP_NOTICE() and >return -EOPNOTSUPP. > >Only compile-tested. > >Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >--- > >This is AFAICT the last driver which didn't validate these flags. > >$ git grep FLOW_DISSECTOR_KEY_CONTROL drivers/ >$ git grep 'flow_rule_.*_control_flags' drivers/ > > drivers/net/ethernet/qlogic/qede/qede_filter.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > >diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c >index a5ac21a0ee33..40f72e700d8e 100644 >--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c >+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c >@@ -1843,6 +1843,19 @@ qede_parse_flow_attr(struct qede_dev *edev, __be16 proto, > return -EPROTONOSUPPORT; > } > >+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) { >+ struct flow_match_control match; >+ >+ flow_rule_match_control(rule, &match); >+ >+ if (match.mask->flags) { >+ DP_NOTICE(edev, >+ "Unsupported match on control.flags %#x", >+ match.mask->flags); >+ return -EOPNOTSUPP; >+ } >+ } >+ > if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) { > struct flow_match_basic match; > >-- >2.43.0 > >
Hi Jiri, On 4/24/24 2:52 PM, Jiri Pirko wrote: > Wed, Apr 24, 2024 at 03:42:48PM CEST, ast@fiberby.net wrote: >> This driver currently doesn't support any flower control flags. >> >> Implement check for control flags, such as can be set through >> `tc flower ... ip_flags frag`. >> >> Since qede_parse_flow_attr() are called by both qede_add_tc_flower_fltr() >> and qede_flow_spec_to_rule(), as the latter doesn't having access to >> extack, then flow_rule_*_control_flags() can't be used in this driver. > > Why? You can pass null. Ah, I see. I hadn't traced that option down through the defines, I incorrectly assumed that NL_SET_ERR_MSG* didn't allow NULL. Currently thinking about doing v2 in this style: if (flow_rule_match_has_control_flags(rule, extack)) { if (!extack) DP_NOTICE(edev, "Unsupported match on control.flags"); return -EOPNOTSUPP; } pw-bot: changes-requested
Wed, Apr 24, 2024 at 06:43:14PM CEST, ast@fiberby.net wrote: >Hi Jiri, > >On 4/24/24 2:52 PM, Jiri Pirko wrote: >> Wed, Apr 24, 2024 at 03:42:48PM CEST, ast@fiberby.net wrote: >> > This driver currently doesn't support any flower control flags. >> > >> > Implement check for control flags, such as can be set through >> > `tc flower ... ip_flags frag`. >> > >> > Since qede_parse_flow_attr() are called by both qede_add_tc_flower_fltr() >> > and qede_flow_spec_to_rule(), as the latter doesn't having access to >> > extack, then flow_rule_*_control_flags() can't be used in this driver. >> >> Why? You can pass null. > >Ah, I see. I hadn't traced that option down through the defines, >I incorrectly assumed that NL_SET_ERR_MSG* didn't allow NULL. > >Currently thinking about doing v2 in this style: > >if (flow_rule_match_has_control_flags(rule, extack)) { > if (!extack) > DP_NOTICE(edev, "Unsupported match on control.flags"); > return -EOPNOTSUPP; >} Looks ok. > >pw-bot: changes-requested > >-- >Best regards >Asbjørn Sloth Tønnesen >Network Engineer >Fiberby - AS42541
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c index a5ac21a0ee33..40f72e700d8e 100644 --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c @@ -1843,6 +1843,19 @@ qede_parse_flow_attr(struct qede_dev *edev, __be16 proto, return -EPROTONOSUPPORT; } + if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) { + struct flow_match_control match; + + flow_rule_match_control(rule, &match); + + if (match.mask->flags) { + DP_NOTICE(edev, + "Unsupported match on control.flags %#x", + match.mask->flags); + return -EOPNOTSUPP; + } + } + if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) { struct flow_match_basic match;
This driver currently doesn't support any flower control flags. Implement check for control flags, such as can be set through `tc flower ... ip_flags frag`. Since qede_parse_flow_attr() are called by both qede_add_tc_flower_fltr() and qede_flow_spec_to_rule(), as the latter doesn't having access to extack, then flow_rule_*_control_flags() can't be used in this driver. This patch therefore re-implements flow_rule_match_has_control_flags(), but with error messaging via DP_NOTICE, instead of NL_SET_ERR_MSG_FMT_MOD. So in case any control flags are masked, we call DP_NOTICE() and return -EOPNOTSUPP. Only compile-tested. Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- This is AFAICT the last driver which didn't validate these flags. $ git grep FLOW_DISSECTOR_KEY_CONTROL drivers/ $ git grep 'flow_rule_.*_control_flags' drivers/ drivers/net/ethernet/qlogic/qede/qede_filter.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)