-Wsometimes-uninitialized Clang warning in net/tipc/node.c

-Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Nathan Chancellor]

Hi all,

We are trying to get Clang's -Wsometimes-uninitialized turned on for the
kernel as it can catch some bugs that GCC can't. This warning came up:

net/tipc/node.c:831:6: warning: variable 'maddr' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
        if (!tipc_link_is_establishing(l)) {
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/tipc/node.c:847:46: note: uninitialized use occurs here
        tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
                                                    ^~~~~
net/tipc/node.c:831:2: note: remove the 'if' if its condition is always true
        if (!tipc_link_is_establishing(l)) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/tipc/node.c:821:31: note: initialize the variable 'maddr' to silence this warning
        struct tipc_media_addr *maddr;
                                     ^
                                      = NULL
1 warning generated.

This definitely appears to be a legitimate warning but I'm not sure of
the proper solution (should maddr be initialized to NULL or should it be
set to something different in the else branch). Your input would be
greatly appreciated.

Cheers,
Nathan

Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Nathan Chancellor]

On Thu, Mar 07, 2019 at 05:17:23PM -0700, Nathan Chancellor wrote:
> Hi all,
> 
> We are trying to get Clang's -Wsometimes-uninitialized turned on for the
> kernel as it can catch some bugs that GCC can't. This warning came up:
> 
> net/tipc/node.c:831:6: warning: variable 'maddr' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>         if (!tipc_link_is_establishing(l)) {
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> net/tipc/node.c:847:46: note: uninitialized use occurs here
>         tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
>                                                     ^~~~~
> net/tipc/node.c:831:2: note: remove the 'if' if its condition is always true
>         if (!tipc_link_is_establishing(l)) {
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> net/tipc/node.c:821:31: note: initialize the variable 'maddr' to silence this warning
>         struct tipc_media_addr *maddr;
>                                      ^
>                                       = NULL
> 1 warning generated.
> 
> This definitely appears to be a legitimate warning but I'm not sure of
> the proper solution (should maddr be initialized to NULL or should it be
> set to something different in the else branch). Your input would be
> greatly appreciated.
> 
> Cheers,
> Nathan

Gentle ping (if there was a response to this, I didn't receive it). I
know I sent it in the middle of a merge window so I get if it slipped
through the cracks.

Thanks,
Nathan

Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Nick Desaulniers]

On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Mar 07, 2019 at 05:17:23PM -0700, Nathan Chancellor wrote:
> > Hi all,
> >
> > We are trying to get Clang's -Wsometimes-uninitialized turned on for the
> > kernel as it can catch some bugs that GCC can't. This warning came up:
> >
> > net/tipc/node.c:831:6: warning: variable 'maddr' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> >         if (!tipc_link_is_establishing(l)) {
> >             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > net/tipc/node.c:847:46: note: uninitialized use occurs here
> >         tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> >                                                     ^~~~~
> > net/tipc/node.c:831:2: note: remove the 'if' if its condition is always true
> >         if (!tipc_link_is_establishing(l)) {
> >         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > net/tipc/node.c:821:31: note: initialize the variable 'maddr' to silence this warning
> >         struct tipc_media_addr *maddr;
> >                                      ^
> >                                       = NULL
> > 1 warning generated.
> >
> > This definitely appears to be a legitimate warning but I'm not sure of
> > the proper solution (should maddr be initialized to NULL or should it be
> > set to something different in the else branch). Your input would be
> > greatly appreciated.
> >
> > Cheers,
> > Nathan
>
> Gentle ping (if there was a response to this, I didn't receive it). I
> know I sent it in the middle of a merge window so I get if it slipped
> through the cracks.
>
> Thanks,
> Nathan

The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out
parameter, so even if the if is taken doesn't guarantee that maddr is
always initialized before calling tipc_bearer_xmit().

At the minimum, we should initialize maddr to NULL.  I think we'd
prefer to risk the possibility of a null pointer dereference to the
possibility of working with uninitialized memory.  To be clear, both
are bad, but one is easier to spot/debug later than the other.

Thanks for bringing this up for discussion, sorry I missed it before.
-- 
Thanks,
~Nick Desaulniers

Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Arnd Bergmann]

On Wed, Mar 20, 2019 at 9:51 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
> On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
>
> The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out
> parameter, so even if the if is taken doesn't guarantee that maddr is
> always initialized before calling tipc_bearer_xmit().

Right, it is only initialized in certain states. It was always
initialized until commit 598411d70f85 ("tipc: make resetting of
links non-atomic"), afterwards only if the link was not reset,
and as of commit 73f646cec354 ("tipc: delay ESTABLISH
state event when link is established") only if it's not
'establishing' or 'reset'.

> At the minimum, we should initialize maddr to NULL.  I think we'd
> prefer to risk the possibility of a null pointer dereference to the
> possibility of working with uninitialized memory.  To be clear, both
> are bad, but one is easier to spot/debug later than the other.

I disagree with setting it to NULL, given that it is still an obviously
incorrect value. We could add a if(maddr) check before calling
tipc_bearer_xmit(), but I think it would be clearer to check
skb_queue_empty(xmitq)) if that avoids the warning:

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 2dc4919ab23c..147786795e48 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node
*n, int bearer_id, bool delete)
        tipc_node_write_unlock(n);
        if (delete)
                tipc_mon_remove_peer(n->net, n->addr, old_bearer_id);
-       tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
+       if (skb_queue_empty(xmitq))
+               tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
        tipc_sk_rcv(n->net, &le->inputq);
 }

This duplicates the check inside of skb_queue_empty(),
but I don't know if the compiler can see through the
logic behind the inlined function calls.

      Arnd

RE: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Jon Maloy]

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: 21-Mar-19 12:45
> To: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <natechancellor@gmail.com>; Jon Maloy
> <jon.maloy@ericsson.com>; Ying Xue <ying.xue@windriver.com>; David S.
> Miller <davem@davemloft.net>; tipc-discussion@lists.sourceforge.net;
> Networking <netdev@vger.kernel.org>; LKML <linux-
> kernel@vger.kernel.org>; clang-built-linux@googlegroups.com
> Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c
> 
> On Wed, Mar 20, 2019 at 9:51 PM 'Nick Desaulniers' via Clang Built Linux
> <clang-built-linux@googlegroups.com> wrote:
> > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> >
> > The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out
> > parameter, so even if the if is taken doesn't guarantee that maddr is
> > always initialized before calling tipc_bearer_xmit().
> 
> Right, it is only initialized in certain states. It was always initialized until
> commit 598411d70f85 ("tipc: make resetting of links non-atomic"),
> afterwards only if the link was not reset, and as of commit 73f646cec354
> ("tipc: delay ESTABLISH state event when link is established") only if it's not
> 'establishing' or 'reset'.
> 
> > At the minimum, we should initialize maddr to NULL.  I think we'd
> > prefer to risk the possibility of a null pointer dereference to the
> > possibility of working with uninitialized memory.  To be clear, both
> > are bad, but one is easier to spot/debug later than the other.
> 
> I disagree with setting it to NULL, given that it is still an obviously incorrect
> value. We could add a if(maddr) check before calling tipc_bearer_xmit(), but
> I think it would be clearer to check
> skb_queue_empty(xmitq)) if that avoids the warning:

I may be wrong, but I would be surprised if that would eliminate the warning.
To me, setting maddr to NULL and then testing for it looks both more comprehensible and is guaranteed to fix the warning.

> 
> diff --git a/net/tipc/node.c b/net/tipc/node.c index
> 2dc4919ab23c..147786795e48 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node *n,
> int bearer_id, bool delete)
>         tipc_node_write_unlock(n);
>         if (delete)
>                 tipc_mon_remove_peer(n->net, n->addr, old_bearer_id);
> -       tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> +       if (skb_queue_empty(xmitq))
> +               tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
>         tipc_sk_rcv(n->net, &le->inputq);  }
> 
> This duplicates the check inside of skb_queue_empty(), but I don't know if
> the compiler can see through the logic behind the inlined function calls.

