Re: [PATCH] igb_ethtool: fix efficiency issues in igb_set_eeprom

Re: [PATCH] igb_ethtool: fix efficiency issues in igb_set_eeprom

[Jesse Brandeburg]

Please include netdev mailing list when mailing the maintainers of netdev.

On 5/9/2022 6:21 PM, lianglixue wrote:
> Modify the value of eeprom in igb_set_eeprom. If the mac address
> of i210 is changed to illegal, then in igp_probe in the
> igb_main file, is_valid_eth_addr (netdev->dev_addr) exits
> with an error, causing the igb driver to fail to load,
> such as ethtool -E eth0 magic 0x15338086 offset 0 value 0x01.

This interface is very much equivalent to a nail and hammer, and if you 
decide to hammer the nail through your foot that's your right.

This has been this way forever (in more than just igb) and we haven't 
changed this interface to "protect" the user, so I'm really not sure 
what the value of the change is now.

> In this way, the igb driver can no longer be loaded,
> and the legal mac address cannot be recovered through ethtool;
> add is_valid_eth_addr to igb_set_eeprom to determine
> whether it is legal to rewrite, so as to avoid driver
> errors due to illegal mac addresses.
> 
> Signed-off-by: lianglixue <lianglixue@greatwall.com.cn>
> ---
>   drivers/net/ethernet/intel/igb/igb_ethtool.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 2a5782063f4c..30554fd684db 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -798,6 +798,13 @@ static int igb_set_eeprom(struct net_device *netdev,
>   	if (eeprom->magic != (hw->vendor_id | (hw->device_id << 16)))
>   		return -EFAULT;
>   
> +	if (hw->mac.type == e1000_i210 && eeprom->offset == 0) {

before you go reading "bytes" length of bytes in the next line, you need 
to be checking the length of the write in this if as well, don't you? 
Why is this check only valid for i210? Is that the only one you know the 
eeprom format for?

> +		if (!is_valid_ether_addr(bytes)) {

this will read six bytes, but is before the length checks below.

> +			dev_err(&adapter->pdev->dev, "Invalid MAC Address for i210\n");
> +			return -EINVAL;
> +		}
> +	}
> +
>   	max_len = hw->nvm.word_size * 2;
>   
>   	first_word = eeprom->offset >> 1;

This change might still be useful, since it is obvious that several 
possible values of the first six bytes are invalid. Please respin and 
respond to the notes above.

Re: [PATCH] igb_ethtool: fix efficiency issues in igb_set_eeprom

[lixue liang]

Thank you very much for replying to my question and for suggesting corrections.

Since the interface (igb_set_eeprom) for arbitrarily modifying the mac address is provided, 
it is equivalent to providing a way to modify the mac address arbitrarily and cause the igb 
driver to fail to load. Therefore, a way to restore a valid mac address should also be reserved, 
so as to prevent most users from being helpless in the case of an invalid mac address.

Except by using tools to flash the firmware or modify the igb driver to continue loading under 
an invalid mac address, however most users do not have this ability. In the case of invalidity, 
the invalid mac address must be changed to a legal address, so it is always better to use a 
valid mac address to continue pretending to be a network card driver when the mac address is invalid,
 which is always better than not being able to load the driver, such as the microchip network card (lan743x) driver .

I think it is worthwhile to replace the invalid mac address with a random valid mac address to 
complete the igb driver loading. In response to the above problem, I will provide a patch on igb_main.c again..

Thank you for taking the time.

> 2022年5月11日 03:43，Jesse Brandeburg <jesse.brandeburg@intel.com> 写道：
> 
> Please include netdev mailing list when mailing the maintainers of netdev.

Re: [Intel-wired-lan] [PATCH] igb_ethtool: fix efficiency issues in igb_set_eeprom

[lixue liang]

Hi Paul,

Thank you very much for your reply and suggestions.I have made the corresponding changes according to your suggestion.

In addition, for the problem that the invalid mac address cannot load the igb driver, I personally think there is a better way to modify it, 
and I will resubmit the patch about igb_main.c.

It's the same question, but I may not know how to continue submitting new patches on this email, sorry about that.

> 2022年5月11日 14:41，Paul Menzel <pmenzel@molgen.mpg.de> 写道：
> 
> Dear lianglixue,
> 
> 
> Thank you for your patch.
> 
> Am 10.05.22 um 03:21 schrieb lianglixue:
> 
> It’d be great if you spelled your name with spaces (if possible).
> 
> Also, currently your Google Mail address would be used for the Author field. If you want to use your company(?) address, please add a From: line at the top.
> 
>> Modify the value of eeprom in igb_set_eeprom. If the mac address
>> of i210 is changed to illegal, then in igp_probe in the
>> igb_main file, is_valid_eth_addr (netdev->dev_addr) exits
>> with an error, causing the igb driver to fail to load,
>> such as ethtool -E eth0 magic 0x15338086 offset 0 value 0x01.
>> In this way, the igb driver can no longer be loaded,
>> and the legal mac address cannot be recovered through ethtool;
>> add is_valid_eth_addr to igb_set_eeprom to determine
>> whether it is legal to rewrite, so as to avoid driver
>> errors due to illegal mac addresses.
> 
> Please reflow the text for 75 characters per line.
> 
>> Signed-off-by: lianglixue <lianglixue@greatwall.com.cn>
>> ---
>> drivers/net/ethernet/intel/igb/igb_ethtool.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 2a5782063f4c..30554fd684db 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -798,6 +798,13 @@ static int igb_set_eeprom(struct net_device *netdev,
>> 	if (eeprom->magic != (hw->vendor_id | (hw->device_id << 16)))
>> 		return -EFAULT;
>> +	if (hw->mac.type == e1000_i210 && eeprom->offset == 0) {
>> +		if (!is_valid_ether_addr(bytes)) {
>> +			dev_err(&adapter->pdev->dev, "Invalid MAC Address for i210\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> 	max_len = hw->nvm.word_size * 2;
>> 	first_word = eeprom->offset >> 1;
> 
> 
> Kind regards,
> 
> Paul

Re: [Intel-wired-lan] [PATCH] igb_ethtool: fix efficiency issues in igb_set_eeprom

[Paul Menzel]

Dear Lixue,


Am 11.05.22 um 09:59 schrieb lixue liang:

> Thank you very much for your reply and suggestions.I have made the
> corresponding changes according to your suggestion.

Thank you.

> In addition, for the problem that the invalid mac address cannot load
> the igb driver, I personally think there is a better way to modify
> it, and I will resubmit the patch about igb_main.c.

Awesome.

> It's the same question, but I may not know how to continue submitting
> new patches on this email, sorry about that.

As you use `git send-email`, what does `git log` show as the patch 
author? It might be as easy as to do

     git config --global user.name "Lixue Liang"
     git config --global user.email lianglixue@greatwall.com.cn
     git commit --amend --author "Lixue Liang <lianglixue@greatwall.com.cn>"


Kind regards,

Paul


PS: When replying, it’d be great if you used interleaved style [1].


[1]: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style