* Re: [PATCH] UBI: Fix crash in try_recover_peb() [not found] <1476367536-24782-1-git-send-email-geert@linux-m68k.org> @ 2016-10-13 14:24 ` Boris Brezillon 2016-10-14 10:49 ` Boris Brezillon 2016-10-17 8:00 ` Geert Uytterhoeven 0 siblings, 2 replies; 6+ messages in thread From: Boris Brezillon @ 2016-10-13 14:24 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Richard Weinberger, Brian Norris, Artem Bityutskiy, David Woodhouse, linux-mtd, linux-kernel Hi Geert, On Thu, 13 Oct 2016 16:05:36 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: > drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function > > The pointer vid_hdr is indeed not initialized, leading to a crash when > it is dereferenced. > > Fix this by obtaining the pointer from the VID buffer, like is done > everywhere else. Indeed, I don't know how I missed that one :-/. > > Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> One minor comment below, otherwise Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > Completely untested. And I know nothing about UBI ;-) > --- > drivers/mtd/ubi/eba.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c > index 95c4048a371e87b6..388e46be6ad92805 100644 > --- a/drivers/mtd/ubi/eba.c > +++ b/drivers/mtd/ubi/eba.c > @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum, > goto out_put; > } > > + vid_hdr = ubi_get_vid_hdr(vidb); Can you move this assignment at variable declaration time? > ubi_assert(vid_hdr->vol_type == UBI_VID_DYNAMIC); > > mutex_lock(&ubi->buf_mutex); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: Fix crash in try_recover_peb() 2016-10-13 14:24 ` [PATCH] UBI: Fix crash in try_recover_peb() Boris Brezillon @ 2016-10-14 10:49 ` Boris Brezillon 2016-10-17 8:00 ` Geert Uytterhoeven 2016-10-17 8:00 ` Geert Uytterhoeven 1 sibling, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2016-10-14 10:49 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Richard Weinberger, Brian Norris, Artem Bityutskiy, David Woodhouse, linux-mtd, linux-kernel On Thu, 13 Oct 2016 16:24:58 +0200 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Geert, > > On Thu, 13 Oct 2016 16:05:36 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: > > drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function > > > > The pointer vid_hdr is indeed not initialized, leading to a crash when > > it is dereferenced. > > > > Fix this by obtaining the pointer from the VID buffer, like is done > > everywhere else. > > Indeed, I don't know how I missed that one :-/. That's really weird. I tried to recompile the kernel on my machine (before applying your patch), to see if the compiler was complaining, and it's not :-(. I also coded simple functions with goto statements, to check that the detection was working correctly, and it is. Maybe you have some ideas why my compiler is failing to detect the problem (I'm currently using the ubuntu cross-toolchain: 'arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.1) 5.4.0 20160609'). > > > > > Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept") > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > One minor comment below, otherwise > > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > --- > > Completely untested. And I know nothing about UBI ;-) > > --- > > drivers/mtd/ubi/eba.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c > > index 95c4048a371e87b6..388e46be6ad92805 100644 > > --- a/drivers/mtd/ubi/eba.c > > +++ b/drivers/mtd/ubi/eba.c > > @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum, > > goto out_put; > > } > > > > + vid_hdr = ubi_get_vid_hdr(vidb); > > Can you move this assignment at variable declaration time? > > > ubi_assert(vid_hdr->vol_type == UBI_VID_DYNAMIC); > > > > mutex_lock(&ubi->buf_mutex); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: Fix crash in try_recover_peb() 2016-10-14 10:49 ` Boris Brezillon @ 2016-10-17 8:00 ` Geert Uytterhoeven 0 siblings, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2016-10-17 8:00 UTC (permalink / raw) To: Boris Brezillon Cc: Richard Weinberger, Brian Norris, Artem Bityutskiy, David Woodhouse, MTD Maling List, linux-kernel@vger.kernel.org Hi Boris, On Fri, Oct 14, 2016 at 12:49 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 13 Oct 2016 16:24:58 +0200 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: >> On Thu, 13 Oct 2016 16:05:36 +0200 >> Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> >> > drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: >> > drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function >> > >> > The pointer vid_hdr is indeed not initialized, leading to a crash when >> > it is dereferenced. >> > >> > Fix this by obtaining the pointer from the VID buffer, like is done >> > everywhere else. >> >> Indeed, I don't know how I missed that one :-/. > > That's really weird. I tried to recompile the kernel on my machine > (before applying your patch), to see if the compiler was complaining, > and it's not :-(. > > I also coded simple functions with goto statements, to check that the > detection was working correctly, and it is. > > Maybe you have some ideas why my compiler is failing to detect the > problem (I'm currently using the ubuntu cross-toolchain: > 'arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.1) 5.4.0 > 20160609'). It depends highly on the toolchain. My good old m68k-linux-gnu-gcc version 4.1.2 20061115 (prerelease) (Ubuntu 4.1.1-21) is known to generate plenty of warnings like these. Due to the high percentage of false positives, this was disabled in later versions of gcc, but (partially) re-enabled later (usually Arnd catches most of these while stll in -next. I'm afraid he's suffering from travelling to Linaro Connect ;-) Still, some of them are real bugs, so that's when my patch habits kick in... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: Fix crash in try_recover_peb() 2016-10-13 14:24 ` [PATCH] UBI: Fix crash in try_recover_peb() Boris Brezillon 2016-10-14 10:49 ` Boris Brezillon @ 2016-10-17 8:00 ` Geert Uytterhoeven 2016-10-17 11:02 ` Boris Brezillon 1 sibling, 1 reply; 6+ messages in thread From: Geert Uytterhoeven @ 2016-10-17 8:00 UTC (permalink / raw) To: Boris Brezillon Cc: Richard Weinberger, Brian Norris, Artem Bityutskiy, David Woodhouse, MTD Maling List, linux-kernel@vger.kernel.org Hi Boris, On Thu, Oct 13, 2016 at 4:24 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Thu, 13 Oct 2016 16:05:36 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >> drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: >> drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function >> >> The pointer vid_hdr is indeed not initialized, leading to a crash when >> it is dereferenced. >> >> Fix this by obtaining the pointer from the VID buffer, like is done >> everywhere else. > > Indeed, I don't know how I missed that one :-/. > >> >> Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept") >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > One minor comment below, otherwise > > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> Thanks! >> --- >> Completely untested. And I know nothing about UBI ;-) >> --- >> drivers/mtd/ubi/eba.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c >> index 95c4048a371e87b6..388e46be6ad92805 100644 >> --- a/drivers/mtd/ubi/eba.c >> +++ b/drivers/mtd/ubi/eba.c >> @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum, >> goto out_put; >> } >> >> + vid_hdr = ubi_get_vid_hdr(vidb); > > Can you move this assignment at variable declaration time? I can do that. However, that moves the call to ubi_get_vid_hdr() _before_ the call to ubi_io_read_vid_hdr(). While that would still work (ubi_get_vid_hdr() would just return a pointer to the not-yet-read data), I think it's better to not move it, to prevent people from accidentally trying to use it before the data has been read. Do you agree? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: Fix crash in try_recover_peb() 2016-10-17 8:00 ` Geert Uytterhoeven @ 2016-10-17 11:02 ` Boris Brezillon 2016-10-17 11:10 ` Geert Uytterhoeven 0 siblings, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2016-10-17 11:02 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Richard Weinberger, Brian Norris, Artem Bityutskiy, David Woodhouse, MTD Maling List, linux-kernel@vger.kernel.org On Mon, 17 Oct 2016 10:00:25 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Boris, > > On Thu, Oct 13, 2016 at 4:24 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Thu, 13 Oct 2016 16:05:36 +0200 > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > >> drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: > >> drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function > >> > >> The pointer vid_hdr is indeed not initialized, leading to a crash when > >> it is dereferenced. > >> > >> Fix this by obtaining the pointer from the VID buffer, like is done > >> everywhere else. > > > > Indeed, I don't know how I missed that one :-/. > > > >> > >> Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept") > >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > One minor comment below, otherwise > > > > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Thanks! > > >> --- > >> Completely untested. And I know nothing about UBI ;-) > >> --- > >> drivers/mtd/ubi/eba.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c > >> index 95c4048a371e87b6..388e46be6ad92805 100644 > >> --- a/drivers/mtd/ubi/eba.c > >> +++ b/drivers/mtd/ubi/eba.c > >> @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum, > >> goto out_put; > >> } > >> > >> + vid_hdr = ubi_get_vid_hdr(vidb); > > > > Can you move this assignment at variable declaration time? > > I can do that. However, that moves the call to ubi_get_vid_hdr() _before_ > the call to ubi_io_read_vid_hdr(). > > While that would still work (ubi_get_vid_hdr() would just return a pointer to > the not-yet-read data), I think it's better to not move it, to prevent people > from accidentally trying to use it before the data has been read. Well, maybe. I just thought it would be safer to assign it as early as possible to avoid another 'uninitialized var' bug if move code around again (which will happen soon ;-)). Anyway, it's not that important, I'll let Richard take the version he prefers. Thanks again for the fix. Boris ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: Fix crash in try_recover_peb() 2016-10-17 11:02 ` Boris Brezillon @ 2016-10-17 11:10 ` Geert Uytterhoeven 0 siblings, 0 replies; 6+ messages in thread From: Geert Uytterhoeven @ 2016-10-17 11:10 UTC (permalink / raw) To: Boris Brezillon Cc: Richard Weinberger, Brian Norris, Artem Bityutskiy, David Woodhouse, MTD Maling List, linux-kernel@vger.kernel.org Hi Boris, On Mon, Oct 17, 2016 at 1:02 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 17 Oct 2016 10:00:25 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Thu, Oct 13, 2016 at 4:24 PM, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > On Thu, 13 Oct 2016 16:05:36 +0200 >> > Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> > >> >> drivers/mtd/ubi/eba.c: In function ‘try_recover_peb’: >> >> drivers/mtd/ubi/eba.c:744: warning: ‘vid_hdr’ is used uninitialized in this function >> >> >> >> The pointer vid_hdr is indeed not initialized, leading to a crash when >> >> it is dereferenced. >> >> >> >> Fix this by obtaining the pointer from the VID buffer, like is done >> >> everywhere else. >> > >> > Indeed, I don't know how I missed that one :-/. >> > >> >> >> >> Fixes: 3291b52f9ff0acc8 ("UBI: introduce the VID buffer concept") >> >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >> > >> > One minor comment below, otherwise >> > >> > Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com> >> >> Thanks! >> >> >> --- >> >> Completely untested. And I know nothing about UBI ;-) >> >> --- >> >> drivers/mtd/ubi/eba.c | 1 + >> >> 1 file changed, 1 insertion(+) >> >> >> >> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c >> >> index 95c4048a371e87b6..388e46be6ad92805 100644 >> >> --- a/drivers/mtd/ubi/eba.c >> >> +++ b/drivers/mtd/ubi/eba.c >> >> @@ -741,6 +741,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum, >> >> goto out_put; >> >> } >> >> >> >> + vid_hdr = ubi_get_vid_hdr(vidb); >> > >> > Can you move this assignment at variable declaration time? >> >> I can do that. However, that moves the call to ubi_get_vid_hdr() _before_ >> the call to ubi_io_read_vid_hdr(). >> >> While that would still work (ubi_get_vid_hdr() would just return a pointer to >> the not-yet-read data), I think it's better to not move it, to prevent people >> from accidentally trying to use it before the data has been read. > > Well, maybe. I just thought it would be safer to assign it as early as > possible to avoid another 'uninitialized var' bug if move code around > again (which will happen soon ;-)). I prefer seeing the warning again (I _will_ notice if it pops up again!) over failing at runtime because the data structure it points to hasn't been initialized yet, which the compiler can't warn me about... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-17 11:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1476367536-24782-1-git-send-email-geert@linux-m68k.org>
2016-10-13 14:24 ` [PATCH] UBI: Fix crash in try_recover_peb() Boris Brezillon
2016-10-14 10:49 ` Boris Brezillon
2016-10-17 8:00 ` Geert Uytterhoeven
2016-10-17 8:00 ` Geert Uytterhoeven
2016-10-17 11:02 ` Boris Brezillon
2016-10-17 11:10 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox