[PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Piotr Gardocki]

In some cases it is possible for kernel to come with request
to change primary MAC address to the address that is already
set on the given interface.

This patch adds proper check to return fast from the function
in these cases.

An example of such case is adding an interface to bonding
channel in balance-alb mode:
modprobe bonding mode=balance-alb miimon=100 max_bonds=1
ip link set bond0 up
ifenslave bond0 <eth>

Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 99d99b247bc9..c2c3ec61397b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8820,6 +8820,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
+		return 0;
 	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
 	if (err)
 		return err;
-- 
2.34.1

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Maciej Fijalkowski]

On Fri, Jun 09, 2023 at 06:52:41PM +0200, Piotr Gardocki wrote:

Hey Piotr,

> In some cases it is possible for kernel to come with request
> to change primary MAC address to the address that is already
> set on the given interface.
> 
> This patch adds proper check to return fast from the function
> in these cases.
> 
> An example of such case is adding an interface to bonding
> channel in balance-alb mode:
> modprobe bonding mode=balance-alb miimon=100 max_bonds=1
> ip link set bond0 up
> ifenslave bond0 <eth>
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 99d99b247bc9..c2c3ec61397b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8820,6 +8820,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>  		return -EINVAL;
>  	if (!netif_device_present(dev))
>  		return -ENODEV;
> +	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
> +		return 0;

Now this check being in makes calls to ether_addr_equal() within driver's
ndo callbacks redundant.

I would rather see this as patchset send directly to netdev where you have
this patch followed by addressing driver's callbacks.

Moar commitz == moar glory ;)

>  	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
>  	if (err)
>  		return err;
> -- 
> 2.34.1
> 

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Piotr Gardocki]

On 09.06.2023 19:00, Maciej Fijalkowski wrote:
> On Fri, Jun 09, 2023 at 06:52:41PM +0200, Piotr Gardocki wrote:
>> @@ -8820,6 +8820,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>>  		return -EINVAL;
>>  	if (!netif_device_present(dev))
>>  		return -ENODEV;
>> +	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
>> +		return 0;
> 
> Now this check being in makes calls to ether_addr_equal() within driver's
> ndo callbacks redundant.
> 
> I would rather see this as patchset send directly to netdev where you have
> this patch followed by addressing driver's callbacks.

Sure, will do :)
I will remove this check in all drivers in drivers/net/ethernet/intel/,
I don't have time to review all usages of this callback in kernel,
there are too many of them. Will resend as a patchset next week.

> Moar commitz == moar glory ;)
> 
>>  	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
>>  	if (err)
>>  		return err;
>> -- 
>> 2.34.1
>>

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Jakub Kicinski]

On Fri,  9 Jun 2023 18:52:41 +0200 Piotr Gardocki wrote:
> +	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
> +		return 0;

not every device is ethernet, you need to use dev->addr_len for
the comparison.
-- 
pw-bot: cr

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Piotr Gardocki]

On 10.06.2023 08:44, Jakub Kicinski wrote:
> On Fri,  9 Jun 2023 18:52:41 +0200 Piotr Gardocki wrote:
>> +	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
>> +		return 0;
> 
> not every device is ethernet, you need to use dev->addr_len for
> the comparison.

Before re-sending I just want to double check.
Did you mean checking if sa->sa_family == AF_LOCAL ?
There's no length in sockaddr.

It would like this:
	if (sa->sa_family == AF_LOCAL &&
	    ether_addr_equal(dev->dev_addr, sa->sa_data))
		return 0;

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Maciej Fijalkowski]

On Mon, Jun 12, 2023 at 04:49:47PM +0200, Piotr Gardocki wrote:
> On 10.06.2023 08:44, Jakub Kicinski wrote:
> > On Fri,  9 Jun 2023 18:52:41 +0200 Piotr Gardocki wrote:
> >> +	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
> >> +		return 0;
> > 
> > not every device is ethernet, you need to use dev->addr_len for
> > the comparison.
> 
> Before re-sending I just want to double check.
> Did you mean checking if sa->sa_family == AF_LOCAL ?
> There's no length in sockaddr.
> 
> It would like this:
> 	if (sa->sa_family == AF_LOCAL &&
> 	    ether_addr_equal(dev->dev_addr, sa->sa_data))
> 		return 0;

I believe Jakub just wanted this:

	if (dev->addr_len)
		if (ether_addr_equal(dev->dev_addr, sa->sa_data))
			return 0;

so no clue why you want anything from sockaddr?

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Piotr Gardocki]

On 12.06.2023 17:37, Maciej Fijalkowski wrote:
> On Mon, Jun 12, 2023 at 04:49:47PM +0200, Piotr Gardocki wrote:
>> On 10.06.2023 08:44, Jakub Kicinski wrote:
>>> On Fri,  9 Jun 2023 18:52:41 +0200 Piotr Gardocki wrote:
>>>> +	if (ether_addr_equal(dev->dev_addr, sa->sa_data))
>>>> +		return 0;
>>>
>>> not every device is ethernet, you need to use dev->addr_len for
>>> the comparison.
>>
>> Before re-sending I just want to double check.
>> Did you mean checking if sa->sa_family == AF_LOCAL ?
>> There's no length in sockaddr.
>>
>> It would like this:
>> 	if (sa->sa_family == AF_LOCAL &&
>> 	    ether_addr_equal(dev->dev_addr, sa->sa_data))
>> 		return 0;
> 
> I believe Jakub just wanted this:
> 
> 	if (dev->addr_len)
> 		if (ether_addr_equal(dev->dev_addr, sa->sa_data))
> 			return 0;
> 
> so no clue why you want anything from sockaddr?

I understood that dev->type and dev->addr_len can just be different
than AF_LOCAL and 48 bits in this function.
Your version does not convince me, let's wait for Jakub's judgement.

Re: [PATCH net-next] net: add check for current MAC address in dev_set_mac_address

[Jakub Kicinski]

On Mon, 12 Jun 2023 18:43:01 +0200 Piotr Gardocki wrote:
> On 12.06.2023 17:37, Maciej Fijalkowski wrote:
> > On Mon, Jun 12, 2023 at 04:49:47PM +0200, Piotr Gardocki wrote:  
> >> Before re-sending I just want to double check.
> >> Did you mean checking if sa->sa_family == AF_LOCAL ?
> >> There's no length in sockaddr.
> >>
> >> It would like this:
> >> 	if (sa->sa_family == AF_LOCAL &&
> >> 	    ether_addr_equal(dev->dev_addr, sa->sa_data))
> >> 		return 0;  
> > 
> > I believe Jakub just wanted this:
> > 
> > 	if (dev->addr_len)
> > 		if (ether_addr_equal(dev->dev_addr, sa->sa_data))
> > 			return 0;
> > 
> > so no clue why you want anything from sockaddr?  
> 
> I understood that dev->type and dev->addr_len can just be different
> than AF_LOCAL and 48 bits in this function.
> Your version does not convince me, let's wait for Jakub's judgement.

I'm probably missing something because I'm not sure where 
the discussion about AF_LOCAL came from. All I meant is:

	if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len))
		return 0;