netdev - Re: [PATCH v6] 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: <cbfb5d1a-dadd-efe5-b5d9-de9f483e44b2@intel.com>
Date:   Tue, 14 Jun 2022 12:09:02 -0700
From:   Tony Nguyen <anthony.l.nguyen@...el.com>
To:     Lixue Liang <lianglixuehao@....com>, <pmenzel@...gen.mpg.de>
CC:     <intel-wired-lan@...ts.osuosl.org>, <jesse.brandeburg@...el.com>,
        <kuba@...nel.org>, <lianglixue@...atwall.com.cn>,
        <netdev@...r.kernel.org>, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v6] igb: Assign random MAC address instead of fail in case
 of invalid one



On 6/9/2022 7:39 PM, Lixue Liang wrote:
> From: Lixue Liang <lianglixue@...atwall.com.cn>
> 
> Add the module parameter "allow_invalid_mac_address" to control the

netdev maintainers:

I know the use of module parameters is extremely frowned upon. Is this a 
use/exception that would be allowed?

> behavior. When set to true, a random MAC address is assigned, and the
> driver can be loaded, allowing the user to correct the invalid MAC address.
> 
> Signed-off-by: Lixue Liang <lianglixue@...atwall.com.cn>
> ---
> Changelog:
> * v6:
>    - Modify commit messages and naming of module parameters
> Suggested-by Paul <pmenzel@...gen.mpg.de>
> * v5:
>    - Through the setting of module parameters, it is allowed to complete
>      the loading of the igb network card driver with an invalid MAC address.
> Suggested-by <alexander.duyck@...il.com>
> * v4:
>    - Change the igb_mian in the title to igb
>    - Fix dev_err message: replace "already assigned random MAC address"
>      with "Invalid MAC address. Assigned random MAC address"
> Suggested-by Tony <anthony.l.nguyen@...el.com>
> 
> * v3:
>    - Add space after comma in commit message
>    - Correct spelling of MAC address
> Suggested-by Paul <pmenzel@...gen.mpg.de>
> 
> * v2:
>    - Change memcpy to ether_addr_copy
>    - Change dev_info to dev_err
>    - Fix the description of the commit message
>    - Change eth_random_addr to eth_hw_addr_random
> Reported-by: kernel test robot <lkp@...el.com>
> 
>   drivers/net/ethernet/intel/igb/igb_main.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 34b33b21e0dc..b61f216331da 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -238,8 +238,11 @@ MODULE_LICENSE("GPL v2");
>   
>   #define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV|NETIF_MSG_PROBE|NETIF_MSG_LINK)
>   static int debug = -1;
> +static bool allow_invalid_mac_address;
>   module_param(debug, int, 0);
> +module_param(allow_invalid_mac_address, bool, 0);
>   MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> +MODULE_PARM_DESC(allow_invalid_mac_address, "Allow NIC driver to be loaded with invalid MAC address");

Lixue:
If the maintainers are willing to take it, I believe the convention is 
to group each parameter together vs mixing them.

e.g.

  static int debug = -1;
  module_param(debug, int, 0);
  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");

+static bool allow_invalid_mac_address;
+module_param(allow_invalid_mac_address, bool, 0);
+MODULE_PARM_DESC(allow_invalid_mac_address, "Allow NIC driver to be 
loaded with invalid MAC address");

Thanks,
Tony

>   struct igb_reg_info {
>   	u32 ofs;