[PATCH net-next v2 0/3] optimize procedure of changing MAC address on interface

[PATCH net-next v2 0/3] optimize procedure of changing MAC address on interface

[Piotr Gardocki]

The first patch adds an if statement in core to skip early when
the MAC address is not being changes.
The remaining patches remove such checks from Intel drivers
as they're redundant at this point.

v2: modified check in core to support addresses of any length,
removed redundant checks in i40e and ice

Piotr Gardocki (3):
  net: add check for current MAC address in dev_set_mac_address
  i40e: remove unnecessary check for old MAC == new MAC
  ice: remove unnecessary check for old MAC == new MAC

 drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ------
 drivers/net/ethernet/intel/ice/ice_main.c   | 5 -----
 net/core/dev.c                              | 2 ++
 3 files changed, 2 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/3] 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 c2456b3667fe..8f1c49ab17df 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8754,6 +8754,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+	if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len))
+		return 0;
 	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
 	if (err)
 		return err;
-- 
2.34.1

[PATCH net-next v2 2/3] i40e: remove unnecessary check for old MAC == new MAC

[Piotr Gardocki]

The check has been moved to core. The ndo_set_mac_address callback
is not being called with new MAC address equal to the old one anymore.

Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b847bd105b16..29ad1797adce 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -1788,12 +1788,6 @@ static int i40e_set_mac(struct net_device *netdev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
-		netdev_info(netdev, "already using mac address %pM\n",
-			    addr->sa_data);
-		return 0;
-	}
-
 	if (test_bit(__I40E_DOWN, pf->state) ||
 	    test_bit(__I40E_RESET_RECOVERY_PENDING, pf->state))
 		return -EADDRNOTAVAIL;
-- 
2.34.1

[PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Piotr Gardocki]

The check has been moved to core. The ndo_set_mac_address callback
is not being called with new MAC address equal to the old one anymore.

Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index caafb12d9cc7..f0ba896a3f46 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5624,11 +5624,6 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
 	if (!is_valid_ether_addr(mac))
 		return -EADDRNOTAVAIL;
 
-	if (ether_addr_equal(netdev->dev_addr, mac)) {
-		netdev_dbg(netdev, "already using mac %pM\n", mac);
-		return 0;
-	}
-
 	if (test_bit(ICE_DOWN, pf->state) ||
 	    ice_is_reset_in_progress(pf->state)) {
 		netdev_err(netdev, "can't set mac %pM. device not ready\n",
-- 
2.34.1

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

[Maciej Fijalkowski]

On Tue, Jun 13, 2023 at 02:24:18PM +0200, Piotr Gardocki wrote:
> 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.

Please use imperative mood here - "add proper check..."

> 
> 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>

I'll let Kuba ack it.

> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index c2456b3667fe..8f1c49ab17df 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8754,6 +8754,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>  		return -EINVAL;
>  	if (!netif_device_present(dev))
>  		return -ENODEV;
> +	if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len))
> +		return 0;
>  	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
>  	if (err)
>  		return err;
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 2/3] i40e: remove unnecessary check for old MAC == new MAC

[Maciej Fijalkowski]

On Tue, Jun 13, 2023 at 02:24:19PM +0200, Piotr Gardocki wrote:
> The check has been moved to core. The ndo_set_mac_address callback
> is not being called with new MAC address equal to the old one anymore.
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b847bd105b16..29ad1797adce 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -1788,12 +1788,6 @@ static int i40e_set_mac(struct net_device *netdev, void *p)
>  	if (!is_valid_ether_addr(addr->sa_data))
>  		return -EADDRNOTAVAIL;
>  
> -	if (ether_addr_equal(netdev->dev_addr, addr->sa_data)) {
> -		netdev_info(netdev, "already using mac address %pM\n",
> -			    addr->sa_data);
> -		return 0;
> -	}
> -
>  	if (test_bit(__I40E_DOWN, pf->state) ||
>  	    test_bit(__I40E_RESET_RECOVERY_PENDING, pf->state))
>  		return -EADDRNOTAVAIL;
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Maciej Fijalkowski]

