| Message ID | 20240410134303.21560-1-ast@fiberby.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] octeontx2-pf: fix FLOW_DIS_IS_FRAGMENT implementation | expand |
On 4/10/2024 6:43 AM, Asbjørn Sloth Tønnesen wrote: > Upon reviewing the flower control flags handling in > this driver, I notice that the key wasn't being used, > only the mask. > > Ie. `tc flower ... ip_flags nofrag` was hardware > offloaded as `... ip_flags frag`. > > Only compile tested, no access to HW. > > Fixes: c672e3727989 ("octeontx2-pf: Add support to filter packet based on IP fragment") > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> > --- > I know nothing about this hardware, but the change does match your description. Hopefully someone from Marvell can comment on the hardware aspect. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > This is a best guess on a fix, I don't know if it will work, > but hopefully someone at Marvell can test it. > > The more certain alternative would be to set an error msg. > and return -EOPNOTSUPP, when `tc flower ip_flags nofrag` > is used.
On Wed, 10 Apr 2024 13:43:01 +0000 Asbjørn Sloth Tønnesen wrote: > Upon reviewing the flower control flags handling in > this driver, I notice that the key wasn't being used, > only the mask. > > Ie. `tc flower ... ip_flags nofrag` was hardware > offloaded as `... ip_flags frag`. > > Only compile tested, no access to HW. > > Fixes: c672e3727989 ("octeontx2-pf: Add support to filter packet based on IP fragment") > Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> Marvell folks, please provide timely review. It is your duty as documented in Documentation/maintainer/feature-and-driver-maintainers.rst
>Subject: [EXTERNAL] [PATCH net] octeontx2-pf: fix FLOW_DIS_IS_FRAGMENT >implementation > >Prioritize security for external emails: Confirm sender and content safety >before clicking links or opening attachments > >---------------------------------------------------------------------- >Upon reviewing the flower control flags handling in this driver, I notice >that the key wasn't being used, only the mask. > >Ie. `tc flower ... ip_flags nofrag` was hardware offloaded as `... ip_flags >frag`. > >Only compile tested, no access to HW. > >Fixes: c672e3727989 ("octeontx2-pf: Add support to filter packet based on >IP fragment") >Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> >--- > >This is a best guess on a fix, I don't know if it will work, but hopefully >someone at Marvell can test it. > >The more certain alternative would be to set an error msg. >and return -EOPNOTSUPP, when `tc flower ip_flags nofrag` is used. > > drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c >b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c >index 87bdb93cb066e..f4655a8c0705d 100644 >--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c >+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c >@@ -688,22 +688,25 @@ static int otx2_tc_prepare_flow(struct otx2_nic *nic, >struct otx2_tc_flow *node, > } > > if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) { > struct flow_match_control match; >+ u32 val; > > flow_rule_match_control(rule, &match); > if (match.mask->flags & FLOW_DIS_FIRST_FRAG) { > NL_SET_ERR_MSG_MOD(extack, "HW doesn't support frag >first/later"); > return -EOPNOTSUPP; > } > > if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) { >+ val = match.key->flags & FLOW_DIS_IS_FRAGMENT; > if (ntohs(flow_spec->etype) == ETH_P_IP) { >- flow_spec->ip_flag = IPV4_FLAG_MORE; >+ flow_spec->ip_flag = val ? IPV4_FLAG_MORE : 0; [Suman] Do we need this? If user provide the command "tc filter add .... ip_flags nofrags" then the above if check should not be hit right? If we are inside the check then we always want to set IPV4_FLAG_MORE right? > flow_mask->ip_flag = IPV4_FLAG_MORE; > req->features |= BIT_ULL(NPC_IPFRAG_IPV4); > } else if (ntohs(flow_spec->etype) == ETH_P_IPV6) { >- flow_spec->next_header = IPPROTO_FRAGMENT; >+ flow_spec->next_header = val ? >+ IPPROTO_FRAGMENT : 0; > flow_mask->next_header = 0xff; > req->features |= BIT_ULL(NPC_IPFRAG_IPV6); > } else { > NL_SET_ERR_MSG_MOD(extack, "flow-type should be either >IPv4 and IPv6"); >-- >2.43.0
Hi Suman, On 4/12/24 5:34 AM, Suman Ghosh wrote: >> if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) { >> + val = match.key->flags & FLOW_DIS_IS_FRAGMENT; >> if (ntohs(flow_spec->etype) == ETH_P_IP) { >> - flow_spec->ip_flag = IPV4_FLAG_MORE; >> + flow_spec->ip_flag = val ? IPV4_FLAG_MORE : 0; > [Suman] Do we need this? If user provide the command "tc filter add .... ip_flags nofrags" then the above if check should not be hit right? If we are inside the check then we always want to set IPV4_FLAG_MORE right? In iproute2, the "frag"/"nofrag" is parsed in flower_parse_matching_flags(), it sets TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT in TCA_FLOWER_KEY_FLAGS and TCA_FLOWER_KEY_FLAGS_MASK. Back in the kernel, in fl_set_key_flags() (net/sched/cls_flower.c) then, directly translates TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT into FLOW_DIS_IS_FRAGMENT, while only setting the key bit, if the mask bit is being set. There are therefore 3 possible cases: - `tc flower ...` (no ip_flags frag or nofrag) (match.key->flags & FLOW_DIS_FIRST_FRAG) is false (match.mask->flags & FLOW_DIS_FIRST_FRAG) is false - `tc flower ... ip_flags nofrag` (match.key->flags & FLOW_DIS_FIRST_FRAG) is false (match.mask->flags & FLOW_DIS_FIRST_FRAG) is true - `tc flower ... ip_flags frag` (match.key->flags & FLOW_DIS_FIRST_FRAG) is true (match.mask->flags & FLOW_DIS_FIRST_FRAG) is true The `nofrag` case will still have the mask bit set, and hence pass the entry condition. >> flow_mask->ip_flag = IPV4_FLAG_MORE; Yes, you should always set IPV4_FLAG_MORE in flow_mask, but not always in flow_spec.
Hi again, On 4/12/24 9:01 AM, Asbjørn Sloth Tønnesen wrote: > There are therefore 3 possible cases: > > - `tc flower ...` (no ip_flags frag or nofrag) > (match.key->flags & FLOW_DIS_FIRST_FRAG) is false > (match.mask->flags & FLOW_DIS_FIRST_FRAG) is false > > - `tc flower ... ip_flags nofrag` > (match.key->flags & FLOW_DIS_FIRST_FRAG) is false > (match.mask->flags & FLOW_DIS_FIRST_FRAG) is true > > - `tc flower ... ip_flags frag` > (match.key->flags & FLOW_DIS_FIRST_FRAG) is true > (match.mask->flags & FLOW_DIS_FIRST_FRAG) is true I was a bit a hurry, to get the reply in before a meeting, so: s/FLOW_DIS_FIRST_FRAG/FLOW_DIS_IS_FRAGMENT/g There are therefore 3 possible cases: - `tc flower ...` (no ip_flags frag or nofrag) (match.key->flags & FLOW_DIS_IS_FRAGMENT) is false (match.mask->flags & FLOW_DIS_IS_FRAGMENT) is false - `tc flower ... ip_flags nofrag` (match.key->flags & FLOW_DIS_IS_FRAGMENT) is false (match.mask->flags & FLOW_DIS_IS_FRAGMENT) is true - `tc flower ... ip_flags frag` (match.key->flags & FLOW_DIS_IS_FRAGMENT) is true (match.mask->flags & FLOW_DIS_IS_FRAGMENT) is true
Hi maintainers, On 4/12/24 10:25 AM, Asbjørn Sloth Tønnesen wrote: > On 4/12/24 9:01 AM, Asbjørn Sloth Tønnesen wrote: > I was a bit in a hurry, to get the reply in before a meeting, > so: s/FLOW_DIS_FIRST_FRAG/FLOW_DIS_IS_FRAGMENT/g It was just a fix for my reply to Suman, so I am a bit confused why the patch itself got marked as "Changes Requested" in patchwork [1]. I can spin a v2, but the only change would currently be Jacob Keller's Reviewed-by. [1] https://patchwork.kernel.org/project/netdevbpf/patch/20240410134303.21560-1-ast@fiberby.net/
> -----Original Message----- > From: Asbjørn Sloth Tønnesen <ast@fiberby.net> > Sent: Friday, April 12, 2024 4:49 PM > To: David S. Miller <davem@davemloft.net>; Paolo Abeni > <pabeni@redhat.com>; Eric Dumazet <edumazet@google.com>; Jakub > Kicinski <kuba@kernel.org> > Cc: linux-kernel@vger.kernel.org; Sunil Kovvuri Goutham > <sgoutham@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>; > Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Hariprasad Kelam > <hkelam@marvell.com>; Suman Ghosh <sumang@marvell.com>; > netdev@vger.kernel.org > Subject: Re: [PATCH net] octeontx2-pf: fix > FLOW_DIS_IS_FRAGMENT implementation > ---------------------------------------------------------------------- > Hi maintainers, > > On 4/12/24 10:25 AM, Asbjørn Sloth Tønnesen wrote: > > On 4/12/24 9:01 AM, Asbjørn Sloth Tønnesen wrote: > > I was a bit in a hurry, to get the reply in before a meeting, > > so: s/FLOW_DIS_FIRST_FRAG/FLOW_DIS_IS_FRAGMENT/g > > It was just a fix for my reply to Suman, so I am a bit confused why the patch > itself got marked as "Changes Requested" in patchwork [1]. > > I can spin a v2, but the only change would currently be Jacob Keller's > Reviewed-by. > Patch looks okay to us. Please submit a v2, meanwhile we will test once and give a tested-by as well. Thanks, Sunil.
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c index 87bdb93cb066e..f4655a8c0705d 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c @@ -688,22 +688,25 @@ static int otx2_tc_prepare_flow(struct otx2_nic *nic, struct otx2_tc_flow *node, } if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) { struct flow_match_control match; + u32 val; flow_rule_match_control(rule, &match); if (match.mask->flags & FLOW_DIS_FIRST_FRAG) { NL_SET_ERR_MSG_MOD(extack, "HW doesn't support frag first/later"); return -EOPNOTSUPP; } if (match.mask->flags & FLOW_DIS_IS_FRAGMENT) { + val = match.key->flags & FLOW_DIS_IS_FRAGMENT; if (ntohs(flow_spec->etype) == ETH_P_IP) { - flow_spec->ip_flag = IPV4_FLAG_MORE; + flow_spec->ip_flag = val ? IPV4_FLAG_MORE : 0; flow_mask->ip_flag = IPV4_FLAG_MORE; req->features |= BIT_ULL(NPC_IPFRAG_IPV4); } else if (ntohs(flow_spec->etype) == ETH_P_IPV6) { - flow_spec->next_header = IPPROTO_FRAGMENT; + flow_spec->next_header = val ? + IPPROTO_FRAGMENT : 0; flow_mask->next_header = 0xff; req->features |= BIT_ULL(NPC_IPFRAG_IPV6); } else { NL_SET_ERR_MSG_MOD(extack, "flow-type should be either IPv4 and IPv6");
Upon reviewing the flower control flags handling in this driver, I notice that the key wasn't being used, only the mask. Ie. `tc flower ... ip_flags nofrag` was hardware offloaded as `... ip_flags frag`. Only compile tested, no access to HW. Fixes: c672e3727989 ("octeontx2-pf: Add support to filter packet based on IP fragment") Signed-off-by: Asbjørn Sloth Tønnesen <ast@fiberby.net> --- This is a best guess on a fix, I don't know if it will work, but hopefully someone at Marvell can test it. The more certain alternative would be to set an error msg. and return -EOPNOTSUPP, when `tc flower ip_flags nofrag` is used. drivers/net/ethernet/marvell/octeontx2/nic/otx2_tc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)