From: Tom Rix <[email protected]>
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 <[email protected]>
---
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
On Sun, May 16, 2021 at 3:08 AM <[email protected]> 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, [email protected] wrote:
> From: Tom Rix <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]> 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, [email protected] wrote:
>> From: Tom Rix <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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, [email protected] wrote:
> > > From: Tom Rix <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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, [email protected] wrote:
> > > > From: Tom Rix <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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, [email protected] wrote:
>>>>> From: Tom Rix <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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
>