netdev - Re: [PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by

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

Message-ID: <6e6dc5e2-5af2-48a7-a017-0ebaebe989e8@amd.com>
Date: Mon, 14 Apr 2025 15:55:06 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: Andrew Lunn <andrew@...n.ch>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, brett.creeley@....com
Subject: Re: [PATCH net-next 2/3] ionic: support ethtool
 get_module_eeprom_by_page

On 4/14/2025 3:18 PM, Andrew Lunn wrote:
> 
>>>> +static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
>>>> +                                        const struct ethtool_module_eeprom *page_data,
>>>> +                                        struct netlink_ext_ack *extack)
> 
>>>> +     switch (page_data->page) {
>>>> +     case 0:
>>>> +             src = &idev->port_info->status.xcvr.sprom[page_data->offset];
>>>> +             break;
>>>> +     case 1:
>>>> +             src = &idev->port_info->sprom_page1[page_data->offset - 128];
>>>> +             break;
>>>> +     case 2:
>>>> +             src = &idev->port_info->sprom_page2[page_data->offset - 128];
>>>> +             break;
>>>> +     default:
>>>> +             return -EINVAL;
>>>
>>> It is a valid page, your firmware just does not support it. EOPNOSUPP.
>>
>> I can see the argument for this, but EINVAL seems to me to match the
>> out-of-bounds case from ionic_get_module_eprom(), as well as what other
>> drivers are reporting for an unsupported address.  It seems to me that
>> passing EOPNOSUPP back to the user is telling them that they can't get
>> eeprom data at all, not that they asked for the wrong page.
> 
> I would disagree with at. Look at the ethtool usage:
> 
> ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
>            [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]
> 
> You can ask for any page. The only validation that can be done is,
> does the page number fit within the page selection register. And that
> is a u8. So any value < 256 is valid for page.  Some pages are
> currently defined, some pages are reserved, and pages 128-255 are for
> vendor specific functions.
> 
> The limitation here is your firmware, you don't support what the
> specification allows. So EOPNOTSUPP for a page you don't supports
> would give an indication of this.
> 
> ethtool's pretty print should handle -EOPNOTSUPP. It knows some netdev
> have limits, and don't give full access to the module data. I would
> not be too surprised to find ethtool actually interprets EINVAL for a
> valid page to be fatal, but i've not checked. EOPNOTSUPP should just
> stop it pretty printing that section of the module.
> 
>          Andrew

I still think that EINVAL is the right answer because we are complaining 
about an argument value, not an operation, but sure, EOPNOTSUPP is fine. 
  I'll update the patchset in a day or two.

sln