diff mbox series

[iwl-next,v1,1/2] ice: tc: check src_vsi in case of traffic from VF

Message ID 20240220105950.6814-2-michal.swiatkowski@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ice: extend tc flower offload | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 940 this patch: 940
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 5 maintainers not CCed: pabeni@redhat.com jesse.brandeburg@intel.com kuba@kernel.org edumazet@google.com anthony.l.nguyen@intel.com
netdev/build_clang success Errors and warnings before: 957 this patch: 957
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 957 this patch: 957
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 26 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Michal Swiatkowski Feb. 20, 2024, 10:59 a.m. UTC 
In case of traffic going from the VF (so ingress for port representor)
there should be a check for source VSI. It is needed for hardware to not
match packets from different port with filters added on other port.

It is only for "from VF" traffic, because other traffic direction
doesn't have source VSI.

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Paul Menzel Feb. 20, 2024, 11:23 a.m. UTC | #1 
Dear Michal,


Thank you for the patch.

Am 20.02.24 um 11:59 schrieb Michal Swiatkowski:
> In case of traffic going from the VF (so ingress for port representor)
> there should be a check for source VSI. It is needed for hardware to not
> match packets from different port with filters added on other port.

… from different port*s* …?

> It is only for "from VF" traffic, because other traffic direction
> doesn't have source VSI.

Do you have a test case to reproduce this?

> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index b890410a2bc0..49ed5fd7db10 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> @@ -28,6 +28,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
>   	 * - ICE_TC_FLWR_FIELD_VLAN_TPID (present if specified)
>   	 * - Tunnel flag (present if tunnel)
>   	 */
> +	if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS)
> +		lkups_cnt++;

Why does the count variable need to be incremented?

>   	if (flags & ICE_TC_FLWR_FIELD_TENANT_ID)
>   		lkups_cnt++;
> @@ -363,6 +365,11 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
>   	/* Always add direction metadata */
>   	ice_rule_add_direction_metadata(&list[ICE_TC_METADATA_LKUP_IDX]);
>   
> +	if (tc_fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
> +		ice_rule_add_src_vsi_metadata(&list[i]);
> +		i++;
> +	}
> +
>   	rule_info->tun_type = ice_sw_type_from_tunnel(tc_fltr->tunnel_type);
>   	if (tc_fltr->tunnel_type != TNL_LAST) {
>   		i = ice_tc_fill_tunnel_outer(flags, tc_fltr, list, i);
> @@ -820,6 +827,7 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
>   
>   	/* specify the cookie as filter_rule_id */
>   	rule_info.fltr_rule_id = fltr->cookie;
> +	rule_info.src_vsi = vsi->idx;

Besides the comment above being redundant (as the code does exactly 
that), the new line looks like to belong to the comment. Please excuse 
my ignorance, but the commit message only talks about adding checks and 
not overwriting the `src_vsi`. It’d be great, if you could elaborate.

>   	ret = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, &rule_added);
>   	if (ret == -EEXIST) {


Kind regards,

Paul
Michal Swiatkowski Feb. 20, 2024, 12:24 p.m. UTC | #2 
On Tue, Feb 20, 2024 at 12:23:11PM +0100, Paul Menzel wrote:
> Dear Michal,
> 
> 
> Thank you for the patch.
>

Thanks for the review.

> Am 20.02.24 um 11:59 schrieb Michal Swiatkowski:
> > In case of traffic going from the VF (so ingress for port representor)
> > there should be a check for source VSI. It is needed for hardware to not
> > match packets from different port with filters added on other port.
> 
> … from different port*s* …?
> 

Will fix it.

> > It is only for "from VF" traffic, because other traffic direction
> > doesn't have source VSI.
> 
> Do you have a test case to reproduce this?
>

I can add tc fileter call in v2. In short, any redirect from VF0 to
uplink should allow going packets only from VF0, but currently it is
also matching traffic from other VFs (like VF1, VF2, etc.)

> > Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> > Reviewed-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > ---
> >   drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > index b890410a2bc0..49ed5fd7db10 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > @@ -28,6 +28,8 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
> >   	 * - ICE_TC_FLWR_FIELD_VLAN_TPID (present if specified)
> >   	 * - Tunnel flag (present if tunnel)
> >   	 */
> > +	if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS)
> > +		lkups_cnt++;
> 
> Why does the count variable need to be incremented?
>
AS you wrote belowe it is needed to add another lookup.

> >   	if (flags & ICE_TC_FLWR_FIELD_TENANT_ID)
> >   		lkups_cnt++;
> > @@ -363,6 +365,11 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
> >   	/* Always add direction metadata */
> >   	ice_rule_add_direction_metadata(&list[ICE_TC_METADATA_LKUP_IDX]);
> > +	if (tc_fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
> > +		ice_rule_add_src_vsi_metadata(&list[i]);
> > +		i++;
> > +	}
> > +
> >   	rule_info->tun_type = ice_sw_type_from_tunnel(tc_fltr->tunnel_type);
> >   	if (tc_fltr->tunnel_type != TNL_LAST) {
> >   		i = ice_tc_fill_tunnel_outer(flags, tc_fltr, list, i);
> > @@ -820,6 +827,7 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> >   	/* specify the cookie as filter_rule_id */
> >   	rule_info.fltr_rule_id = fltr->cookie;
> > +	rule_info.src_vsi = vsi->idx;
> 
> Besides the comment above being redundant (as the code does exactly that),
> the new line looks like to belong to the comment. Please excuse my
> ignorance, but the commit message only talks about adding checks and not
> overwriting the `src_vsi`. It’d be great, if you could elaborate.
>

I will rephrase commit message to mark that it is not checking in code,
but matching in hardware, thanks.

> >   	ret = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, &rule_added);
> >   	if (ret == -EEXIST) {
> 
> 
> Kind regards,
> 
> Paul
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index b890410a2bc0..49ed5fd7db10 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -28,6 +28,8 @@  ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 	 * - ICE_TC_FLWR_FIELD_VLAN_TPID (present if specified)
 	 * - Tunnel flag (present if tunnel)
 	 */
+	if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS)
+		lkups_cnt++;
 
 	if (flags & ICE_TC_FLWR_FIELD_TENANT_ID)
 		lkups_cnt++;
@@ -363,6 +365,11 @@  ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 	/* Always add direction metadata */
 	ice_rule_add_direction_metadata(&list[ICE_TC_METADATA_LKUP_IDX]);
 
+	if (tc_fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
+		ice_rule_add_src_vsi_metadata(&list[i]);
+		i++;
+	}
+
 	rule_info->tun_type = ice_sw_type_from_tunnel(tc_fltr->tunnel_type);
 	if (tc_fltr->tunnel_type != TNL_LAST) {
 		i = ice_tc_fill_tunnel_outer(flags, tc_fltr, list, i);
@@ -820,6 +827,7 @@  ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
 
 	/* specify the cookie as filter_rule_id */
 	rule_info.fltr_rule_id = fltr->cookie;
+	rule_info.src_vsi = vsi->idx;
 
 	ret = ice_add_adv_rule(hw, list, lkups_cnt, &rule_info, &rule_added);
 	if (ret == -EEXIST) {