netdev - Re: [Intel-wired-lan] [PATCH net-next v7 1/8] net: ethtool: pass a pointer to parameters to get/set

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 6 Dec 2023 13:57:13 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Ahmed Zaki <ahmed.zaki@...el.com>
CC: <netdev@...r.kernel.org>, <mkubecek@...e.cz>, <andrew@...n.ch>,
	<willemdebruijn.kernel@...il.com>, <corbet@....net>,
	<vladimir.oltean@....com>, <gal@...dia.com>, <linux-doc@...r.kernel.org>,
	<jesse.brandeburg@...el.com>, <ecree.xilinx@...il.com>,
	<edumazet@...gle.com>, <anthony.l.nguyen@...el.com>, <horms@...nel.org>,
	<kuba@...nel.org>, Jacob Keller <jacob.e.keller@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>, <pabeni@...hat.com>,
	<davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH net-next v7 1/8] net: ethtool: pass a
 pointer to parameters to get/set_rxfh ethtool ops

From: Ahmed Zaki <ahmed.zaki@...el.com>
Date: Tue,  5 Dec 2023 16:00:42 -0700

Duplicating my comment from the internal review:

> The get/set_rxfh ethtool ops currently takes the rxfh (RSS) parameters
> as direct function arguments. This will force us to change the API (and
> all drivers' functions) every time some new parameters are added.

[...]

> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index f7fba0dc87e5..f6e229e465b1 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1229,6 +1229,27 @@ struct ethtool_rxnfc {
>  	__u32				rule_locs[];
>  };
>  
> +/**
> + * struct ethtool_rxfh_param - RXFH (RSS) parameters
> + * @hfunc: Defines the current RSS hash function used by HW (or to be set to).
> + *	Valid values are one of the %ETH_RSS_HASH_*.
> + * @indir_size: On SET, the array size of the user buffer for the
> + *	indirection table, which may be zero, or
> + *	%ETH_RXFH_INDIR_NO_CHANGE.  On GET (read from the driver),
> + *	the array size of the hardware indirection table.
> + * @indir: The indirection table of size @indir_size entries.
> + * @key_size: On SET, the array size of the user buffer for the hash key,
> + *	which may be zero.  On GET (read from the driver), the size of the
> + *	hardware hash key.
> + * @key: The hash key of size @key_size bytes.
> + */
> +struct ethtool_rxfh_param {
> +	__u8	hfunc;
> +	__u32   indir_size;
> +	__u32	*indir;
> +	__u32   key_size;
> +	__u8	*key;
> +};
1. Why is this structure needed in UAPI? Do you plan to use it somewhere
   in userspace?
2. Kernel and userspace can't share pointers (as well as unsigned longs,
   size_ts, and so on) as you may run a 32-bit application on a 64-bit
   kernel.
3. Please never pass UAPI structures directly to the drivers, it's a bad
   idea and you may end up converting all those drivers once again when
   you'd need to to e.g. change the type of a field there. You won't be
   able to change the type in a UAPI structure.

Thanks,
Olek