| Message ID | 20240912110119.2025503-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: tipc: avoid possible garbage value | expand |
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
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 > >
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
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
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 ? */
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(-)