On Tue, Jun 13, 2023 at 02:24:20PM +0200, Piotr Gardocki wrote:
> The check has been moved to core. The ndo_set_mac_address callback
> is not being called with new MAC address equal to the old one anymore.
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index caafb12d9cc7..f0ba896a3f46 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -5624,11 +5624,6 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
>  	if (!is_valid_ether_addr(mac))
>  		return -EADDRNOTAVAIL;
>  
> -	if (ether_addr_equal(netdev->dev_addr, mac)) {
> -		netdev_dbg(netdev, "already using mac %pM\n", mac);
> -		return 0;
> -	}
> -
>  	if (test_bit(ICE_DOWN, pf->state) ||
>  	    ice_is_reset_in_progress(pf->state)) {
>  		netdev_err(netdev, "can't set mac %pM. device not ready\n",
> -- 
> 2.34.1
> 

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

[Paul Menzel]

Dear Maciej,


Am 13.06.23 um 15:10 schrieb Maciej Fijalkowski:
> On Tue, Jun 13, 2023 at 02:24:18PM +0200, Piotr Gardocki wrote:
>> 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.
> 
> Please use imperative mood here - "add proper check..."

Just a note, that in “add check …” the verb *add* is already in 
imperative mood. (You can shorten “add noun …” often to just use the 
verb for the noun. In this case:

Check current MAC address in `dev_set_mac_address`

But it’s not specific enough. Maybe:

Avoid setting same MAC address


Kind regards,

Paul


>> 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>
> 
> I'll let Kuba ack it.
> 
>> ---
>>   net/core/dev.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index c2456b3667fe..8f1c49ab17df 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -8754,6 +8754,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>>   		return -EINVAL;
>>   	if (!netif_device_present(dev))
>>   		return -ENODEV;
>> +	if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len))
>> +		return 0;
>>   	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
>>   	if (err)
>>   		return err;
>> -- 
>> 2.34.1
>>

RE: [PATCH net-next v2 1/3] net: add check for current MAC address in dev_set_mac_address

[Fijalkowski, Maciej]

> Am 13.06.23 um 15:10 schrieb Maciej Fijalkowski:
> > On Tue, Jun 13, 2023 at 02:24:18PM +0200, Piotr Gardocki wrote:
> >> 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.
> >
> > Please use imperative mood here - "add proper check..."
> 
> Just a note, that in “add check …” the verb *add* is already in
> imperative mood. (You can shorten “add noun …” often to just use the
> verb for the noun. In this case:

I just meant to get rid of 'this patch...'

> 
> Check current MAC address in `dev_set_mac_address`
> 
> But it’s not specific enough. Maybe:
> 
> Avoid setting same MAC address
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> >> 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>
> >
> > I'll let Kuba ack it.
> >
> >> ---
> >>   net/core/dev.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index c2456b3667fe..8f1c49ab17df 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -8754,6 +8754,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
> >>   		return -EINVAL;
> >>   	if (!netif_device_present(dev))
> >>   		return -ENODEV;
> >> +	if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len))
> >> +		return 0;
> >>   	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
> >>   	if (err)
> >>   		return err;
> >> --
> >> 2.34.1
> >>

Re: [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Przemek Kitszel]

