Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
On Sun, Sep 22, 2013 at 10:21 PM, Steven Falco stevenfa...@gmail.com wrote: Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. In this case, there will be a null cmdtp pointer, and we must not dereference it. Signed-off-by: Steven A. Falco stevenfa...@gmail.com --- Acked-by: Otavio Salvador ota...@ossystems.com.br Tom, will you pick this or should it be Cced to someone specific? -- Otavio Salvador O.S. Systems http://www.ossystems.com.brhttp://code.ossystems.com.br Mobile: +55 (53) 9981-7854Mobile: +1 (347) 903-9750 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
On Mon, Sep 23, 2013 at 1:11 PM, Albert ARIBAUD albert.u.b...@aribaud.net wrote: Then shouldn't the patch subject/summary be print command name only if cmdtp is not NULL rather than the quite uninformative prevent a crash? Yes, I agree that original subject is a bit misleading. When I read it I thought it was a Wandboard related problem. Regards, Fabio Estevam ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
On Mon, Sep 23, 2013 at 01:21:34PM -0300, Fabio Estevam wrote: On Mon, Sep 23, 2013 at 1:11 PM, Albert ARIBAUD albert.u.b...@aribaud.net wrote: Then shouldn't the patch subject/summary be print command name only if cmdtp is not NULL rather than the quite uninformative prevent a crash? Yes, I agree that original subject is a bit misleading. I'm re-wording the commit message now, just got side-tracked fixing another problem I introduced on Friday :( -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Dear Fabio Estevam, In message caomzo5aj56kvtfrqbd3wq7oip8q3wa0yv4evuzerkxfufvx...@mail.gmail.com you wrote: Then shouldn't the patch subject/summary be print command name only if cmdtp is not NULL rather than the quite uninformative prevent a crash? Yes, I agree that original subject is a bit misleading. When I read it I thought it was a Wandboard related problem. I don't know if it's only Wandboard, or if other boards are affected, too (which are these? under which exact test cases?). In any case. the problem is not here, but on the caller's side. It should not call a function which expects a command name with a NULL pointer passed as argument. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The Buddha, the Godhead, resides quite as comfortably in the circuits of a digital computer or the gears of a cycle transmission as he does at the top of a mountain or in the petals of a flower. - R. Pirsig, Zen and the Art of Motorcycle Maintenance ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Hi Steven, On Sun, 22 Sep 2013 21:21:32 -0400, Steven Falco stevenfa...@gmail.com wrote: Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. In this case, there will be a null cmdtp pointer, and we must not dereference it. Signed-off-by: Steven A. Falco stevenfa...@gmail.com --- In file cmd_pxe.c around line 687 is a call: do_bootm(NULL, 0, bootm_argc, bootm_argv); Notice that the first argument is NULL. Therefore, the cmdtp pointer will always be NULL when using the pxe boot mechanism. do_bootm() eventually calls boot_get_kernel(), still with cmdtp == NULL. In the Wandboard case, the vmlinuz binary is not legacy format, nor is it fit format, so U-Boot attempts to print: printf(Wrong Image Format for %s command\n, cmdtp-name); That is doomed to fail, because cmdtp is NULL. The following patch corrects the problem; the command name will be printed only if the pointer is valid. Then shouldn't the patch subject/summary be print command name only if cmdtp is not NULL rather than the quite uninformative prevent a crash? Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Dear Otavio Salvador, In message cap9odkrdjdph+8mrxof+zojzl3qs1u9k5kadey896foo_cl...@mail.gmail.com you wrote: On Sun, Sep 22, 2013 at 10:21 PM, Steven Falco stevenfa...@gmail.com wrote: Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. In this case, there will be a null cmdtp pointer, and we must not dereference it. Signed-off-by: Steven A. Falco stevenfa...@gmail.com --- Acked-by: Otavio Salvador ota...@ossystems.com.br Tom, will you pick this or should it be Cced to someone specific? Please don't. This should be fixed at the root of the problem instead. And in no case a bogus error message should be printed. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Horses just naturally have mohawk haircuts. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
On Mon, Sep 23, 2013 at 08:50:55PM +0200, Wolfgang Denk wrote: Dear Fabio Estevam, In message caomzo5aj56kvtfrqbd3wq7oip8q3wa0yv4evuzerkxfufvx...@mail.gmail.com you wrote: Then shouldn't the patch subject/summary be print command name only if cmdtp is not NULL rather than the quite uninformative prevent a crash? Yes, I agree that original subject is a bit misleading. When I read it I thought it was a Wandboard related problem. I don't know if it's only Wandboard, or if other boards are affected, too (which are these? under which exact test cases?). In any case. the problem is not here, but on the caller's side. It should not call a function which expects a command name with a NULL pointer passed as argument. I looked around at this a bit this morning. cmd_pxe.c would need a lot of mangling to pass around cmdtp, just for the sake of an error message that's then ignored as the caller logic is: 1) Try bootm on the image 2) If CONFIG_CMD_BOOTZ, if bootm returned, maybe we got a zImage? do_bootz instead (also NULL cmdtp). The error message wouldn't exactly make sense here either, being invoked via menu. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Dear Steven Falco, In message 523f979c.1070...@gmail.com you wrote: Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. In this case, there will be a null cmdtp pointer, and we must not dereference it. ... - printf(Wrong Image Format for %s command\n, cmdtp-name); + if (cmdtp) + printf(Wrong Image Format for %s command\n, cmdtp-name); + else + printf(Wrong Image Format for command\n); This is the wrong way to fix it. Instead of handling this here, please fix the place where a NULL pointer is passed incorrectly. Also, the error message Wrong Image Format for command makes no sense and gives no help to the user to understand what's wrong. NAK. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Never worry about theory as long as the machinery does what it's supposed to do. - R. A. Heinlein ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
On Mon, Sep 23, 2013 at 03:17:57PM -0400, Tom Rini wrote: On Mon, Sep 23, 2013 at 08:50:55PM +0200, Wolfgang Denk wrote: Dear Fabio Estevam, In message caomzo5aj56kvtfrqbd3wq7oip8q3wa0yv4evuzerkxfufvx...@mail.gmail.com you wrote: Then shouldn't the patch subject/summary be print command name only if cmdtp is not NULL rather than the quite uninformative prevent a crash? Yes, I agree that original subject is a bit misleading. When I read it I thought it was a Wandboard related problem. I don't know if it's only Wandboard, or if other boards are affected, too (which are these? under which exact test cases?). In any case. the problem is not here, but on the caller's side. It should not call a function which expects a command name with a NULL pointer passed as argument. I looked around at this a bit this morning. cmd_pxe.c would need a lot of mangling to pass around cmdtp, just for the sake of an error message that's then ignored as the caller logic is: 1) Try bootm on the image 2) If CONFIG_CMD_BOOTZ, if bootm returned, maybe we got a zImage? do_bootz instead (also NULL cmdtp). The error message wouldn't exactly make sense here either, being invoked via menu. So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get an error message out, and it's not too bad looking, but it highlights another problem, which is that we could really use a way to get at least the is this a ... ? code, and just get the error code, rather than printouts. The pxe (and really it's the syslinux.conf/extlinux.conf parsing) code shouldn't be doing bootm();bootz() but checking the image type and calling the right boot. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Dear Tom, In message 20130923191757.GZ5273@bill-the-cat you wrote: I don't know if it's only Wandboard, or if other boards are affected, too (which are these? under which exact test cases?). In any case. the problem is not here, but on the caller's side. It should not call a function which expects a command name with a NULL pointer passed as argument. I looked around at this a bit this morning. cmd_pxe.c would need a lot of mangling to pass around cmdtp, just for the sake of an error message that's then ignored as the caller logic is: 1) Try bootm on the image 2) If CONFIG_CMD_BOOTZ, if bootm returned, maybe we got a zImage? do_bootz instead (also NULL cmdtp). The error message wouldn't exactly make sense here either, being invoked via menu. To me that meens that the whole logic needs rework. We should never just try out if an image is in uImage format or a zImage - we are perfectly able to detect such information from the file header (in case of uImage at least). If we keep the code as is, what's wrong then with using the pxe command as ID string? Then the end user knows at least that it was this command which was failing (or producing the message). Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Plan to throw one away. You will anyway. - Fred Brooks, The Mythical Man Month ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Dear Tom, In message 20130923194554.GA5273@bill-the-cat you wrote: So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get an error message out, and it's not too bad looking, but it highlights another problem, which is that we could really use a way to get at least the is this a ... ? code, and just get the error code, rather than printouts. The pxe (and really it's the syslinux.conf/extlinux.conf parsing) code shouldn't be doing bootm();bootz() but checking the image type and calling the right boot. Ah, full ACK (I should have read the thread to end before posting). Thanks! Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Here's a fish hangs in the net like a poor man's right in the law. 'Twill hardly come out. - Shakespeare, Pericles, Act II, Scene 1 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
On 09/23/2013 04:44 PM, Wolfgang Denk wrote: Dear Tom, In message 20130923194554.GA5273@bill-the-cat you wrote: So, I went off an modified cmd_pxe.c to pass around cmdtp so that we get an error message out, and it's not too bad looking, but it highlights another problem, which is that we could really use a way to get at least the is this a ... ? code, and just get the error code, rather than printouts. The pxe (and really it's the syslinux.conf/extlinux.conf parsing) code shouldn't be doing bootm();bootz() but checking the image type and calling the right boot. Ah, full ACK (I should have read the thread to end before posting). Thanks! Best regards, Wolfgang Denk I understand your point that it is better to fix the problem at the source. I also like Tom's suggestion that the type be checked earlier, and then just directly choose the right bootX() variant. So naturally, I withdraw my patch, in favor of your fix - at least I was able to isolate the source of the crash for you. :-) Steve ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Prevent a U-Boot crash on Wandboard
Prevent a crash when PXE boot calls do_bootm with a vmlinuz formatted image. In this case, there will be a null cmdtp pointer, and we must not dereference it. Signed-off-by: Steven A. Falco stevenfa...@gmail.com --- In file cmd_pxe.c around line 687 is a call: do_bootm(NULL, 0, bootm_argc, bootm_argv); Notice that the first argument is NULL. Therefore, the cmdtp pointer will always be NULL when using the pxe boot mechanism. do_bootm() eventually calls boot_get_kernel(), still with cmdtp == NULL. In the Wandboard case, the vmlinuz binary is not legacy format, nor is it fit format, so U-Boot attempts to print: printf(Wrong Image Format for %s command\n, cmdtp-name); That is doomed to fail, because cmdtp is NULL. The following patch corrects the problem; the command name will be printed only if the pointer is valid. diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 349f165..2249682 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -985,7 +985,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, break; #endif default: - printf(Wrong Image Format for %s command\n, cmdtp-name); + if (cmdtp) + printf(Wrong Image Format for %s command\n, cmdtp-name); + else + printf(Wrong Image Format for command\n); bootstage_error(BOOTSTAGE_ID_FIT_KERNEL_INFO); return NULL; } ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
