* [patch] kexec: potetially using uninitialized variable
@ 2016-03-11 8:07 Dan Carpenter
2016-03-11 8:52 ` Xunlei Pang
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2016-03-11 8:07 UTC (permalink / raw)
To: Andrew Morton, Vivek Goyal
Cc: Dave Young, Xunlei Pang, Ingo Molnar, Toshi Kani, Mimi Zohar,
Minfei Huang, linux-kernel, kernel-janitors
At the end of the function we check if "ret" has a negative error code,
but it seems possible that it is uninitialized.
Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 503bc2d..63d1af3 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -795,7 +795,7 @@ out:
static int kexec_apply_relocations(struct kimage *image)
{
- int i, ret;
+ int i, ret = 0;
struct purgatory_info *pi = &image->purgatory_info;
Elf_Shdr *sechdrs = pi->sechdrs;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch] kexec: potetially using uninitialized variable
2016-03-11 8:07 [patch] kexec: potetially using uninitialized variable Dan Carpenter
@ 2016-03-11 8:52 ` Xunlei Pang
2016-03-11 9:19 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Xunlei Pang @ 2016-03-11 8:52 UTC (permalink / raw)
To: Dan Carpenter, Andrew Morton, Vivek Goyal
Cc: Dave Young, Xunlei Pang, Ingo Molnar, Toshi Kani, Mimi Zohar,
Minfei Huang, linux-kernel, kernel-janitors
Hi Dan,
On 2016/03/11 at 16:07, Dan Carpenter wrote:
> At the end of the function we check if "ret" has a negative error code,
> but it seems possible that it is uninitialized.
>
> Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 503bc2d..63d1af3 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -795,7 +795,7 @@ out:
>
> static int kexec_apply_relocations(struct kimage *image)
> {
> - int i, ret;
> + int i, ret = 0;
> struct purgatory_info *pi = &image->purgatory_info;
> Elf_Shdr *sechdrs = pi->sechdrs;
>
Look further, there is a condition at the beginning of the for loop:
if (sechdrs[i].sh_type != SHT_RELA &&
sechdrs[i].sh_type != SHT_REL)
continue;
So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.
Regards,
Xunlei
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] kexec: potetially using uninitialized variable
2016-03-11 8:52 ` Xunlei Pang
@ 2016-03-11 9:19 ` Dan Carpenter
2016-03-11 9:47 ` walter harms
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2016-03-11 9:19 UTC (permalink / raw)
To: xlpang
Cc: Andrew Morton, Vivek Goyal, Dave Young, Ingo Molnar, Toshi Kani,
Mimi Zohar, Minfei Huang, linux-kernel, kernel-janitors
On Fri, Mar 11, 2016 at 04:52:43PM +0800, Xunlei Pang wrote:
> Hi Dan,
>
> On 2016/03/11 at 16:07, Dan Carpenter wrote:
> > At the end of the function we check if "ret" has a negative error code,
> > but it seems possible that it is uninitialized.
> >
> > Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 503bc2d..63d1af3 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -795,7 +795,7 @@ out:
> >
> > static int kexec_apply_relocations(struct kimage *image)
> > {
> > - int i, ret;
> > + int i, ret = 0;
> > struct purgatory_info *pi = &image->purgatory_info;
> > Elf_Shdr *sechdrs = pi->sechdrs;
> >
>
> Look further, there is a condition at the beginning of the for loop:
>
>
> if (sechdrs[i].sh_type != SHT_RELA &&
> sechdrs[i].sh_type != SHT_REL)
> continue;
>
> So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.
Ah, right...
This wasn't a GCC warning. GCC misses a lot of uninitialized variable
bugs so I'm doing this with Smatch.
Anyway, I'll patch this up in Smatch to not warn about this.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] kexec: potetially using uninitialized variable
2016-03-11 9:19 ` Dan Carpenter
@ 2016-03-11 9:47 ` walter harms
2016-03-11 15:38 ` Minfei Huang
0 siblings, 1 reply; 7+ messages in thread
From: walter harms @ 2016-03-11 9:47 UTC (permalink / raw)
To: Dan Carpenter
Cc: xlpang, Andrew Morton, Vivek Goyal, Dave Young, Ingo Molnar,
Toshi Kani, Mimi Zohar, Minfei Huang, linux-kernel,
kernel-janitors
Am 11.03.2016 10:19, schrieb Dan Carpenter:
> On Fri, Mar 11, 2016 at 04:52:43PM +0800, Xunlei Pang wrote:
>> Hi Dan,
>>
>> On 2016/03/11 at 16:07, Dan Carpenter wrote:
>>> At the end of the function we check if "ret" has a negative error code,
>>> but it seems possible that it is uninitialized.
>>>
>>> Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
>>> index 503bc2d..63d1af3 100644
>>> --- a/kernel/kexec_file.c
>>> +++ b/kernel/kexec_file.c
>>> @@ -795,7 +795,7 @@ out:
>>>
>>> static int kexec_apply_relocations(struct kimage *image)
>>> {
>>> - int i, ret;
>>> + int i, ret = 0;
>>> struct purgatory_info *pi = &image->purgatory_info;
>>> Elf_Shdr *sechdrs = pi->sechdrs;
>>>
>>
>> Look further, there is a condition at the beginning of the for loop:
>>
>>
>> if (sechdrs[i].sh_type != SHT_RELA &&
>> sechdrs[i].sh_type != SHT_REL)
>> continue;
>>
>> So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.
>
> Ah, right...
>
> This wasn't a GCC warning. GCC misses a lot of uninitialized variable
> bugs so I'm doing this with Smatch.
>
> Anyway, I'll patch this up in Smatch to not warn about this.
>
I am not so sure about this. the point should be that the reviewer can read it easily
not if gcc complains or not.
just my 2 cents,
re,
wh
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] kexec: potetially using uninitialized variable
2016-03-11 9:47 ` walter harms
@ 2016-03-11 15:38 ` Minfei Huang
2016-03-14 10:58 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Minfei Huang @ 2016-03-11 15:38 UTC (permalink / raw)
To: walter harms
Cc: Dan Carpenter, xlpang, Andrew Morton, Vivek Goyal, Dave Young,
Ingo Molnar, Toshi Kani, Mimi Zohar, linux-kernel,
kernel-janitors
On 03/11/16 at 10:47am, walter harms wrote:
>
>
> Am 11.03.2016 10:19, schrieb Dan Carpenter:
> > On Fri, Mar 11, 2016 at 04:52:43PM +0800, Xunlei Pang wrote:
> >> Hi Dan,
> >>
> >> On 2016/03/11 at 16:07, Dan Carpenter wrote:
> >>> At the end of the function we check if "ret" has a negative error code,
> >>> but it seems possible that it is uninitialized.
> >>>
> >>> Fixes: 12db5562e035 ('kexec: load and relocate purgatory at kernel load time')
> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>>
> >>> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> >>> index 503bc2d..63d1af3 100644
> >>> --- a/kernel/kexec_file.c
> >>> +++ b/kernel/kexec_file.c
> >>> @@ -795,7 +795,7 @@ out:
> >>>
> >>> static int kexec_apply_relocations(struct kimage *image)
> >>> {
> >>> - int i, ret;
> >>> + int i, ret = 0;
> >>> struct purgatory_info *pi = &image->purgatory_info;
> >>> Elf_Shdr *sechdrs = pi->sechdrs;
> >>>
> >>
> >> Look further, there is a condition at the beginning of the for loop:
> >>
> >>
> >> if (sechdrs[i].sh_type != SHT_RELA &&
> >> sechdrs[i].sh_type != SHT_REL)
> >> continue;
> >>
> >> So, I think that's ok, but I don't konw if GCC is smart enough not to throw warnings.
> >
> > Ah, right...
> >
> > This wasn't a GCC warning. GCC misses a lot of uninitialized variable
> > bugs so I'm doing this with Smatch.
> >
> > Anyway, I'll patch this up in Smatch to not warn about this.
> >
>
> I am not so sure about this. the point should be that the reviewer can read it easily
> not if gcc complains or not.
Hi, All.
I think we can modify the logic a bit to make code simple. Thus gcc will
not complain about any more, and the logic is earier.
Following is a draft patch.
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 007b791..7144e3b 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -887,7 +887,7 @@ static int kexec_apply_relocations(struct kimage *image)
if (sechdrs[i].sh_type == SHT_RELA)
ret = arch_kexec_apply_relocations_add(pi->ehdr,
sechdrs, i);
- else if (sechdrs[i].sh_type == SHT_REL)
+ else
ret = arch_kexec_apply_relocations(pi->ehdr,
sechdrs, i);
if (ret)
>
> just my 2 cents,
>
> re,
> wh
>
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [patch] kexec: potetially using uninitialized variable
2016-03-11 15:38 ` Minfei Huang
@ 2016-03-14 10:58 ` Dan Carpenter
2016-03-14 11:25 ` Minfei Huang
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2016-03-14 10:58 UTC (permalink / raw)
To: Minfei Huang
Cc: walter harms, xlpang, Andrew Morton, Vivek Goyal, Dave Young,
Ingo Molnar, Toshi Kani, Mimi Zohar, linux-kernel,
kernel-janitors
On Fri, Mar 11, 2016 at 11:38:19PM +0800, Minfei Huang wrote:
> I think we can modify the logic a bit to make code simple. Thus gcc will
> not complain about any more, and the logic is earier.
This is a Smatch warning, not a GCC warning. If you think the new code
is clearer, that's fine but don't just silence the warning to please
Smatch. I'm pretty sure I can silence this warning in Smatch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch] kexec: potetially using uninitialized variable
2016-03-14 10:58 ` Dan Carpenter
@ 2016-03-14 11:25 ` Minfei Huang
0 siblings, 0 replies; 7+ messages in thread
From: Minfei Huang @ 2016-03-14 11:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: walter harms, xlpang, Andrew Morton, Vivek Goyal, Dave Young,
Ingo Molnar, Toshi Kani, Mimi Zohar, linux-kernel,
kernel-janitors
On 03/14/16 at 01:58pm, Dan Carpenter wrote:
> On Fri, Mar 11, 2016 at 11:38:19PM +0800, Minfei Huang wrote:
> > I think we can modify the logic a bit to make code simple. Thus gcc will
> > not complain about any more, and the logic is earier.
>
> This is a Smatch warning, not a GCC warning. If you think the new code
> is clearer, that's fine but don't just silence the warning to please
> Smatch. I'm pretty sure I can silence this warning in Smatch.
>
> regards,
> dan carpenter
>
Hi, Dan.
If not a GCC warning, I'm fine to fix it in Smatch, since the code logic
is clear enough.
Thanks
Minfei
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-03-14 11:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-11 8:07 [patch] kexec: potetially using uninitialized variable Dan Carpenter
2016-03-11 8:52 ` Xunlei Pang
2016-03-11 9:19 ` Dan Carpenter
2016-03-11 9:47 ` walter harms
2016-03-11 15:38 ` Minfei Huang
2016-03-14 10:58 ` Dan Carpenter
2016-03-14 11:25 ` Minfei Huang
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).