Probably not, but this is not in any way time critical.

///jon

> 
>       Arnd

Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Arnd Bergmann]

On Thu, Mar 21, 2019 at 3:57 PM Jon Maloy <jon.maloy@ericsson.com> wrote:
> > -----Original Message-----
> > From: Arnd Bergmann <arnd@arndb.de>
> > Sent: 21-Mar-19 12:45
> > To: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Nathan Chancellor <natechancellor@gmail.com>; Jon Maloy
> > <jon.maloy@ericsson.com>; Ying Xue <ying.xue@windriver.com>; David S.
> > Miller <davem@davemloft.net>; tipc-discussion@lists.sourceforge.net;
> > Networking <netdev@vger.kernel.org>; LKML <linux-
> > kernel@vger.kernel.org>; clang-built-linux@googlegroups.com
> > Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c
> >
> > On Wed, Mar 20, 2019 at 9:51 PM 'Nick Desaulniers' via Clang Built Linux
> > <clang-built-linux@googlegroups.com> wrote:
> > > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > >
> > > The use in tipc_bearer_xmit() isn't even guaranteed to set the in/out
> > > parameter, so even if the if is taken doesn't guarantee that maddr is
> > > always initialized before calling tipc_bearer_xmit().
> >
> > Right, it is only initialized in certain states. It was always initialized until
> > commit 598411d70f85 ("tipc: make resetting of links non-atomic"),
> > afterwards only if the link was not reset, and as of commit 73f646cec354
> > ("tipc: delay ESTABLISH state event when link is established") only if it's not
> > 'establishing' or 'reset'.
> >
> > > At the minimum, we should initialize maddr to NULL.  I think we'd
> > > prefer to risk the possibility of a null pointer dereference to the
> > > possibility of working with uninitialized memory.  To be clear, both
> > > are bad, but one is easier to spot/debug later than the other.
> >
> > I disagree with setting it to NULL, given that it is still an obviously incorrect
> > value. We could add a if(maddr) check before calling tipc_bearer_xmit(), but
> > I think it would be clearer to check
> > skb_queue_empty(xmitq)) if that avoids the warning:
>
> I may be wrong, but I would be surprised if that would eliminate the warning.
> To me, setting maddr to NULL and then testing for it looks both more comprehensible and is guaranteed to fix the warning.
>
> >
> > diff --git a/net/tipc/node.c b/net/tipc/node.c index
> > 2dc4919ab23c..147786795e48 100644
> > --- a/net/tipc/node.c
> > +++ b/net/tipc/node.c
> > @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node *n,
> > int bearer_id, bool delete)
> >         tipc_node_write_unlock(n);
> >         if (delete)
> >                 tipc_mon_remove_peer(n->net, n->addr, old_bearer_id);
> > -       tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> > +       if (skb_queue_empty(xmitq))
> > +               tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> >         tipc_sk_rcv(n->net, &le->inputq);  }
> >
> > This duplicates the check inside of skb_queue_empty(), but I don't know if
> > the compiler can see through the logic behind the inlined function calls.
>
> Probably not, but this is not in any way time critical.

