[v1] net: tipc: avoid possible garbage value

[PATCH net-next] net: tipc: avoid possible garbage value

Posted by Su Hui 2 months, 2 weeks ago

Clang static checker (scan-build) warning:
net/tipc/bcast.c:305:4:
The expression is an uninitialized value. The computed value will also
be garbage [core.uninitialized.Assign]
  305 |                         (*cong_link_cnt)++;
      |                         ^~~~~~~~~~~~~~~~~~

tipc_rcast_xmit() will increase cong_link_cnt's value, but cong_link_cnt
is uninitialized. Although it won't really cause a problem, it's better
to fix it.

Fixes: dca4a17d24ee ("tipc: fix potential hanging after b/rcast changing")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 net/tipc/bcast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 593846d25214..a3699be6a634 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -321,7 +321,7 @@ static int tipc_mcast_send_sync(struct net *net, struct sk_buff *skb,
 	struct tipc_msg *hdr, *_hdr;
 	struct sk_buff_head tmpq;
 	struct sk_buff *_skb;
-	u16 cong_link_cnt;
+	u16 cong_link_cnt = 0;
 	int rc = 0;
 
 	/* Is a cluster supporting with new capabilities ? */
-- 
2.30.2

Re: [PATCH net-next] net: tipc: avoid possible garbage value

Posted by Simon Horman 2 months, 2 weeks ago

On Thu, Sep 12, 2024 at 07:01:20PM +0800, Su Hui wrote:
> Clang static checker (scan-build) warning:
> net/tipc/bcast.c:305:4:
> The expression is an uninitialized value. The computed value will also
> be garbage [core.uninitialized.Assign]
>   305 |                         (*cong_link_cnt)++;
>       |                         ^~~~~~~~~~~~~~~~~~
> 
> tipc_rcast_xmit() will increase cong_link_cnt's value, but cong_link_cnt
> is uninitialized. Although it won't really cause a problem, it's better
> to fix it.
> 
> Fixes: dca4a17d24ee ("tipc: fix potential hanging after b/rcast changing")
> Signed-off-by: Su Hui <suhui@nfschina.com>

Hi Su Hui,

This looks like a bug fix. If so it should be targeted at net rather than
net-next. If not, the Fixes tag should be dropped, and the commit can be
referenced in the patch description with some other text around:

commit dca4a17d24ee ("tipc: fix potential hanging after b/rcast changing")

> ---
>  net/tipc/bcast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 593846d25214..a3699be6a634 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -321,7 +321,7 @@ static int tipc_mcast_send_sync(struct net *net, struct sk_buff *skb,
>  	struct tipc_msg *hdr, *_hdr;
>  	struct sk_buff_head tmpq;
>  	struct sk_buff *_skb;
> -	u16 cong_link_cnt;
> +	u16 cong_link_cnt = 0;
>  	int rc = 0;

I think we should preserve reverse xmas tree order - longest like to
shortest - for these local variable declarations.

>  
>  	/* Is a cluster supporting with new capabilities ? */
> -- 
> 2.30.2
> 
>

Re: [PATCH net-next] net: tipc: avoid possible garbage value

Posted by Dan Carpenter 2 months, 2 weeks ago

On Sat, Sep 14, 2024 at 10:42:44AM +0100, Simon Horman wrote:
> On Thu, Sep 12, 2024 at 07:01:20PM +0800, Su Hui wrote:
> > Clang static checker (scan-build) warning:
> > net/tipc/bcast.c:305:4:
> > The expression is an uninitialized value. The computed value will also
> > be garbage [core.uninitialized.Assign]
> >   305 |                         (*cong_link_cnt)++;
> >       |                         ^~~~~~~~~~~~~~~~~~
> > 
> > tipc_rcast_xmit() will increase cong_link_cnt's value, but cong_link_cnt
> > is uninitialized. Although it won't really cause a problem, it's better
> > to fix it.
> > 
> > Fixes: dca4a17d24ee ("tipc: fix potential hanging after b/rcast changing")
> > Signed-off-by: Su Hui <suhui@nfschina.com>
> 
> Hi Su Hui,
> 
> This looks like a bug fix. If so it should be targeted at net rather than
> net-next. If not, the Fixes tag should be dropped, and the commit can be
> referenced in the patch description with some other text around:
> 


It's one of those borderline things.  As the commit message says it doesn't
really cause a problem because cong_link_cnt is never used.  I guess if you had
UBSan turned on it would generate a runtime warning.  Still it also doesn't seem
intentional so I would probably count it as a bugfix and target net like you
suggest.

regards,
dan carpenter

Re: [PATCH net-next] net: tipc: avoid possible garbage value

Posted by Su Hui 2 months, 2 weeks ago

On 2024/9/14 18:05, Dan Carpenter wrote:

> On Sat, Sep 14, 2024 at 10:42:44AM +0100, Simon Horman wrote:
>> On Thu, Sep 12, 2024 at 07:01:20PM +0800, Su Hui wrote:
>>> Clang static checker (scan-build) warning:
>>> net/tipc/bcast.c:305:4:
>>> The expression is an uninitialized value. The computed value will also
>>> be garbage [core.uninitialized.Assign]
>>>    305 |                         (*cong_link_cnt)++;
>>>        |                         ^~~~~~~~~~~~~~~~~~
>>>
>>> tipc_rcast_xmit() will increase cong_link_cnt's value, but cong_link_cnt
>>> is uninitialized. Although it won't really cause a problem, it's better
>>> to fix it.
>>>
>>> Fixes: dca4a17d24ee ("tipc: fix potential hanging after b/rcast changing")
>>> Signed-off-by: Su Hui <suhui@nfschina.com>
>> Hi Su Hui,
>>
>> This looks like a bug fix. If so it should be targeted at net rather than
>> net-next. If not, the Fixes tag should be dropped, and the commit can be
>> referenced in the patch description with some other text around:
>>
>
> It's one of those borderline things.  As the commit message says it doesn't
> really cause a problem because cong_link_cnt is never used.  I guess if you had
> UBSan turned on it would generate a runtime warning.  Still it also doesn't seem
> intentional so I would probably count it as a bugfix and target net like you
> suggest.
Got it. I will send a v2 patch to net and  keeping reverse xmas tree order.
Thanks for the suggestions:).

Su Hui

Re: [PATCH net-next] net: tipc: avoid possible garbage value

Posted by Justin Stitt 2 months, 2 weeks ago

Hi,

On Thu, Sep 12, 2024 at 4:01 AM Su Hui <suhui@nfschina.com> wrote:
>
> Clang static checker (scan-build) warning:
> net/tipc/bcast.c:305:4:
> The expression is an uninitialized value. The computed value will also
> be garbage [core.uninitialized.Assign]
>   305 |                         (*cong_link_cnt)++;
>       |                         ^~~~~~~~~~~~~~~~~~
>
> tipc_rcast_xmit() will increase cong_link_cnt's value, but cong_link_cnt
> is uninitialized. Although it won't really cause a problem, it's better
> to fix it.

Agreed.

Reviewed-by: Justin Stitt <justinstitt@google.com>
>
> Fixes: dca4a17d24ee ("tipc: fix potential hanging after b/rcast changing")
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  net/tipc/bcast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 593846d25214..a3699be6a634 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -321,7 +321,7 @@ static int tipc_mcast_send_sync(struct net *net, struct sk_buff *skb,
>         struct tipc_msg *hdr, *_hdr;
>         struct sk_buff_head tmpq;
>         struct sk_buff *_skb;
> -       u16 cong_link_cnt;
> +       u16 cong_link_cnt = 0;
>         int rc = 0;
>
>         /* Is a cluster supporting with new capabilities ? */
> --
> 2.30.2
>

Thanks
Justin