On 6/13/23 14:24, Piotr Gardocki wrote:
> The check has been moved to core. The ndo_set_mac_address callback
> is not being called with new MAC address equal to the old one anymore.
> 
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_main.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index caafb12d9cc7..f0ba896a3f46 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -5624,11 +5624,6 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
>   	if (!is_valid_ether_addr(mac))
>   		return -EADDRNOTAVAIL;
>   
> -	if (ether_addr_equal(netdev->dev_addr, mac)) {
> -		netdev_dbg(netdev, "already using mac %pM\n", mac);
> -		return 0;
> -	}
> -
>   	if (test_bit(ICE_DOWN, pf->state) ||
>   	    ice_is_reset_in_progress(pf->state)) {
>   		netdev_err(netdev, "can't set mac %pM. device not ready\n",

I would expect one patch that adds check in the core, then one patch 
that removes it in all, incl non-intel, drivers; with CC to their 
respective maintainers (like Tony for intel, ./scripts/get_maintainer.pl 
will help)

Re: [Intel-wired-lan] [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Przemek Kitszel]

On 6/13/23 16:02, Przemek Kitszel wrote:
> On 6/13/23 14:24, Piotr Gardocki wrote:
>> The check has been moved to core. The ndo_set_mac_address callback
>> is not being called with new MAC address equal to the old one anymore.
>>
>> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_main.c | 5 -----

[...]
> 
> I would expect one patch that adds check in the core, then one patch 
> that removes it in all, incl non-intel, drivers; with CC to their 
> respective maintainers (like Tony for intel, ./scripts/get_maintainer.pl 
> will help)

I have checked, it's almost 200 handlers, which amounts to over 3500 
lines of code (short-cutting analysis on eth_hw_addr_set()), what 
probably could warrant more than one patch/person to spread the work

anybody willing to see the above code-to-look-at, or wants to re-run it 
for their directory of interests, here is dirty bash script (which just 
approximates what's to be done, but rather closely to reality):

  grep -InrE '\.'ndo_set_mac_address'\s+=' |
  awk '!/NULL/ {gsub(/,$/, ""); print $NF}' |
  sort -u |
  xargs -I% bash -c 'grep -ERwIl %'"'"'\(struct net_device.+\)$'"'"' |
    xargs -I @  awk '"'"'/%\(struct net_device.+\)$/, 
/^}|eth_hw_addr_set\(/ { print  "@:" NR $0 }'"'"' @' |
cat -n

@Piotr, perhaps resolve all intel drivers in your series?

Re: [Intel-wired-lan] [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Piotr Gardocki]

On 13.06.2023 17:10, Przemek Kitszel wrote:
[...]
>>
>> I would expect one patch that adds check in the core, then one patch that removes it in all, incl non-intel, drivers; with CC to their respective maintainers (like Tony for intel, ./scripts/get_maintainer.pl will help)
> 
> I have checked, it's almost 200 handlers, which amounts to over 3500 lines of code (short-cutting analysis on eth_hw_addr_set()), what probably could warrant more than one patch/person to spread the work
> 
> anybody willing to see the above code-to-look-at, or wants to re-run it for their directory of interests, here is dirty bash script (which just approximates what's to be done, but rather closely to reality):
> 
>  grep -InrE '\.'ndo_set_mac_address'\s+=' |
>  awk '!/NULL/ {gsub(/,$/, ""); print $NF}' |
>  sort -u |
>  xargs -I% bash -c 'grep -ERwIl %'"'"'\(struct net_device.+\)$'"'"' |
>    xargs -I @  awk '"'"'/%\(struct net_device.+\)$/, /^}|eth_hw_addr_set\(/ { print  "@:" NR $0 }'"'"' @' |
> cat -n
> 
> @Piotr, perhaps resolve all intel drivers in your series?

Thanks for script, looks impressive :). Someone might really
use it to detect all occurrences. As you said there are a lot
of callbacks in kernel, so unfortunately I can't fix all of them.
I fixed it for drivers/net/ethernet/intel directory,
only i40e and ice had these checks. If you want me to check any
other intel directory or if I missed something here, please let
me know.

Re: [Intel-wired-lan] [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Przemek Kitszel]

On 6/13/23 17:16, Piotr Gardocki wrote:
> On 13.06.2023 17:10, Przemek Kitszel wrote:
> [...]
>>>
>>> I would expect one patch that adds check in the core, then one patch that removes it in all, incl non-intel, drivers; with CC to their respective maintainers (like Tony for intel, ./scripts/get_maintainer.pl will help)
>>
>> I have checked, it's almost 200 handlers, which amounts to over 3500 lines of code (short-cutting analysis on eth_hw_addr_set()), what probably could warrant more than one patch/person to spread the work
>>
>> anybody willing to see the above code-to-look-at, or wants to re-run it for their directory of interests, here is dirty bash script (which just approximates what's to be done, but rather closely to reality):
>>
>>   grep -InrE '\.'ndo_set_mac_address'\s+=' |
>>   awk '!/NULL/ {gsub(/,$/, ""); print $NF}' |
>>   sort -u |
>>   xargs -I% bash -c 'grep -ERwIl %'"'"'\(struct net_device.+\)$'"'"' |
>>     xargs -I @  awk '"'"'/%\(struct net_device.+\)$/, /^}|eth_hw_addr_set\(/ { print  "@:" NR $0 }'"'"' @' |
>> cat -n
>>
>> @Piotr, perhaps resolve all intel drivers in your series?
> 
> Thanks for script, looks impressive :). Someone might really
> use it to detect all occurrences. As you said there are a lot
> of callbacks in kernel, so unfortunately I can't fix all of them.
> I fixed it for drivers/net/ethernet/intel directory,
> only i40e and ice had these checks. If you want me to check any
> other intel directory or if I missed something here, please let
> me know.

there is ether_addr_equal() call in iavf_set_mac(), even if not exactly 
before eth_hw_addr_set(), it still should be removed ;)

Anyway, I would fix all 3 drivers with one patch.

Re: [Intel-wired-lan] [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Piotr Gardocki]

On 13.06.2023 17:24, Przemek Kitszel wrote:
> On 6/13/23 17:16, Piotr Gardocki wrote:
>> On 13.06.2023 17:10, Przemek Kitszel wrote:
>> [...]
>>>>
>>>> I would expect one patch that adds check in the core, then one patch that removes it in all, incl non-intel, drivers; with CC to their respective maintainers (like Tony for intel, ./scripts/get_maintainer.pl will help)
>>>
>>> I have checked, it's almost 200 handlers, which amounts to over 3500 lines of code (short-cutting analysis on eth_hw_addr_set()), what probably could warrant more than one patch/person to spread the work
>>>
>>> anybody willing to see the above code-to-look-at, or wants to re-run it for their directory of interests, here is dirty bash script (which just approximates what's to be done, but rather closely to reality):
>>>
>>>   grep -InrE '\.'ndo_set_mac_address'\s+=' |
>>>   awk '!/NULL/ {gsub(/,$/, ""); print $NF}' |
>>>   sort -u |
>>>   xargs -I% bash -c 'grep -ERwIl %'"'"'\(struct net_device.+\)$'"'"' |
>>>     xargs -I @  awk '"'"'/%\(struct net_device.+\)$/, /^}|eth_hw_addr_set\(/ { print  "@:" NR $0 }'"'"' @' |
>>> cat -n
>>>
>>> @Piotr, perhaps resolve all intel drivers in your series?
>>
>> Thanks for script, looks impressive :). Someone might really
>> use it to detect all occurrences. As you said there are a lot
>> of callbacks in kernel, so unfortunately I can't fix all of them.
>> I fixed it for drivers/net/ethernet/intel directory,
>> only i40e and ice had these checks. If you want me to check any
>> other intel directory or if I missed something here, please let
>> me know.
> 
> there is ether_addr_equal() call in iavf_set_mac(), even if not exactly before eth_hw_addr_set(), it still should be removed ;)
> 
> Anyway, I would fix all 3 drivers with one patch.

I guess you're looking at old version of dev-queue branch on Tony's tree :)
Regarding ice and i40e I made two patches to have different prefixes in titles.
I don't mind merging them, but I'll wait for someone else speaking up about this.