I meant it's unclear whether compilers should be expected to see that
skb_queue_empty() is true after the call to __skb_queue_head_init()
initializes it.

I think recent versions of gcc do see that, not sure about clang, as it
does inlining differently. tipc_bearer_xmit() cannot be inlined here
(unless we start using LTO).

       Arnd

Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Arnd Bergmann]

On Thu, Mar 21, 2019 at 4:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Mar 21, 2019 at 3:57 PM Jon Maloy <jon.maloy@ericsson.com> wrote:

> > >
> > > diff --git a/net/tipc/node.c b/net/tipc/node.c index
> > > 2dc4919ab23c..147786795e48 100644
> > > --- a/net/tipc/node.c
> > > +++ b/net/tipc/node.c
> > > @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct tipc_node *n,
> > > int bearer_id, bool delete)
> > >         tipc_node_write_unlock(n);
> > >         if (delete)
> > >                 tipc_mon_remove_peer(n->net, n->addr, old_bearer_id);
> > > -       tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> > > +       if (skb_queue_empty(xmitq))
> > > +               tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> > >         tipc_sk_rcv(n->net, &le->inputq);  }
> > >
> > > This duplicates the check inside of skb_queue_empty(), but I don't know if
> > > the compiler can see through the logic behind the inlined function calls.
> >
> > Probably not, but this is not in any way time critical.
>
> I meant it's unclear whether compilers should be expected to see that
> skb_queue_empty() is true after the call to __skb_queue_head_init()
> initializes it.

I reproduced the warning now, and verified that my change
eliminates the warning. I still think that is the more logical
solution here.

      Arnd

RE: -Wsometimes-uninitialized Clang warning in net/tipc/node.c

[Jon Maloy]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> On Behalf Of Arnd Bergmann
> Sent: 21-Mar-19 19:23
> To: Jon Maloy <jon.maloy@ericsson.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>; Nathan Chancellor
> <natechancellor@gmail.com>; Ying Xue <ying.xue@windriver.com>; David S.
> Miller <davem@davemloft.net>; tipc-discussion@lists.sourceforge.net;
> Networking <netdev@vger.kernel.org>; LKML <linux-
> kernel@vger.kernel.org>; clang-built-linux@googlegroups.com
> Subject: Re: -Wsometimes-uninitialized Clang warning in net/tipc/node.c
> 
> On Thu, Mar 21, 2019 at 4:25 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Thu, Mar 21, 2019 at 3:57 PM Jon Maloy <jon.maloy@ericsson.com>
> wrote:
> 
> > > >
> > > > diff --git a/net/tipc/node.c b/net/tipc/node.c index
> > > > 2dc4919ab23c..147786795e48 100644
> > > > --- a/net/tipc/node.c
> > > > +++ b/net/tipc/node.c
> > > > @@ -844,7 +844,8 @@ static void tipc_node_link_down(struct
> > > > tipc_node *n, int bearer_id, bool delete)
> > > >         tipc_node_write_unlock(n);
> > > >         if (delete)
> > > >                 tipc_mon_remove_peer(n->net, n->addr, old_bearer_id);
> > > > -       tipc_bearer_xmit(n->net, bearer_id, &xmitq, maddr);
> > > > +       if (skb_queue_empty(xmitq))
> > > > +               tipc_bearer_xmit(n->net, bearer_id, &xmitq,
> > > > + maddr);
> > > >         tipc_sk_rcv(n->net, &le->inputq);  }
> > > >
> > > > This duplicates the check inside of skb_queue_empty(), but I don't
> > > > know if the compiler can see through the logic behind the inlined
> function calls.
> > >
> > > Probably not, but this is not in any way time critical.
> >
> > I meant it's unclear whether compilers should be expected to see that
> > skb_queue_empty() is true after the call to __skb_queue_head_init()
> > initializes it.
> 
> I reproduced the warning now, and verified that my change eliminates the
> warning. I still think that is the more logical solution here.

Ok.  No problems with that.

///jon

> 
>       Arnd