| Message ID | 1406074006-15736-1-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > From: Stephen Warren <swarren@nvidia.com> > > When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves > the downloaded filename to global variable BootFile. If the boot > operation is aborted, this global state is not cleared. If "dhcp" is > executed later without any arguments, BootFile is not cleared, and when > the DHCP response is received, BootpCopyNetParams() writes the value into > environment variable bootfile. > > This causes the following scenario: > > * Boot script executes dhcp; pxe get; pxe boot > > * User CTRL-C's the PXE menu, which causes the first menu item to be > booted, which causes some file to be downloaded. > > (This boot-on-CTRL-C behaviour is arguably a bug too, but it's a > separate bug and the bug this patch fixes would still exist if the user > simply waited to press CTRL-C until "pxe boot" started downloading > files) > > * User CTRL-C's the file downloads, but the filename is still written to > the bootfile environment variable. > > * User re-runs the boot command, which in my case executes "dhcp; pxe get; > pxe boot" again, and "dhcp" picks up the saved bootfile environment > variable and proceeds to download a file that it shouldn't. > > To solve this, modify the implementation of "pxe get" to clear BootFile > if the whole boot operation fails, which avoids this whole mess. > > An alternative would be to modify netboot_common() such that the no- > arguments case explicitly clears the global variable BootFile. However, > that would prevent the following command sequences from working: > > $ dhcp filename # downloads "filename" > $ dhcp # downloads $bootfile, i.e. "filename" > > or: > $ setenv bootfile filename > $ dhcp # downloads $bootfile, i.e. "filename" > > ... and I assume someone relies on U-Boot working that way. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
On 07/23/2014 03:37 PM, Joe Hershberger wrote: > > On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren <swarren@wwwdotorg.org > <mailto:swarren@wwwdotorg.org>> wrote: >> >> From: Stephen Warren <swarren@nvidia.com <mailto:swarren@nvidia.com>> >> >> When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves >> the downloaded filename to global variable BootFile. If the boot >> operation is aborted, this global state is not cleared. If "dhcp" is >> executed later without any arguments, BootFile is not cleared, and when >> the DHCP response is received, BootpCopyNetParams() writes the value into >> environment variable bootfile. ... > Acked-by: Joe Hershberger <joe.hershberger@ni.com > <mailto:joe.hershberger@ni.com>> Thanks. I'm not sure if you ack'd this simply so you'd remember you'd reviewed it, or because you expect someone else to apply the change? If the latter, I should forward the patch to them so they know about it:-)
On Wed, Jul 23, 2014 at 4:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 07/23/2014 03:37 PM, Joe Hershberger wrote: > > > > On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren <swarren@wwwdotorg.org > > <mailto:swarren@wwwdotorg.org>> wrote: > >> > >> From: Stephen Warren <swarren@nvidia.com <mailto:swarren@nvidia.com>> > >> > >> When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves > >> the downloaded filename to global variable BootFile. If the boot > >> operation is aborted, this global state is not cleared. If "dhcp" is > >> executed later without any arguments, BootFile is not cleared, and when > >> the DHCP response is received, BootpCopyNetParams() writes the value into > >> environment variable bootfile. > ... > > Acked-by: Joe Hershberger <joe.hershberger@ni.com > > <mailto:joe.hershberger@ni.com>> > > Thanks. I'm not sure if you ack'd this simply so you'd remember you'd > reviewed it, or because you expect someone else to apply the change? If > the latter, I should forward the patch to them so they know about it:-) Partly for me to remember and partly because recently Tom has been picking these few net things up directly. Cheers -Joe
On 07/23/2014 03:56 PM, Joe Hershberger wrote: > > On Wed, Jul 23, 2014 at 4:41 PM, Stephen Warren <swarren@wwwdotorg.org > <mailto:swarren@wwwdotorg.org>> wrote: > > > > On 07/23/2014 03:37 PM, Joe Hershberger wrote: > > > > > > On Tue, Jul 22, 2014 at 7:06 PM, Stephen Warren > <swarren@wwwdotorg.org <mailto:swarren@wwwdotorg.org> > > > <mailto:swarren@wwwdotorg.org <mailto:swarren@wwwdotorg.org>>> wrote: > > >> > > >> From: Stephen Warren <swarren@nvidia.com > <mailto:swarren@nvidia.com> <mailto:swarren@nvidia.com > <mailto:swarren@nvidia.com>>> > > >> > > >> When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() > saves > > >> the downloaded filename to global variable BootFile. If the boot > > >> operation is aborted, this global state is not cleared. If "dhcp" is > > >> executed later without any arguments, BootFile is not cleared, and > when > > >> the DHCP response is received, BootpCopyNetParams() writes the > value into > > >> environment variable bootfile. > > ... > > > Acked-by: Joe Hershberger <joe.hershberger@ni.com > <mailto:joe.hershberger@ni.com> > > > <mailto:joe.hershberger@ni.com <mailto:joe.hershberger@ni.com>>> > > > > Thanks. I'm not sure if you ack'd this simply so you'd remember you'd > > reviewed it, or because you expect someone else to apply the change? If > > the latter, I should forward the patch to them so they know about it:-) > > Partly for me to remember and partly because recently Tom has been > picking these few net things up directly. Tom, are you planning on picking up this patch?
On Tue, Jul 22, 2014 at 06:06:46PM -0600, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > When "pxe boot" downloads the initrd/kernel/DTB, netboot_common() saves > the downloaded filename to global variable BootFile. If the boot > operation is aborted, this global state is not cleared. If "dhcp" is > executed later without any arguments, BootFile is not cleared, and when > the DHCP response is received, BootpCopyNetParams() writes the value into > environment variable bootfile. > > This causes the following scenario: > > * Boot script executes dhcp; pxe get; pxe boot > > * User CTRL-C's the PXE menu, which causes the first menu item to be > booted, which causes some file to be downloaded. > > (This boot-on-CTRL-C behaviour is arguably a bug too, but it's a > separate bug and the bug this patch fixes would still exist if the user > simply waited to press CTRL-C until "pxe boot" started downloading > files) > > * User CTRL-C's the file downloads, but the filename is still written to > the bootfile environment variable. > > * User re-runs the boot command, which in my case executes "dhcp; pxe get; > pxe boot" again, and "dhcp" picks up the saved bootfile environment > variable and proceeds to download a file that it shouldn't. > > To solve this, modify the implementation of "pxe get" to clear BootFile > if the whole boot operation fails, which avoids this whole mess. > > An alternative would be to modify netboot_common() such that the no- > arguments case explicitly clears the global variable BootFile. However, > that would prevent the following command sequences from working: > > $ dhcp filename # downloads "filename" > $ dhcp # downloads $bootfile, i.e. "filename" > > or: > $ setenv bootfile filename > $ dhcp # downloads $bootfile, i.e. "filename" > > ... and I assume someone relies on U-Boot working that way. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > Acked-by: Joe Hershberger <joe.hershberger@ni.com> Applied to u-boot/master, thanks!
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c index ba48692e8641..28999f573496 100644 --- a/common/cmd_pxe.c +++ b/common/cmd_pxe.c @@ -1554,6 +1554,8 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) destroy_pxe_menu(cfg); + copy_filename(BootFile, "", sizeof(BootFile)); + return 0; }