[PATCH net-next] ethtool: Clarify len/n_stats fields in/out semantics

[PATCH net-next] ethtool: Clarify len/n_stats fields in/out semantics

[Gal Pressman]

Document that the 'len' field in ethtool_gstrings and 'n_stats' field in
ethtool_stats serve dual purposes: on entry they specify the number of
items requested, and on return they indicate the number actually
returned (which is not necessarily the same).

Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
 include/uapi/linux/ethtool.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index eb7ff2602fbb..f66adf15a3c8 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1094,7 +1094,8 @@ enum ethtool_module_fw_flash_status {
  * struct ethtool_gstrings - string set for data tagging
  * @cmd: Command number = %ETHTOOL_GSTRINGS
  * @string_set: String set ID; one of &enum ethtool_stringset
- * @len: On return, the number of strings in the string set
+ * @len: On entry, the number of strings requested.
+ *	On return, the number of strings returned.
  * @data: Buffer for strings.  Each string is null-padded to a size of
  *	%ETH_GSTRING_LEN.
  *
@@ -1177,7 +1178,8 @@ struct ethtool_test {
 /**
  * struct ethtool_stats - device-specific statistics
  * @cmd: Command number = %ETHTOOL_GSTATS
- * @n_stats: On return, the number of statistics
+ * @n_stats: On entry, the number of stats requested.
+	On return, the number of stats returned.
  * @data: Array of statistics
  *
  * Users must use %ETHTOOL_GSSET_INFO or %ETHTOOL_GDRVINFO to find the
-- 
2.40.1

Re: [PATCH net-next] ethtool: Clarify len/n_stats fields in/out semantics

[Jakub Kicinski]

On Mon, 5 Jan 2026 18:39:23 +0200 Gal Pressman wrote:
> - * @n_stats: On return, the number of statistics
> + * @n_stats: On entry, the number of stats requested.
> +	On return, the number of stats returned.
>   * @data: Array of statistics

Missing a '*'
But stepping back we should rephrase the comment to cover both
directions instead of mechanically adding the corresponding "On entry"

FTR my recollection was that we never validated these field on entry and
if that's the case 7b07be1ff1cb6 is quite questionable, uAPI-breakage
wise.
-- 
pw-bot: cr

Re: [PATCH net-next] ethtool: Clarify len/n_stats fields in/out semantics

[Gal Pressman]

On 07/01/2026 3:48, Jakub Kicinski wrote:
> On Mon, 5 Jan 2026 18:39:23 +0200 Gal Pressman wrote:
>> - * @n_stats: On return, the number of statistics
>> + * @n_stats: On entry, the number of stats requested.
>> +	On return, the number of stats returned.
>>   * @data: Array of statistics
> 
> Missing a '*'

Ah, missed it, thanks!

> But stepping back we should rephrase the comment to cover both
> directions instead of mechanically adding the corresponding "On entry"

What do you mean?
How would you phrase it?

> 
> FTR my recollection was that we never validated these field on entry and
> if that's the case 7b07be1ff1cb6 is quite questionable, uAPI-breakage
> wise.

Can you describe the breakage please?

The kernel didn't look at this field on entry, but AFAICT, it was passed
from userspace since the beginning of time.

As a precaution, the cited patch only looks at the input values if
they're different than zero, so theoretical apps that didn't fill them
shouldn't be affected.

Maybe if the app deliberately put a wrong length value on the input buffer?

Re: [PATCH net-next] ethtool: Clarify len/n_stats fields in/out semantics

[Jakub Kicinski]

On Wed, 7 Jan 2026 10:51:46 +0200 Gal Pressman wrote:
> On 07/01/2026 3:48, Jakub Kicinski wrote:
> > On Mon, 5 Jan 2026 18:39:23 +0200 Gal Pressman wrote:  
> >> - * @n_stats: On return, the number of statistics
> >> + * @n_stats: On entry, the number of stats requested.
> >> +	On return, the number of stats returned.
> >>   * @data: Array of statistics  
> > 
> > Missing a '*'  
> 
> Ah, missed it, thanks!
> 
> > But stepping back we should rephrase the comment to cover both
> > directions instead of mechanically adding the corresponding "On entry"  
> 
> What do you mean?
> How would you phrase it?

Maybe just "number of stats"? 

If you want you can (in the body of the doc) go into the detail that
setting the value on input is optional. And on output it will either 
be the number of stats reported or 0 if there's a mismatch?

> > FTR my recollection was that we never validated these field on entry and
> > if that's the case 7b07be1ff1cb6 is quite questionable, uAPI-breakage
> > wise.  
> 
> Can you describe the breakage please?
> 
> The kernel didn't look at this field on entry, but AFAICT, it was passed
> from userspace since the beginning of time.
> 
> As a precaution, the cited patch only looks at the input values if
> they're different than zero, so theoretical apps that didn't fill them
> shouldn't be affected.
> 
> Maybe if the app deliberately put a wrong length value on the input buffer?

Not deliberately, but there used to be nothing illegal about
malloc()'ing the area and only initializing cmd. n_stats was
clearly defined as output only, and zeroing out the buffer
was kinda pointless given that kernel was expected to override
the stats area immediately with data.

Don't think we need to revert the change now, let's see if anyone
complains (perhaps ethtool CLI is the main way people interact with
the stats?) But there have been LWN articles about this sort of "start
using an un-validate field" in the past. It's well understood to be
a no-no.

Re: [PATCH net-next] ethtool: Clarify len/n_stats fields in/out semantics

[Gal Pressman]

On 08/01/2026 4:39, Jakub Kicinski wrote:
> On Wed, 7 Jan 2026 10:51:46 +0200 Gal Pressman wrote:
>> On 07/01/2026 3:48, Jakub Kicinski wrote:
>>> On Mon, 5 Jan 2026 18:39:23 +0200 Gal Pressman wrote:  
>>>> - * @n_stats: On return, the number of statistics
>>>> + * @n_stats: On entry, the number of stats requested.
>>>> +	On return, the number of stats returned.
>>>>   * @data: Array of statistics  
>>>
>>> Missing a '*'  
>>
>> Ah, missed it, thanks!
>>
>>> But stepping back we should rephrase the comment to cover both
>>> directions instead of mechanically adding the corresponding "On entry"  
>>
>> What do you mean?
>> How would you phrase it?
> 
> Maybe just "number of stats"? 
> 
> If you want you can (in the body of the doc) go into the detail that
> setting the value on input is optional. And on output it will either 
> be the number of stats reported or 0 if there's a mismatch?

Will do.

> 
>>> FTR my recollection was that we never validated these field on entry and
>>> if that's the case 7b07be1ff1cb6 is quite questionable, uAPI-breakage
>>> wise.  
>>
>> Can you describe the breakage please?
>>
>> The kernel didn't look at this field on entry, but AFAICT, it was passed
>> from userspace since the beginning of time.
>>
>> As a precaution, the cited patch only looks at the input values if
>> they're different than zero, so theoretical apps that didn't fill them
>> shouldn't be affected.
>>
>> Maybe if the app deliberately put a wrong length value on the input buffer?
> 
> Not deliberately, but there used to be nothing illegal about
> malloc()'ing the area and only initializing cmd. n_stats was
> clearly defined as output only, and zeroing out the buffer
> was kinda pointless given that kernel was expected to override
> the stats area immediately with data.

Yes, passing an uninitialized buffer from userspace seems like bad
practice, but you're right.

> 
> Don't think we need to revert the change now, let's see if anyone
> complains (perhaps ethtool CLI is the main way people interact with
> the stats?) But there have been LWN articles about this sort of "start
> using an un-validate field" in the past. It's well understood to be
> a no-no.

I agree with this statement, but it's usually referring to not start
using unvalidated reserved fields.

In this case, since ethtool always used this value with input semantics,
it is slightly different.