netdev - Re: [PATCH iwl-next 1/1] ice: Fix persistent failure in ice_get

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <0dd3d9d5-2e44-4a6b-a8f6-d997a979e128@intel.com>
Date: Fri, 5 Dec 2025 11:36:34 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Cody Haas <chaas@...tgames.com>
CC: <anthony.l.nguyen@...el.com>, <andrew+netdev@...n.ch>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	<intel-wired-lan@...ts.osuosl.org>
Subject: Re: [PATCH iwl-next 1/1] ice: Fix persistent failure in ice_get_rxfh

On 12/3/25 19:49, Cody Haas wrote:
> Several ioctl functions have the ability to call ice_get_rxfh, however
> all of these iotctl functions do not provide all of the expected
> information in ethtool_rxfh_param. For example,ethtool_get_rxfh_indir does
> not provide an rss_key. Previously, caused ethtool_get_rxfh_indir to
> always fail with an -EINVAL.
> 
> This change draws inspiration from i40e_get_rss to handle this
> situation, by only calling the appropriate rss helpers when the
> necessary information has been provided via ethtool_rxfh_param.
> 
> Signed-off-by: Cody Haas <chaas@...tgames.com>
> Closes: https://lore.kernel.org/intel-wired-lan/CAH7f-UKkJV8MLY7zCdgCrGE55whRhbGAXvgkDnwgiZ9gUZT7_w@mail.gmail.com/

thank you again for the report, and thank you for the fix, it looks good
just some little nitpicks from me:

1. this is a bugfix, so you should add a Fixes: tag with the commit that
added the regression (I remember you have a "slow to rebuild" platform,
so just let me know how far you have reached with bisection/looking for
the root cause)
2. bugfixes should have [PATCH iwl-net] in the Subject
3. you should CC netdev mailing list on IWL submissions too:
	netdev@...r.kernel.org
4. minor thing in the code, see below

> 
> ---
>   drivers/net/ethernet/intel/ice/ice.h         |  1 +
>   drivers/net/ethernet/intel/ice/ice_ethtool.c |  6 +----
>   drivers/net/ethernet/intel/ice/ice_main.c    | 28 ++++++++++++++++++++
>   3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index c9104b13e1d2..87f4098324ed 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -953,6 +953,7 @@ void ice_map_xdp_rings(struct ice_vsi *vsi);
>   int
>   ice_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
>   	     u32 flags);
> +int ice_get_rss(struct ice_vsi *vsi, u8 *seed, u8 *lut, u16 lut_size);
>   int ice_set_rss_lut(struct ice_vsi *vsi, u8 *lut, u16 lut_size);
>   int ice_get_rss_lut(struct ice_vsi *vsi, u8 *lut, u16 lut_size);
>   int ice_set_rss_key(struct ice_vsi *vsi, u8 *seed);
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index b0805704834d..a5c139cc536d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3649,11 +3649,7 @@ ice_get_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh)
>   	if (!lut)
>   		return -ENOMEM;
>   
> -	err = ice_get_rss_key(vsi, rxfh->key);
> -	if (err)
> -		goto out;
> -
> -	err = ice_get_rss_lut(vsi, lut, vsi->rss_table_size);
> +	err = ice_get_rss(vsi, rxfh->key, lut, vsi->rss_table_size);
>   	if (err)
>   		goto out;
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index b084839eb811..7b409b9fca5c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -8072,6 +8072,34 @@ int ice_get_rss_key(struct ice_vsi *vsi, u8 *seed)
>   	return status;
>   }
>   
> +/**
> + * ice_get_rss - Get RSS LUT and/or key
> + * @vsi: Pointer to VSI structure
> + * @seed: Buffer to store the key in
> + * @lut: Buffer to store the lookup table entries
> + * @lut_size: Size of buffer to store the lookup table entries
> + *
> + * Returns 0 on success, negative on failure
> + */
> +int ice_get_rss(struct ice_vsi *vsi, u8 *seed, u8 *lut, u16 lut_size)
> +{
> +	int status = 0;
> +
> +	if (lut) {
> +		status = ice_get_rss_lut(vsi, lut, lut_size);
> +		if (status)
> +			return status;
> +	}
> +
> +	if (seed) {
> +		status = ice_get_rss_key(vsi, seed);
> +		if (status)
> +			return status;
> +	}
> +
> +	return status;

nit: you could simply "return 0" here
then the status variable initialization during declaration could be
removed

yet another thing: for new code I would name such variable "err"


> +}
> +
>   /**
>    * ice_set_rss_hfunc - Set RSS HASH function
>    * @vsi: Pointer to VSI structure