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: <20250214-civet-of-regular-refinement-23b247@leitao>
Date: Fri, 14 Feb 2025 06:21:18 -0800
From: Breno Leitao <leitao@...ian.org>
To: Andrew Lunn <andrew@...n.ch>
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()

hello Andew,

On Fri, Feb 14, 2025 at 02:33:45PM +0100, Andrew Lunn wrote:
> 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.

Right, I have analyzed this when creating this fix. I was wondering if
I need to rename the inner old_flags or just reuse it, and I added it in
the commit message:

 > This change has no functional impact on the code, as the inner variable
 > does not affect the outer one.

But I agree with you, if you needed to look at it, it means the message
is NOT good enough. I will update it.

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

I haven't look at this, and I don't think this should be backported,
thus, that is why I sent to net-next and didn't cc: stable.

That said, I don't think this should be backported, since it is not
a big deal. Shouldn't I add the Fixes: in such case?

Thanks for the review,
--breno