* [PATCH] percpu: initialize best_upa variable @ 2021-05-15 18:08 trix 2021-05-16 23:45 ` Wonhyuk Yang 2021-05-17 2:05 ` Dennis Zhou 0 siblings, 2 replies; 8+ messages in thread From: trix @ 2021-05-15 18:08 UTC (permalink / raw) To: dennis, tj, cl, akpm; +Cc: linux-mm, linux-kernel, Tom Rix From: Tom Rix <trix@redhat.com> Static analysis reports this problem percpu.c:2945:6: warning: Assigned value is garbage or undefined upa = best_upa; ^ ~~~~~~~~ best_upa may not be set, so initialize it. Signed-off-by: Tom Rix <trix@redhat.com> --- mm/percpu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/percpu.c b/mm/percpu.c index a257c3efdf18b..6578b706fae81 100644 --- a/mm/percpu.c +++ b/mm/percpu.c @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( * Related to atom_size, which could be much larger than the unit_size. */ last_allocs = INT_MAX; + best_upa = max_upa; for (upa = max_upa; upa; upa--) { int allocs = 0, wasted = 0; -- 2.26.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-15 18:08 [PATCH] percpu: initialize best_upa variable trix @ 2021-05-16 23:45 ` Wonhyuk Yang 2021-05-17 2:05 ` Dennis Zhou 1 sibling, 0 replies; 8+ messages in thread From: Wonhyuk Yang @ 2021-05-16 23:45 UTC (permalink / raw) To: trix Cc: Dennis Zhou, Tejun Heo, Christoph Lameter, Andrew Morton, linux-mm, linux-kernel On Sun, May 16, 2021 at 3:08 AM <trix@redhat.com> wrote: > Static analysis reports this problem > percpu.c:2945:6: warning: Assigned value is garbage or undefined > upa = best_upa; > ^ ~~~~~~~~ > best_upa may not be set, so initialize it. Hi, Actually, best_upa is always set in the for loop below. when upa is 1, It will always satisfy all conditions. > diff --git a/mm/percpu.c b/mm/percpu.c > index a257c3efdf18b..6578b706fae81 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( > * Related to atom_size, which could be much larger than the unit_size. > */ > last_allocs = INT_MAX; > + best_upa = max_upa; > for (upa = max_upa; upa; upa--) { > int allocs = 0, wasted = 0; It doesn't seem to be a problem. But, how about this? best_upa = 1; for (upa = max_upa; upa>1; upa--) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-15 18:08 [PATCH] percpu: initialize best_upa variable trix 2021-05-16 23:45 ` Wonhyuk Yang @ 2021-05-17 2:05 ` Dennis Zhou 2021-05-17 11:06 ` Wonhyuk Yang 2021-05-17 13:17 ` Tom Rix 1 sibling, 2 replies; 8+ messages in thread From: Dennis Zhou @ 2021-05-17 2:05 UTC (permalink / raw) To: trix; +Cc: tj, cl, akpm, linux-mm, linux-kernel Hello, On Sat, May 15, 2021 at 11:08:17AM -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > Static analysis reports this problem > percpu.c:2945:6: warning: Assigned value is garbage or undefined > upa = best_upa; > ^ ~~~~~~~~ > best_upa may not be set, so initialize it. > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > mm/percpu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/percpu.c b/mm/percpu.c > index a257c3efdf18b..6578b706fae81 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( > * Related to atom_size, which could be much larger than the unit_size. > */ > last_allocs = INT_MAX; > + best_upa = max_upa; > for (upa = max_upa; upa; upa--) { > int allocs = 0, wasted = 0; > > -- > 2.26.3 > I think the proper fix would be: best_upa = 0; for (...) { } BUG_ON(!best_upa); upa = best_upa; If you're fine with this I'll make the changes and apply it to for-5.13-fixes. Can you also tell me what static analysis tool produced this? I'm just a little curious because this code hasn't changed in several years so I'd have expected some static analyzer to have caught this by now. Thanks, Dennis ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-17 2:05 ` Dennis Zhou @ 2021-05-17 11:06 ` Wonhyuk Yang 2021-05-17 13:17 ` Tom Rix 1 sibling, 0 replies; 8+ messages in thread From: Wonhyuk Yang @ 2021-05-17 11:06 UTC (permalink / raw) To: Dennis Zhou Cc: trix, Tejun Heo, Christoph Lameter, Andrew Morton, linux-mm, linux-kernel On Mon, May 17, 2021 at 11:05 AM Dennis Zhou <dennis@kernel.org> wrote: > > Can you also tell me what static analysis tool produced this? I'm just a > little curious because this code hasn't changed in several years so I'd > have expected some static analyzer to have caught this by now. > It's because uninitialize_var() was gone. It was introduced commit : 3f649ab728cda8("treewide: Remove uninitialized_var() usage") ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-17 2:05 ` Dennis Zhou 2021-05-17 11:06 ` Wonhyuk Yang @ 2021-05-17 13:17 ` Tom Rix 2021-05-17 14:39 ` Dennis Zhou 1 sibling, 1 reply; 8+ messages in thread From: Tom Rix @ 2021-05-17 13:17 UTC (permalink / raw) To: Dennis Zhou; +Cc: tj, cl, akpm, linux-mm, linux-kernel On 5/16/21 7:05 PM, Dennis Zhou wrote: > Hello, > > On Sat, May 15, 2021 at 11:08:17AM -0700, trix@redhat.com wrote: >> From: Tom Rix <trix@redhat.com> >> >> Static analysis reports this problem >> percpu.c:2945:6: warning: Assigned value is garbage or undefined >> upa = best_upa; >> ^ ~~~~~~~~ >> best_upa may not be set, so initialize it. >> >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> mm/percpu.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/mm/percpu.c b/mm/percpu.c >> index a257c3efdf18b..6578b706fae81 100644 >> --- a/mm/percpu.c >> +++ b/mm/percpu.c >> @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( >> * Related to atom_size, which could be much larger than the unit_size. >> */ >> last_allocs = INT_MAX; >> + best_upa = max_upa; >> for (upa = max_upa; upa; upa--) { >> int allocs = 0, wasted = 0; >> >> -- >> 2.26.3 >> > I think the proper fix would be: > > best_upa = 0; I was looking for initializing with something that would work. > for (...) { } > BUG_ON(!best_upa); WARN_ON instead? > upa = best_upa; > > If you're fine with this I'll make the changes and apply it to > for-5.13-fixes. > > Can you also tell me what static analysis tool produced this? I'm just a > little curious because this code hasn't changed in several years so I'd > have expected some static analyzer to have caught this by now. Clang 10 Tom > > Thanks, > Dennis > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-17 13:17 ` Tom Rix @ 2021-05-17 14:39 ` Dennis Zhou 2021-05-27 20:24 ` Dennis Zhou 0 siblings, 1 reply; 8+ messages in thread From: Dennis Zhou @ 2021-05-17 14:39 UTC (permalink / raw) To: Tom Rix; +Cc: tj, cl, akpm, linux-mm, linux-kernel On Mon, May 17, 2021 at 06:17:47AM -0700, Tom Rix wrote: > > On 5/16/21 7:05 PM, Dennis Zhou wrote: > > Hello, > > > > On Sat, May 15, 2021 at 11:08:17AM -0700, trix@redhat.com wrote: > > > From: Tom Rix <trix@redhat.com> > > > > > > Static analysis reports this problem > > > percpu.c:2945:6: warning: Assigned value is garbage or undefined > > > upa = best_upa; > > > ^ ~~~~~~~~ > > > best_upa may not be set, so initialize it. > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > --- > > > mm/percpu.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/mm/percpu.c b/mm/percpu.c > > > index a257c3efdf18b..6578b706fae81 100644 > > > --- a/mm/percpu.c > > > +++ b/mm/percpu.c > > > @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( > > > * Related to atom_size, which could be much larger than the unit_size. > > > */ > > > last_allocs = INT_MAX; > > > + best_upa = max_upa; > > > for (upa = max_upa; upa; upa--) { > > > int allocs = 0, wasted = 0; > > > -- > > > 2.26.3 > > > > > I think the proper fix would be: > > > > best_upa = 0; > > I was looking for initializing with something that would work. > I think I prefer setting it to 0 because it forces the loop to have succeeded vs being able to bypass it if the for loop logic was changed. > > for (...) { } > > BUG_ON(!best_upa); > WARN_ON instead? This is initialization code. So if upa == 0, it really is a problem. Having 0 units per allocation is bogus. > > upa = best_upa; > > > > If you're fine with this I'll make the changes and apply it to > > for-5.13-fixes. > > > > Can you also tell me what static analysis tool produced this? I'm just a > > little curious because this code hasn't changed in several years so I'd > > have expected some static analyzer to have caught this by now. > > Clang 10 > > Tom > Thanks, Dennis ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-17 14:39 ` Dennis Zhou @ 2021-05-27 20:24 ` Dennis Zhou 2021-05-27 21:09 ` Tom Rix 0 siblings, 1 reply; 8+ messages in thread From: Dennis Zhou @ 2021-05-27 20:24 UTC (permalink / raw) To: Tom Rix; +Cc: tj, cl, akpm, linux-mm, linux-kernel Hello, On Mon, May 17, 2021 at 02:39:21PM +0000, Dennis Zhou wrote: > On Mon, May 17, 2021 at 06:17:47AM -0700, Tom Rix wrote: > > > > On 5/16/21 7:05 PM, Dennis Zhou wrote: > > > Hello, > > > > > > On Sat, May 15, 2021 at 11:08:17AM -0700, trix@redhat.com wrote: > > > > From: Tom Rix <trix@redhat.com> > > > > > > > > Static analysis reports this problem > > > > percpu.c:2945:6: warning: Assigned value is garbage or undefined > > > > upa = best_upa; > > > > ^ ~~~~~~~~ > > > > best_upa may not be set, so initialize it. > > > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > > --- > > > > mm/percpu.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/mm/percpu.c b/mm/percpu.c > > > > index a257c3efdf18b..6578b706fae81 100644 > > > > --- a/mm/percpu.c > > > > +++ b/mm/percpu.c > > > > @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( > > > > * Related to atom_size, which could be much larger than the unit_size. > > > > */ > > > > last_allocs = INT_MAX; > > > > + best_upa = max_upa; > > > > for (upa = max_upa; upa; upa--) { > > > > int allocs = 0, wasted = 0; > > > > -- > > > > 2.26.3 > > > > > > > I think the proper fix would be: > > > > > > best_upa = 0; > > > > I was looking for initializing with something that would work. > > > > I think I prefer setting it to 0 because it forces the loop to have > succeeded vs being able to bypass it if the for loop logic was changed. > > > > for (...) { } > > > BUG_ON(!best_upa); > > WARN_ON instead? > > This is initialization code. So if upa == 0, it really is a problem. > Having 0 units per allocation is bogus. > > > > upa = best_upa; > > > > > > If you're fine with this I'll make the changes and apply it to > > > for-5.13-fixes. > > > > > > Can you also tell me what static analysis tool produced this? I'm just a > > > little curious because this code hasn't changed in several years so I'd > > > have expected some static analyzer to have caught this by now. > > > > Clang 10 > > > > Tom > > > > Thanks, > Dennis Following up here. Are you find with me making the changes and attributing it to you? Otherwise I can just spin another patch real quick. At this point I've already sent my PR for-5.13-fixes. So I'll queue some fix for-5.14. Thanks, Dennis ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] percpu: initialize best_upa variable 2021-05-27 20:24 ` Dennis Zhou @ 2021-05-27 21:09 ` Tom Rix 0 siblings, 0 replies; 8+ messages in thread From: Tom Rix @ 2021-05-27 21:09 UTC (permalink / raw) To: Dennis Zhou; +Cc: tj, cl, akpm, linux-mm, linux-kernel On 5/27/21 1:24 PM, Dennis Zhou wrote: > Hello, > > On Mon, May 17, 2021 at 02:39:21PM +0000, Dennis Zhou wrote: >> On Mon, May 17, 2021 at 06:17:47AM -0700, Tom Rix wrote: >>> On 5/16/21 7:05 PM, Dennis Zhou wrote: >>>> Hello, >>>> >>>> On Sat, May 15, 2021 at 11:08:17AM -0700, trix@redhat.com wrote: >>>>> From: Tom Rix <trix@redhat.com> >>>>> >>>>> Static analysis reports this problem >>>>> percpu.c:2945:6: warning: Assigned value is garbage or undefined >>>>> upa = best_upa; >>>>> ^ ~~~~~~~~ >>>>> best_upa may not be set, so initialize it. >>>>> >>>>> Signed-off-by: Tom Rix <trix@redhat.com> >>>>> --- >>>>> mm/percpu.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/mm/percpu.c b/mm/percpu.c >>>>> index a257c3efdf18b..6578b706fae81 100644 >>>>> --- a/mm/percpu.c >>>>> +++ b/mm/percpu.c >>>>> @@ -2916,6 +2916,7 @@ static struct pcpu_alloc_info * __init __flatten pcpu_build_alloc_info( >>>>> * Related to atom_size, which could be much larger than the unit_size. >>>>> */ >>>>> last_allocs = INT_MAX; >>>>> + best_upa = max_upa; >>>>> for (upa = max_upa; upa; upa--) { >>>>> int allocs = 0, wasted = 0; >>>>> -- >>>>> 2.26.3 >>>>> >>>> I think the proper fix would be: >>>> >>>> best_upa = 0; >>> I was looking for initializing with something that would work. >>> >> I think I prefer setting it to 0 because it forces the loop to have >> succeeded vs being able to bypass it if the for loop logic was changed. >> >>>> for (...) { } >>>> BUG_ON(!best_upa); >>> WARN_ON instead? >> This is initialization code. So if upa == 0, it really is a problem. >> Having 0 units per allocation is bogus. >> >>>> upa = best_upa; >>>> >>>> If you're fine with this I'll make the changes and apply it to >>>> for-5.13-fixes. >>>> >>>> Can you also tell me what static analysis tool produced this? I'm just a >>>> little curious because this code hasn't changed in several years so I'd >>>> have expected some static analyzer to have caught this by now. >>> Clang 10 >>> >>> Tom >>> >> Thanks, >> Dennis > Following up here. Are you find with me making the changes and > attributing it to you? Otherwise I can just spin another patch real > quick. I am fine with you respinning, no need to attribute the change to me. If you would like a review, include me on the cc. Thanks! Tom > At this point I've already sent my PR for-5.13-fixes. So I'll queue some > fix for-5.14. > > Thanks, > Dennis > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-27 21:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-15 18:08 [PATCH] percpu: initialize best_upa variable trix 2021-05-16 23:45 ` Wonhyuk Yang 2021-05-17 2:05 ` Dennis Zhou 2021-05-17 11:06 ` Wonhyuk Yang 2021-05-17 13:17 ` Tom Rix 2021-05-17 14:39 ` Dennis Zhou 2021-05-27 20:24 ` Dennis Zhou 2021-05-27 21:09 ` Tom Rix
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).