netdev - Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1d7e3018-9c82-4a00-8e10-3451b4a19a0d@lunn.ch>
Date: Fri, 14 Feb 2025 14:33:45 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Breno Leitao <leitao@...ian.org>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: Remove redundant variable declaration in
 __dev_change_flags()

On Fri, Feb 14, 2025 at 04:47:49AM -0800, Breno Leitao wrote:
> The old_flags variable is declared twice in __dev_change_flags(),
> causing a shadow variable warning. This patch fixes the issue by
> removing the redundant declaration, reusing the existing old_flags
> variable instead.
> 
> 	net/core/dev.c:9225:16: warning: declaration shadows a local variable [-Wshadow]
> 	9225 |                 unsigned int old_flags = dev->flags;
> 	|                              ^
> 	net/core/dev.c:9185:15: note: previous declaration is here
> 	9185 |         unsigned int old_flags = dev->flags;
> 	|                      ^
> 	1 warning generated.
> 
> This change has no functional impact on the code, as the inner variable
> does not affect the outer one. The fix simply eliminates the unnecessary
> declaration and resolves the warning.

I'm not a compiler person... but there might be some subtlety here:


int __dev_change_flags(struct net_device *dev, unsigned int flags,
		       struct netlink_ext_ack *extack)
{
	unsigned int old_flags = dev->flags;
	int ret;

This old_flags gets the value of flags at the time of entry into the
function.

...

	if ((old_flags ^ flags) & IFF_UP) {
		if (old_flags & IFF_UP)
			__dev_close(dev);
		else
			ret = __dev_open(dev, extack);
	}

If you dig down into __dev_close(dev) you find

		dev->flags &= ~IFF_UP;

then

...

	if ((flags ^ dev->gflags) & IFF_PROMISC) {
		int inc = (flags & IFF_PROMISC) ? 1 : -1;
		unsigned int old_flags = dev->flags;

This inner old_flags now has the IFF_UP removed, and so is different
to the outer old_flags.

The outer old_flags is not used after this point, so in the end it
might not matter, but that fact i felt i needed to look deeper at the
code suggests the commit message needs expanding to include more
analyses.

> Fixes: 991fb3f74c142e ("dev: always advertise rx_flags changes via netlink")

I suppose there is also a danger here this code has at some point in
the past has been refactored, such that the outer old_flags was used
at some point? Backporting this patch could then break something?  Did
you check for this? Again, a comment in the commit message that you
have checked this is safe to backport would be nice.

    Andrew

---
pw-bot: cr