netdev - Re: [Intel-wired-lan] [PATCH] igb_main：Added invalid mac address handling in igb

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: <f486b0a0-2f6b-13e9-e905-8ad9163020a7@molgen.mpg.de>
Date:   Wed, 11 May 2022 16:53:56 +0200
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     lixue liang <lianglixue@...atwall.com.cn>
Cc:     jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
        kuba@...nel.org, intel-wired-lan@...ts.osuosl.org,
        netdev@...r.kernel.org
Subject: Re: [Intel-wired-lan] [PATCH] igb_main：Added invalid mac address handling in igb_probe

Dear Lixue,


Thank you for the patch. Please tag patch iterations with a version. 
(Use `-v 2` in `git send-email` for example.)

Am 11.05.22 um 10:07 schrieb lixue liang:

Please use the normal colon : in the summary.

Also, please use imperative mood in present tense: Add …

But, in this case

Handle invalid MAC address …

is shorter. Or:

Assign random MAC address instead of fail in case of invalid one

> In some cases, when the user uses igb_set_eeprom to modify
> the mac address to be invalid, the igb driver will fail to load.
> If there is no network card device, the user must modify it to
> a valid mac address by other means. It is only the invalid
> mac address that causes the driver The fatal problem of

… MAC address causing the driver to failure. The fatal …

> loading failure will cause most users no choice but to trouble.

Maybe remove this sentence, or rephrase.

> Since the mac address may be changed to be invalid, it must
> also be changed to a valid mac address, then add a random
> valid mac address to replace the invalid mac address in the
> driver, continue to load the igb network card driver,
> and output the relevant log reminder. vital to the user.

Please reflow for 75 characters per line. (More words fit in one line.)

> Signed-off-by: lixue liang <lianglixue@...atwall.com.cn>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 7 ++++---
>   1 file changed, 4 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..a513570c2ad6 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3359,9 +3359,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	eth_hw_addr_set(netdev, hw->mac.addr);
>   
>   	if (!is_valid_ether_addr(netdev->dev_addr)) {
> -		dev_err(&pdev->dev, "Invalid MAC Address\n");
> -		err = -EIO;
> -		goto err_eeprom;
> +		eth_random_addr(netdev->dev_addr);
> +		memcpy(hw->mac.addr, netdev->dev_addr, netdev->addr_len);
> +		dev_info(&pdev->dev,
> +			 "Invalid Mac Address, already got random Mac Address\n");

Is there a valid MAC address that should be only used for testing. Maybe 
that can be used. Maybe also log the address.

Lastly, please fully capitalize MAC.

>   	}
>   
>   	igb_set_default_mac_filter(adapter);


Kind regards,

Paul