Re: [Intel-wired-lan] [PATCH net-next v2 3/3] ice: remove unnecessary check for old MAC == new MAC

[Jakub Kicinski]

On Tue, 13 Jun 2023 17:32:50 +0200 Piotr Gardocki wrote:
> > there is ether_addr_equal() call in iavf_set_mac(), even if not
> > exactly before eth_hw_addr_set(), it still should be removed ;)
> > 
> > Anyway, I would fix all 3 drivers with one patch.  
> 
> I guess you're looking at old version of dev-queue branch on Tony's
> tree :) Regarding ice and i40e I made two patches to have different
> prefixes in titles. I don't mind merging them, but I'll wait for
> someone else speaking up about this.

I think the series is good enough, FWIW. We're already at -rc6, seems
more important to give syzbot and testers time to exercise the core
change than fishing out more drivers.

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

[Gal Pressman]

On 13/06/2023 15:24, Piotr Gardocki wrote:
> 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>

Hello Piotr,

I believe this patch has an (unintended?) side effect.
The early return in dev_set_mac_address() makes it so
'dev->addr_assign_type' doesn't get assigned with NET_ADDR_SET, I don't
think this is the correct behavior.

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

[Piotr Gardocki]

On 20.06.2023 09:16, Gal Pressman wrote:
> On 13/06/2023 15:24, Piotr Gardocki wrote:
>> 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>
> 
> Hello Piotr,
> 
> I believe this patch has an (unintended?) side effect.
> The early return in dev_set_mac_address() makes it so
> 'dev->addr_assign_type' doesn't get assigned with NET_ADDR_SET, I don't
> think this is the correct behavior.

Hi Gal,

