Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command
On 6/19/23 12:12, Xavier Drudis Ferran wrote:
It seems the email addresses are being constantly corrupted in each email. This time the ML address is wrong and missing an e at the end. There is some e@ nonexistent address which I have to keep removing.
When DISTRO_DEFAULTS is not set, the default environment has bootcmd=bootflow
That is not right, on $randomboard I picked the bootcmd is something else.
? and this will cause a UCLASS_BOOTDEV device to be
added as sibling of those UCLASS_BLK devices in boot_targets, until boot succeeds from some device. If none succeeds, and usb is in boot_targets, and an usb storage device is plugged to some usb port at boot time, its UCLASS_MASS_STORAGE device will have a UCLASS_BOOTDEV device as child, besides a UCLASS_BLK child.
If once the boot fails the user enters at the U-Boot shell prompt:
usb info
or
usb tree
The code in cmd/usb.c will eventually recurse into the UCLASS_BOOTDEV and pass a null pointer to usb_device (because it has no parent_priv_). This causes a reset.
Does this happen if you set empty bootcmd ('=> setenv bootcmd 'echo hello' for example), then 'saveenv' , then 'reset' , then drop into U-Boot shell and run 'usb reset ; usb info' too ?
[...]
El Mon, Jun 19, 2023 at 11:49:18PM +0200, Marek Vasut deia:
On 6/19/23 12:12, Xavier Drudis Ferran wrote:
It seems the email addresses are being constantly corrupted in each email. This time the ML address is wrong and missing an e at the end. There is some e@ nonexistent address which I have to keep removing.
Yes, that's my fault. I'm sorry. I apologize to you and others. I resent my mail with the proper address. Please just look for my mail with the wrong address and delete it from your mail archive to prevent further such problems. You can reply to the other mail I sent (June 19th), because it has the same content, just with an added apology. Sorry again.
When DISTRO_DEFAULTS is not set, the default environment has bootcmd=bootflow
That is not right, on $randomboard I picked the bootcmd is something else.
And how default is your default environment for your $randomboard ?
Almost half the configs/* redefine CONFIG_BOOTCOMMAND (524/1268)
When DISTRO_DEFAULTS is not set, that makes BOOTSTD_BOOTCOMMAND default to y and the default for BOOTCMD is not "run distro_boootcmd", but "bootflow scan" or (for sandbox) "bootflow scan -lb". When there's bootcmd at all.
But this is only the default for the default environment. It can be overriden and the Kconfig is not exactly simple. An extract:
next branch:
arch/Arm/Kconfig:
config ARCH_ROCKCHIP [...] imply BOOTSTD_DEFAULTS [...]
cmd/Kconfig:
config CMD_BOOTFLOW bool "bootflow" depends on BOOTSTD default y [...]
config CMD_BOOTFLOW_FULL bool "bootflow - extract subcommands" depends on BOOTSTD_FULL default y [...]
boot/Kconfig:
config BOOT_DEFAULTS bool # Common defaults for standard boot and distroboot imply USE_BOOTCOMMAND [...]
config BOOTSTD bool "Standard boot support" default y [...]
config BOOTSTD_FULL bool "Enhanced features for standard boot" default y if SANDBOX [...]
if BOOTSTD [...]
config BOOTSTD_DEFAULTS bool "Select some common defaults for standard boot" depends on BOOTSTD imply USE_BOOTCOMMAND select BOOT_DEFAULTS select BOOTMETH_DISTRO [...]
config BOOTSTD_BOOTCOMMAND bool "Use bootstd to boot" default y if !DISTRO_DEFAULTS [...] [...] endif [...] config DISTRO_DEFAULTS bool "Select defaults suitable for booting general purpose Linux distributions" select BOOT_DEFAULTS [...]
config BOOTCOMMAND string "bootcmd value" depends on USE_BOOTCOMMAND && !USE_DEFAULT_ENV_FILE default "bootflow scan -lb" if BOOTSTD_DEFAULTS && CMD_BOOTFLOW_FULL default "bootflow scan" if BOOTSTD_DEFAULTS && !CMD_BOOTFLOW_FULL default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
Does this happen if you set empty bootcmd ('=> setenv bootcmd 'echo hello' for example), then 'saveenv' , then 'reset' , then drop into U-Boot shell and run 'usb reset ; usb info' too ?
I haven't tested it. If bootflow scan is not run it might not happen. Someone has to hang the UCLASS_BOOTDEV on the usb mass storage device, for it to fail. But as far as I know the idea is to make bootflow the default in more and more cases. You'll always be able to avoid it running in your board by setting your own environment at runtime or changing the configuration, yes, but what's the point ?
I thought that failing one scenario was enough to fix things. When one finds a bug it tries to help others to reproduce it. When others help the bug finder to run other scenarios that don't have the bug, what's that useful for ?
Note that it won't fail if the boot succeeds, because then you won't have a shell to run usb info/tree. It won't fail if usb is not in boot_targets. It won't fail if there's no mass storage device connected to usb when bootflow scan is run...
But I still think the failing case is worth fixing. Someone may be wondering why bootflow fails, run usb info and find a reset, when setting up a new board, or trying to boot from the wrong usb stick after the system partition has been corrupted, or whatever. It's not something that breaks any board in production, but it's not something to leave forever broken. In theory a null pointer dereference might be used by some attacker, but in this case I don't really see any useful attack, maybe it's my lack of imagination. So I'm not claiming it's a severe bug. It's just a normal bug that needs fixing when possible.
Or are you trying to hint that the solution shouldn't be changing cmd/usb.c but cleaning up the UCLASS_BOOTDEVs after bootflow scan somehow ?
Or I should change the commit message because the point is not so much what's the default environment or the default default environment, but simply that bootflow scan is run with an usb mass storage device connected and no boot content present in any of the boot_targets media, and then usb tree/info is run ?
If it's just that you can't reproduce it, can you try to ?:
- set up a board with no boot media (I tested like this but it might not be needed),
- put usb in boot_targets (if you put only usb there you may not need the previous step): setenv boot_targets usb
- plug a non-booting usb mass storage device to an usb port,
- run usb reset in case you already had usb initialized at boot, or skip this if usb is not initialized yet. If in doubt, run usb reset.
- run bootflow scan
- run usb info
It should list some devices, but give you a reset just after listing the usb mass storage device without my patch, and it should just list all usb devices and go back to the prompt with my patch.
On 6/20/23 11:17, Xavier Drudis Ferran wrote:
El Mon, Jun 19, 2023 at 11:49:18PM +0200, Marek Vasut deia:
On 6/19/23 12:12, Xavier Drudis Ferran wrote:
It seems the email addresses are being constantly corrupted in each email. This time the ML address is wrong and missing an e at the end. There is some e@ nonexistent address which I have to keep removing.
Yes, that's my fault. I'm sorry. I apologize to you and others. I resent my mail with the proper address. Please just look for my mail with the wrong address and delete it from your mail archive to prevent further such problems. You can reply to the other mail I sent (June 19th), because it has the same content, just with an added apology. Sorry again.
That's fine
When DISTRO_DEFAULTS is not set, the default environment has bootcmd=bootflow
That is not right, on $randomboard I picked the bootcmd is something else.
And how default is your default environment for your $randomboard ?
Default, see: $ git grep CONFIG_BOOTCOMMAND configs/
Almost half the configs/* redefine CONFIG_BOOTCOMMAND (524/1268)
When DISTRO_DEFAULTS is not set, that makes BOOTSTD_BOOTCOMMAND default to y and the default for BOOTCMD is not "run distro_boootcmd", but "bootflow scan" or (for sandbox) "bootflow scan -lb". When there's bootcmd at all.
But this is only the default for the default environment. It can be overriden and the Kconfig is not exactly simple. An extract:
next branch:
arch/Arm/Kconfig:
config ARCH_ROCKCHIP [...] imply BOOTSTD_DEFAULTS [...]
cmd/Kconfig:
config CMD_BOOTFLOW bool "bootflow" depends on BOOTSTD default y [...]
config CMD_BOOTFLOW_FULL bool "bootflow - extract subcommands" depends on BOOTSTD_FULL default y [...]
boot/Kconfig:
config BOOT_DEFAULTS bool # Common defaults for standard boot and distroboot imply USE_BOOTCOMMAND [...]
config BOOTSTD bool "Standard boot support" default y [...]
config BOOTSTD_FULL bool "Enhanced features for standard boot" default y if SANDBOX [...]
if BOOTSTD [...]
config BOOTSTD_DEFAULTS bool "Select some common defaults for standard boot" depends on BOOTSTD imply USE_BOOTCOMMAND select BOOT_DEFAULTS select BOOTMETH_DISTRO [...]
config BOOTSTD_BOOTCOMMAND bool "Use bootstd to boot" default y if !DISTRO_DEFAULTS [...] [...] endif [...] config DISTRO_DEFAULTS bool "Select defaults suitable for booting general purpose Linux distributions" select BOOT_DEFAULTS [...]
config BOOTCOMMAND string "bootcmd value" depends on USE_BOOTCOMMAND && !USE_DEFAULT_ENV_FILE default "bootflow scan -lb" if BOOTSTD_DEFAULTS && CMD_BOOTFLOW_FULL default "bootflow scan" if BOOTSTD_DEFAULTS && !CMD_BOOTFLOW_FULL default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
Does this happen if you set empty bootcmd ('=> setenv bootcmd 'echo hello' for example), then 'saveenv' , then 'reset' , then drop into U-Boot shell and run 'usb reset ; usb info' too ?
I haven't tested it. If bootflow scan is not run it might not happen.
So, what is the minimal test case ? I have been asking for that for a while.
Someone has to hang the UCLASS_BOOTDEV on the usb mass storage device, for it to fail. But as far as I know the idea is to make bootflow the default in more and more cases. You'll always be able to avoid it running in your board by setting your own environment at runtime or changing the configuration, yes, but what's the point ?
I thought that failing one scenario was enough to fix things. When one finds a bug it tries to help others to reproduce it. When others help the bug finder to run other scenarios that don't have the bug, what's that useful for ?
Note that it won't fail if the boot succeeds, because then you won't have a shell to run usb info/tree. It won't fail if usb is not in boot_targets. It won't fail if there's no mass storage device connected to usb when bootflow scan is run...
But I still think the failing case is worth fixing. Someone may be wondering why bootflow fails, run usb info and find a reset, when setting up a new board, or trying to boot from the wrong usb stick after the system partition has been corrupted, or whatever. It's not something that breaks any board in production, but it's not something to leave forever broken. In theory a null pointer dereference might be used by some attacker, but in this case I don't really see any useful attack, maybe it's my lack of imagination. So I'm not claiming it's a severe bug. It's just a normal bug that needs fixing when possible.
Or are you trying to hint that the solution shouldn't be changing cmd/usb.c but cleaning up the UCLASS_BOOTDEVs after bootflow scan somehow ?
I would really like a minimal test case. Empty your env and figure out the commands which need to be executed to trigger this. Without any interference from other commands/scripting/...
Or I should change the commit message because the point is not so much what's the default environment or the default default environment, but simply that bootflow scan is run with an usb mass storage device connected and no boot content present in any of the boot_targets media, and then usb tree/info is run ?
If it's just that you can't reproduce it, can you try to ?:
set up a board with no boot media (I tested like this but it might not be needed),
put usb in boot_targets (if you put only usb there you may not need the previous step): setenv boot_targets usb
Here you assume distro bootcommand or some such . Can we remove that assumption ? (I think yes, and we should)
plug a non-booting usb mass storage device to an usb port,
run usb reset in case you already had usb initialized at boot, or skip this if usb is not initialized yet. If in doubt, run usb reset.
run bootflow scan
run usb info
It should list some devices, but give you a reset just after listing the usb mass storage device without my patch, and it should just list all usb devices and go back to the prompt with my patch.
Does it crash if you empty your env and run simply
=> usb reset ; bootflow scan ; usb info
?
El Tue, Jun 20, 2023 at 11:49:36AM +0200, Marek Vasut deia:
Default, see: $ git grep CONFIG_BOOTCOMMAND configs/
I'm lost.
I called default what Kconfig used as default. You seem to call default what's in board specific config files. Whatever. Fix the wording in the commit message if you like.
So, what is the minimal test case ? I have been asking for that for a while.
I sent a minimal test case last week.
https://lists.denx.de/pipermail/u-boot/2023-June/520109.html
You build for a Rock Pi 4 board, boot with usb stick and no boot media and run usb info and you get a reset.
I won't send it again because I can't guess what you consider minimal.
I would really like a minimal test case. Empty your env and figure out the commands which need to be executed to trigger this. Without any interference from other commands/scripting/...
I'm sorry but if what I sent isn't enough I don't think I'll have time to help you any further. Find your minimal test case yourself or ignore my patch.
If it's just that you can't reproduce it, can you try to ?:
set up a board with no boot media (I tested like this but it might not be needed),
put usb in boot_targets (if you put only usb there you may not need the previous step): setenv boot_targets usb
Here you assume distro bootcommand or some such . Can we remove that assumption ? (I think yes, and we should)
I don't think I'm assuming anything about bootcommand. That's precisely why I wrote these steps instead of the "just boot a Rock Pi 4" scenario last week.
The commit message talks about bootcmd because it justifies that the bootflow scan will be called automatically in some cases, so the bug has more impact that it would otherwise have.
But the bug should appear whether or not you have bootcmd. The bug should be an interaction between what bootflow scan does and what usb info or usb tree do.
I'm assuming bootflow reads the boot_targets environment variable to know where it searches for boot devices, and therefore to which devices it will attach a UCLASS_BOOTDEV child to some devices, in particular to usb mass storage devices if any is present, that will later break usb info/usb tree. Whether bootflow is called from bootcmd or not should be irrelevant.
If you follow the code from the bootflow command you may find yourself that the boot_targets variable is involved. I did it last week or sometime and won't do it again now for you, sorry. I know I may have misunderstood something, and I'm sorry for the noise if so.
plug a non-booting usb mass storage device to an usb port,
run usb reset in case you already had usb initialized at boot, or skip this if usb is not initialized yet. If in doubt, run usb reset.
run bootflow scan
run usb info
It should list some devices, but give you a reset just after listing the usb mass storage device without my patch, and it should just list all usb devices and go back to the prompt with my patch.
Does it crash if you empty your env and run simply
=> usb reset ; bootflow scan ; usb info
?
I guess it won't crash if environment var boot_targets is absent or empty. Or even if it's full but has no "usb" in it. But I'm not sure anymore at what time the variable is read, so it might be that emptying it when bootflow structures are already set up wouldn't change things, I don't know, I don't remember.
But I won't try it, sorry.
I found a bug, I sent a 4 line patch. Since I didn't justify it enough I sent followup mails on how to reproduce. This week I sent a second version, with redundant measures to seek consensus. It's a 6 line patch or 8 or whatever. I didn't write bootflow, and I didn't write cmd/usb.c
And I don't have time to keep writing long emails. I'm sorry. Not even to count how many lines of text I wrote compared to the 8 lines of code or whatever. If someone has the bureaucratic skills and patience to pursue this further, they can do what they want with my patch (under GPL2 as implied by the sign off). If not, you all can keep your bugs, I won't try anymore to steal them.
Bye and sorry for any disturbances.
On 6/20/23 12:43, Xavier Drudis Ferran wrote:
El Tue, Jun 20, 2023 at 11:49:36AM +0200, Marek Vasut deia:
Default, see: $ git grep CONFIG_BOOTCOMMAND configs/
I'm lost.
I called default what Kconfig used as default. You seem to call default what's in board specific config files. Whatever. Fix the wording in the commit message if you like.
All I am really asking for is minimal test case. That is all I was asking for since the very beginning.
So to answer my own question, this is the minimal test case:
=> usb reset ; bootflow scan ; usb info
Can you please add it to the commit message, collect RB from Simon, add
Reviewed-by: Marek Vasut marex@denx.de Tested-by: Marek Vasut marex@denx.de
And send V2 so I can pick it for this release ?
Thank you
participants (2)
-
Marek Vasut -
Xavier Drudis Ferran