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

[Jon Maloy]

> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxx>
> Sent: 21-Mar-19 12:45
> To: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> Cc: Nathan Chancellor <natechancellor@xxxxxxxxx>; Jon Maloy
> <jon.maloy@xxxxxxxxxxxx>; Ying Xue <ying.xue@xxxxxxxxxxxxx>; David S.
> Miller <davem@xxxxxxxxxxxxx>; tipc-discussion@xxxxxxxxxxxxxxxxxxxxx;
> Networking <netdev@xxxxxxxxxxxxxxx>; LKML <linux-
> kernel@xxxxxxxxxxxxxxx>; clang-built-linux@xxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxx> wrote:
> > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
> > <natechancellor@xxxxxxxxx> 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