I checked it, you're right. When the addr_assign_type is PERM or RANDOM
and user or some driver sets the same MAC address the type doesn't change
to NET_ADDR_SET. In my testing I didn't notice issues with that, but I'm
sure there are cases I didn't cover. Did you discover any useful cases
that broke after this patch or did you just notice it in code?

The less invasive solution might be to skip only call to ndo_set_mac_address
if the address doesn't change, but call everything else - I suppose the
notifying mechanism would be required if we change addr_assign_type, right?

The patch set was already in v3 and it's applied to netdev next queue.
I'll let maintainers decide how to proceed with it now. I can take care of it,
but need to know whether to submit new patch or send v4.
@Jakub Kicinski, could you please take a look at request and give us some guidance?

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

[Gal Pressman]

On 20/06/2023 13:29, Piotr Gardocki wrote:
> On 20.06.2023 09:16, Gal Pressman wrote:
>> On 13/06/2023 15:24, Piotr Gardocki wrote:
>>> 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>
>>
>> Hello Piotr,
>>
>> I believe this patch has an (unintended?) side effect.
>> The early return in dev_set_mac_address() makes it so
>> 'dev->addr_assign_type' doesn't get assigned with NET_ADDR_SET, I don't
>> think this is the correct behavior.
> 
> Hi Gal,
> 
> I checked it, you're right. When the addr_assign_type is PERM or RANDOM
> and user or some driver sets the same MAC address the type doesn't change
> to NET_ADDR_SET. In my testing I didn't notice issues with that, but I'm
> sure there are cases I didn't cover. Did you discover any useful cases
> that broke after this patch or did you just notice it in code?

This behavior change was caught in our regression tests.

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

[Jakub Kicinski]

On Tue, 20 Jun 2023 13:42:14 +0300 Gal Pressman wrote:
> > I checked it, you're right. When the addr_assign_type is PERM or RANDOM
> > and user or some driver sets the same MAC address the type doesn't change
> > to NET_ADDR_SET. In my testing I didn't notice issues with that, but I'm
> > sure there are cases I didn't cover. Did you discover any useful cases
> > that broke after this patch or did you just notice it in code?  
> 
> This behavior change was caught in our regression tests.

Why was the regression test written this way?

I guess we won't flip it back to PERM if user sets a completely
different address temporarily and then back to PERM - so for consistency
going to SET even when the addr doesn't change may be reasonable.

Piotr, you'll need to send a new followup patch on top of net-next.

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

[Piotr Gardocki]

On 20.06.2023 17:59, Jakub Kicinski wrote:
> On Tue, 20 Jun 2023 13:42:14 +0300 Gal Pressman wrote:
>>> I checked it, you're right. When the addr_assign_type is PERM or RANDOM
>>> and user or some driver sets the same MAC address the type doesn't change
>>> to NET_ADDR_SET. In my testing I didn't notice issues with that, but I'm
>>> sure there are cases I didn't cover. Did you discover any useful cases
>>> that broke after this patch or did you just notice it in code?  
>>
>> This behavior change was caught in our regression tests.
> 
> Why was the regression test written this way?
> 
> I guess we won't flip it back to PERM if user sets a completely
> different address temporarily and then back to PERM - so for consistency
> going to SET even when the addr doesn't change may be reasonable.
> 
> Piotr, you'll need to send a new followup patch on top of net-next.

Thanks, I'll prepare, test and send new patch tomorrow.

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

[Gal Pressman]

On 20/06/2023 18:59, Jakub Kicinski wrote:
> On Tue, 20 Jun 2023 13:42:14 +0300 Gal Pressman wrote:
>>> I checked it, you're right. When the addr_assign_type is PERM or RANDOM
>>> and user or some driver sets the same MAC address the type doesn't change
>>> to NET_ADDR_SET. In my testing I didn't notice issues with that, but I'm
>>> sure there are cases I didn't cover. Did you discover any useful cases
>>> that broke after this patch or did you just notice it in code?  
>>
>> This behavior change was caught in our regression tests.
> 
> Why was the regression test written this way?

This test environment is quite good at detecting state/behavior changes.

The test isn't written in any specific way, AFAIU we hit this patch flow
by default when using bonding (bond takes the first slave MAC address
and then sets the same address to all slaves?), it's not a test that
tries to set the same MAC address explicitly.