Re: [PATCH net-next] net: Remove redundant variable declaration in __dev_change_flags()

From: Breno Leitao
Date: Fri Feb 14 2025 - 09:21:34 EST


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