netdev - Re: [External] : Re: [PATCH net] net: ena: fix duplicate Autoneg setting in get_link

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]

Message-ID: <2cad7a63-56e9-409b-90b1-69dfe73358c7@oracle.com>
Date: Thu, 11 Sep 2025 22:48:05 +0530
From: ALOK TIWARI <alok.a.tiwari@...cle.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: shayagr@...zon.com, akiyano@...zon.com, saeedb@...zon.com,
        darinzon@...zon.com, andrew+netdev@...n.ch, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        horms@...nel.org, netdev@...r.kernel.org
Subject: Re: [External] : Re: [PATCH net] net: ena: fix duplicate Autoneg
 setting in get_link_ksettings


Thanks Andrew,

On 9/11/2025 7:57 PM, Andrew Lunn wrote:
>> The ENA ethtool implementation mistakenly sets the Autoneg link mode
>> twice in the 'supported' mask, leaving the 'advertising mask unset.
> These are not masks. They are bitfields.

You are right, these are not masks, they are bitfields.

> 
>> Fix this by setting Autoneg in 'advertising' instead of duplicating
>> it in 'supported'.
>>
>> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
>> Signed-off-by: Alok Tiwari<alok.a.tiwari@...cle.com>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_ethtool.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
>> index a81d3a7a3bb9..a6ef12c157ca 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
>> @@ -471,7 +471,7 @@ static int ena_get_link_ksettings(struct net_device *netdev,
>>   		ethtool_link_ksettings_add_link_mode(link_ksettings,
>>   						     supported, Autoneg);
>>   		ethtool_link_ksettings_add_link_mode(link_ksettings,
>> -						     supported, Autoneg);
>> +						     advertising, Autoneg);
> While i agree the current code looks wrong, i'm not convinced your
> change is correct.
> 
> What does ENA_ADMIN_GET_FEATURE_LINK_DESC_AUTONEG_MASK mean?
> 
> That the firmware support autoneg? If so, setting the bit in supported
> makes sense. But does it mean it is actually enabled? If its not
> enabled, you should not set it in advertising.
> 
> However, if we assume the firmware always supports autoneg, but
> ENA_ADMIN_GET_FEATURE_LINK_DESC_AUTONEG_MASK indicates it is enabled,
> supported should always have the bit set, and advertising should be
> set based on this flag.
> 
> 	Andrew

Thanks Andrew, for pointing this out.
You are right the distinction between supported and advertising (as 
runtime state) was not properly reflected in my earlier patch.

 From my understanding of ENA_ADMIN_GET_FEATURE_LINK_DESC_AUTONEG_MASK:
- It indicates whether autonegotiation is enabled.
- ENA devices always support autoneg, so supported should always have 
the bit set unconditionally.
- advertising should be set only if the flag is present,  since that 
reflects  runtime enablement.

So we have two possible approaches:
1. Explicitly set supported unconditionally and gate advertising on the 
flag:

/* Autoneg is always supported */
ethtool_link_ksettings_add_link_mode(link_ksettings, supported, Autoneg);

if (link->flags & ENA_ADMIN_GET_FEATURE_LINK_DESC_AUTONEG_MASK)
         ethtool_link_ksettings_add_link_mode(link_ksettings, 
advertising, Autoneg);

2. Leave the logic unchanged but remove the redundant assignment to
supported to avoid confusion. (send to [net-next] + no Fixes tag)

If this approach looks correct to you, I’ll prepare a v2 with the 
preferred fix.

Thanks,
Alok