> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: 21-Mar-19 12:45
> To: Nick Desaulniers <[email protected]>
> Cc: Nathan Chancellor <[email protected]>; Jon Maloy
> <[email protected]>; Ying Xue <[email protected]>; David S.
> Miller <[email protected]>; [email protected];
> Networking <[email protected]>; LKML <linux-
> [email protected]>; [email protected]
> 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
> <[email protected]> wrote:
> > On Wed, Mar 20, 2019 at 12:07 PM Nathan Chancellor
> > <[email protected]> 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
_______________________________________________
tipc-discussion mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tipc-discussion
