netdev - Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow warnings throughout the file

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: <CANn89iL+goDgitdic97+um6D-PZDw2xYf=2eYgnNYEYU-Nws0Q@mail.gmail.com>
Date: Tue, 2 Apr 2024 14:45:07 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Paolo Abeni <pabeni@...hat.com>, Dmitry Safonov <0x7f454c46@...il.com>, 
	Heiner Kallweit <hkallweit1@...il.com>, nex.sw.ncis.osdt.itp.upstreaming@...el.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] netdev_queues: fix -Wshadow / Sparse shadow
 warnings throughout the file

On Tue, Apr 2, 2024 at 1:55 PM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> From: Jakub Kicinski <kuba@...nel.org>
> Date: Fri, 29 Mar 2024 13:53:44 -0700
>
> > On Fri, 29 Mar 2024 13:18:57 -0700 Jakub Kicinski wrote:
> >>> Sparse:
> >>>
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: warning: symbol '_res' shadows an earlier one
> >>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:1992:16: originally declared here
> >>
> >> I don't see these building with LLVM=1 W=12 C=1
> >> and I really don't like complicating the code because the compiler
> >> is stupid. Can't you solve this with some renames? Add another
>
> It's not the compiler, its warnings are valid actually. Shadowing makes
> it very easy to confuse variables and make bugs...
>
> >> underscore or something?
> >
> > I'm stupid I tried on the test branch which already had your fix..
>
> :D Sometimes it happens.
>
> >
> > This is enough:
> >
> > diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> > index 1ec408585373..2270fbb99cf7 100644
> > --- a/include/net/netdev_queues.h
> > +++ b/include/net/netdev_queues.h
> > @@ -89,7 +89,7 @@ struct netdev_stat_ops {
> >
> >  #define netif_txq_try_stop(txq, get_desc, start_thrs)                        \
> >       ({                                                              \
> > -             int _res;                                               \
> > +             int __res;                                              \
> >                                                                       \
> >               netif_tx_stop_queue(txq);                               \
> >               /* Producer index and stop bit must be visible          \
> > @@ -101,12 +101,12 @@ struct netdev_stat_ops {
> >               /* We need to check again in a case another             \
> >                * CPU has just made room available.                    \
> >                */                                                     \
> > -             _res = 0;                                               \
> > +             __res = 0;                                              \
> >               if (unlikely(get_desc >= start_thrs)) {                 \
> >                       netif_tx_start_queue(txq);                      \
> > -                     _res = -1;                                      \
> > +                     __res = -1;                                     \
> >               }                                                       \
> > -             _res;                                                   \
> > +             __res;                                                  \
> >       })                                                              \
> >
> >  /**
>
> But what if there's a function which calls one of these functions and
> already has _res or __res or something? I know renaming is enough for
> the warnings I mentioned, but without __UNIQUE_ID() anything can happen
> anytime, so I wanted to fix that once and for all :z
>
> I already saw some macros which have a layer of indirection for
> __UNIQUE_ID(), but previously they didn't and then there were fixes
> which added underscores, renamed variables etc etc...
>

We have hundreds of macros in include/ directory which use local names without
__UNIQUE_ID()

What is the plan ? Hundreds of patches obfuscating them more than they are ?

Can you show us how rb_entry_safe() (random choice) would be rewritten ?