netdev - Re: [Intel-wired-lan] [iwl-next v1 1/2] ice: tc: check src

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30416589-7340-4ad3-8749-bef1f82743cb@molgen.mpg.de>
Date: Tue, 20 Feb 2024 12:23:11 +0100
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Cc: wojciech.drewek@...el.com, marcin.szycik@...el.com,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 Jedrzej Jagielski <jedrzej.jagielski@...el.com>, sridhar.samudrala@...el.com
Subject: Re: [Intel-wired-lan] [iwl-next v1 1/2] ice: tc: check src_vsi in
 case of traffic from VF

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@...el.com>
> Reviewed-by: Sridhar Samudrala <sridhar.samudrala@...el.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.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