| Message ID | 20210515180817.1751084-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | percpu: initialize best_upa variable | expand |
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--)
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
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")
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 >
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
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
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 >
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;