netdev - Re: [PATCH net-next] net: fix net device address assign type

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 22 Jun 2023 10:22:58 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Piotr Gardocki <piotrx.gardocki@...el.com>
Cc: netdev@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
	przemyslaw.kitszel@...el.com, michal.swiatkowski@...ux.intel.com,
	pmenzel@...gen.mpg.de, kuba@...nel.org,
	maciej.fijalkowski@...el.com, anthony.l.nguyen@...el.com,
	simon.horman@...igine.com, aleksander.lobakin@...el.com,
	gal@...dia.com
Subject: Re: [PATCH net-next] net: fix net device address assign type

Wed, Jun 21, 2023 at 03:21:06PM CEST, piotrx.gardocki@...el.com wrote:
>Commit ad72c4a06acc introduced optimization to return from function

Out of curiosity, what impact does this optimization have? Is it worth
it to have such optimization at all? Wouldn't simple revert of the fixes
commit do the trick? If not, see below.


>quickly if the MAC address is not changing at all. It was reported
>that such change causes dev->addr_assign_type to not change
>to NET_ADDR_SET from _PERM or _RANDOM.
>Restore the old behavior and skip only call to ndo_set_mac_address.
>
>Fixes: ad72c4a06acc ("net: add check for current MAC address in dev_set_mac_address")
>Reported-by: Gal Pressman <gal@...dia.com>
>Signed-off-by: Piotr Gardocki <piotrx.gardocki@...el.com>
>---
> net/core/dev.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index e4ff0adf5523..69a3e544676c 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -8781,14 +8781,14 @@ 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;
>-	err = ops->ndo_set_mac_address(dev, sa);
>-	if (err)
>-		return err;
>+	if (memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) {
>+		err = ops->ndo_set_mac_address(dev, sa);
>+		if (err)
>+			return err;
>+	}
> 	dev->addr_assign_type = NET_ADDR_SET;
> 	call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);

Although I don't think the notifiers here and
dev_pre_changeaddr_notify() above have to be called in case the address
didn't actually change, it restores the old behaviour, even with the
netlink notification, which is probably good.

Reviewed-by: Jiri Pirko <jiri@...dia.com>


> 	add_device_randomness(dev->dev_addr, dev->addr_len);
>-- 
>2.34.1
>
>