linux-kernel - Re: [PATCH v7] igb: Assign random MAC address instead of fail in case of invalid one

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: <2D8AD99A-E29B-40CC-AFEC-3D9D4AC80C14@126.com>
Date:   Thu, 15 Dec 2022 11:24:54 +0800
From:   梁礼学 <lianglixuehao@....com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Leon Romanovsky <leon@...nel.org>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        linux-kernel@...r.kernel.org,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v7] igb: Assign random MAC address instead of fail in case
 of invalid one

The module parameter method does bring some inconvenience to the user, 
especially the parameter needs to be specified when the module is loaded. 
But as alexander said, if the net device is not successfully registered, 
the user has no chance to modify the invalid MAC address in the current EEPROM. 
At present, the read/write of EEPROM is bundled with the net driver. 
I am not sure if there is any other way to complete the modification of EEPROM 
independently of the network driver;

Is it necessary to bind the registration of net device to the judgment of invalid MAC?
I personally think that MAC configuration is not the capability or function of the device, 
this should not affect the registration of the device;
Can the invalid MAC be judged in the up stage of the network device? 
In this way, the net driver can continue to be loaded successfully, 
and the MAC can be changed using ethtool, and it will not increase the difficulty of debugging for users.

Thanks

> 2022年12月15日 07:17，Alexander Duyck <alexander.duyck@...il.com> 写道：
> 
> On Wed, Dec 14, 2022 at 1:43 PM Heiner Kallweit <hkallweit1@...il.com> wrote:
>> 
>> On 14.12.2022 21:50, Jakub Kicinski wrote:
>>> On Wed, 14 Dec 2022 20:53:30 +0200 Leon Romanovsky wrote:
>>>> On Wed, Dec 14, 2022 at 08:51:06AM -0800, Jakub Kicinski wrote:
>>>>> On Wed, 14 Dec 2022 09:22:13 +0200 Leon Romanovsky wrote:
>>>>>> NAK to any module driver parameter. If it is applicable to all drivers,
>>>>>> please find a way to configure it to more user-friendly. If it is not,
>>>>>> try to do the same as other drivers do.
>>>>> 
>>>>> I think this one may be fine. Configuration which has to be set before
>>>>> device probing can't really be per-device.
>>>> 
>>>> This configuration can be different between multiple devices
>>>> which use same igb module. Module parameters doesn't allow such
>>>> separation.
>>> 
>>> Configuration of the device, sure, but this module param is more of
>>> a system policy. BTW the name of the param is not great, we're allowing
>>> the use of random address, not an invalid address.
>>> 
>>>> Also, as a user, I despise random module parameters which I need
>>>> to set after every HW update/replacement.
>>> 
>>> Agreed, IIUC the concern was alerting users to incorrect EEPROM values.
>>> I thought falling back to a random address was relatively common, but
>>> I haven't done the research.
>> 
>> My 2ct, because I once added the fallback to a random MAC address to r8169:
>> Question is whether there's any scenario where you would prefer bailing out
>> in case of invalid MAC address over assigning a random MAC address (that the
>> user may manually change later) plus a warning.
>> I'm not aware of such a scenario. Therefore I decided to hardcode this
>> fallback in the driver.
> 
> I've seen issues with such a solution in the past. In addition it is
> very easy for the user to miss the warning and when the EEPROM is
> corrupted on the Intel NICs it has other side effects. That is one of
> the reasons why the MAC address is used as a requirement for us to
> spawn a netdev.
> 
> As far as the discussion for module parameter vs something else. The
> problem with the driver is that it is monolithic so it isn't as if
> there is a devlink interface to configure a per-network parameter
> before the network portion is loaded. The module parameter is a
> compromise that should only be used to enable the workaround so that
> the driver can be loaded so that the EEPROM can then be edited. If
> anything, tying the EEPROM to ethtool is the issue. If somebody wants
> to look at providing an option to edit the EEPROM via devlink that
> would solve the issue as then the driver could expose the devlink
> interface to edit the EEPROM without having to allocate and register a
> netdev.