| Message ID | 20230613122420.855486-2-piotrx.gardocki@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | optimize procedure of changing MAC address on interface | expand |
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 >
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 >>
> 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 > >>
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.
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?
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.
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.
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.
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.
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;
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(+)