* [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).