| Message ID | 20171020121614.9863-3-maxime.ripard@free-electrons.com |
|---|---|
| State | Accepted |
| Commit | 819f1e081c527d2d02cdaeec0027384688cf5de0 |
| Headers | show |
| Series | sunxi: Fix boot of Cubietruk and al. | expand |
Maxime Ripard <maxime.ripard@free-electrons.com> writes: > The U-Boot binary may trip over its actual allocated size in the storage. > In such a case, the environment will not be readable anymore (because > corrupted when the new image was flashed), and any attempt at using saveenv > to reconstruct the environment will result in a corrupted U-Boot binary. > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi > index 5adfd9bca2ec..72e95afd780e 100644 > --- a/arch/arm/dts/sunxi-u-boot.dtsi > +++ b/arch/arm/dts/sunxi-u-boot.dtsi > @@ -1,5 +1,14 @@ > #include <config.h> > > +/* > + * This is the maximum size the U-Boot binary can be, which is basically > + * the start of the environment, minus the start of the U-Boot binary in > + * the MMC. This makes the assumption that the MMC is using 512-bytes > + * blocks, but devices using something other than that remains to be > + * seen. > + */ > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) > + > / { > binman { > filename = "u-boot-sunxi-with-spl.bin"; > @@ -8,6 +17,9 @@ > filename = "spl/sunxi-spl.bin"; > }; > u-boot-img { > +#ifdef CONFIG_MMC > + size = <UBOOT_MMC_MAX_SIZE>; > +#endif > pos = <CONFIG_SPL_PAD_TO>; > }; > }; > -- This is broken in (at least) two ways: 1. It is simply nonsensical if u-boot and env are on different devices or not on mmc even if mmc support is enabled. 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. At best, this is useless. If the env doesn't immediately follow u-boot, it really breaks things. Please fix or revert, I don't really care which.
On Tue, 01 May 2018 18:25:06 +0100 Måns Rullgård <mans@mansr.com> wrote: > Maxime Ripard <maxime.ripard@free-electrons.com> writes: > > > The U-Boot binary may trip over its actual allocated size in the storage. > > In such a case, the environment will not be readable anymore (because > > corrupted when the new image was flashed), and any attempt at using saveenv > > to reconstruct the environment will result in a corrupted U-Boot binary. > > > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi > > index 5adfd9bca2ec..72e95afd780e 100644 > > --- a/arch/arm/dts/sunxi-u-boot.dtsi > > +++ b/arch/arm/dts/sunxi-u-boot.dtsi > > @@ -1,5 +1,14 @@ > > #include <config.h> > > > > +/* > > + * This is the maximum size the U-Boot binary can be, which is basically > > + * the start of the environment, minus the start of the U-Boot binary in > > + * the MMC. This makes the assumption that the MMC is using 512-bytes > > + * blocks, but devices using something other than that remains to be > > + * seen. > > + */ > > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) > > + > > / { > > binman { > > filename = "u-boot-sunxi-with-spl.bin"; > > @@ -8,6 +17,9 @@ > > filename = "spl/sunxi-spl.bin"; > > }; > > u-boot-img { > > +#ifdef CONFIG_MMC > > + size = <UBOOT_MMC_MAX_SIZE>; > > +#endif > > pos = <CONFIG_SPL_PAD_TO>; > > }; > > }; > > -- > > This is broken in (at least) two ways: > > 1. It is simply nonsensical if u-boot and env are on different devices > or not on mmc even if mmc support is enabled. > > 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. > At best, this is useless. If the env doesn't immediately follow > u-boot, it really breaks things. > > Please fix or revert, I don't really care which. The padding is not useless. It reserves space for future size expansions and makes it harder for the users to hurt themselves. For example, if the padded U-Boot size is 1024K while the actual size is only ~800K, then adventurous users might be tempted to fit some of their data into this gap. Yay, ~200K of storage space for free! Except that the next U-Boot release may grow up to 900K without any warning and if the users are not careful enough, then their data would be corrupted during upgrade. Could you please tell us what is your problem and why reverting this patch would fix it?
Siarhei Siamashka <siarhei.siamashka@gmail.com> writes: > On Tue, 01 May 2018 18:25:06 +0100 > Måns Rullgård <mans@mansr.com> wrote: > >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: >> >> > The U-Boot binary may trip over its actual allocated size in the storage. >> > In such a case, the environment will not be readable anymore (because >> > corrupted when the new image was flashed), and any attempt at using saveenv >> > to reconstruct the environment will result in a corrupted U-Boot binary. >> > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> > --- >> > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ >> > 1 file changed, 12 insertions(+) >> > >> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi >> > index 5adfd9bca2ec..72e95afd780e 100644 >> > --- a/arch/arm/dts/sunxi-u-boot.dtsi >> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi >> > @@ -1,5 +1,14 @@ >> > #include <config.h> >> > >> > +/* >> > + * This is the maximum size the U-Boot binary can be, which is basically >> > + * the start of the environment, minus the start of the U-Boot binary in >> > + * the MMC. This makes the assumption that the MMC is using 512-bytes >> > + * blocks, but devices using something other than that remains to be >> > + * seen. >> > + */ >> > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) >> > + >> > / { >> > binman { >> > filename = "u-boot-sunxi-with-spl.bin"; >> > @@ -8,6 +17,9 @@ >> > filename = "spl/sunxi-spl.bin"; >> > }; >> > u-boot-img { >> > +#ifdef CONFIG_MMC >> > + size = <UBOOT_MMC_MAX_SIZE>; >> > +#endif >> > pos = <CONFIG_SPL_PAD_TO>; >> > }; >> > }; >> > -- >> >> This is broken in (at least) two ways: >> >> 1. It is simply nonsensical if u-boot and env are on different devices >> or not on mmc even if mmc support is enabled. >> >> 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. >> At best, this is useless. If the env doesn't immediately follow >> u-boot, it really breaks things. >> >> Please fix or revert, I don't really care which. > > The padding is not useless. It reserves space for future size expansions > and makes it harder for the users to hurt themselves. > > For example, if the padded U-Boot size is 1024K while the actual size > is only ~800K, then adventurous users might be tempted to fit some of > their data into this gap. Yay, ~200K of storage space for free! Except > that the next U-Boot release may grow up to 900K without any warning > and if the users are not careful enough, then their data would be > corrupted during upgrade. Do U-Boot users really need that level of hand-holding? > Could you please tell us what is your problem and why reverting this > patch would fix it? See above. Best case, it just wastes space in the created file. If the configuration is anything other than U-Boot immediately followed by env on the same device, it _will_ break things horribly. A few examples: 1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env in SPI NOR flash. In this case, padding the file to the env offset makes no sense. Writing the image will corrupt anything stored after U-Boot at a smaller (but still safe) offset. 2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default value, is smaller than the offset of U-Boot in its actual device, the padding will be too large. Writing the image file will then corrupt a stored env. 3. U-Boot at start of device, env at end as indicated by a negative CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the image to (nearly) 2^64 bytes and promptly fills up your storage device. I keep running into variants of these, and I'm weary of it.
1;5201;0c On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: > Siarhei Siamashka <siarhei.siamashka@gmail.com> writes: > > > On Tue, 01 May 2018 18:25:06 +0100 > > Måns Rullgård <mans@mansr.com> wrote: > > > >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: > >> > >> > The U-Boot binary may trip over its actual allocated size in the storage. > >> > In such a case, the environment will not be readable anymore (because > >> > corrupted when the new image was flashed), and any attempt at using saveenv > >> > to reconstruct the environment will result in a corrupted U-Boot binary. > >> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >> > --- > >> > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ > >> > 1 file changed, 12 insertions(+) > >> > > >> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi > >> > index 5adfd9bca2ec..72e95afd780e 100644 > >> > --- a/arch/arm/dts/sunxi-u-boot.dtsi > >> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi > >> > @@ -1,5 +1,14 @@ > >> > #include <config.h> > >> > > >> > +/* > >> > + * This is the maximum size the U-Boot binary can be, which is basically > >> > + * the start of the environment, minus the start of the U-Boot binary in > >> > + * the MMC. This makes the assumption that the MMC is using 512-bytes > >> > + * blocks, but devices using something other than that remains to be > >> > + * seen. > >> > + */ > >> > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) > >> > + > >> > / { > >> > binman { > >> > filename = "u-boot-sunxi-with-spl.bin"; > >> > @@ -8,6 +17,9 @@ > >> > filename = "spl/sunxi-spl.bin"; > >> > }; > >> > u-boot-img { > >> > +#ifdef CONFIG_MMC > >> > + size = <UBOOT_MMC_MAX_SIZE>; > >> > +#endif > >> > pos = <CONFIG_SPL_PAD_TO>; > >> > }; > >> > }; > >> > -- > >> > >> This is broken in (at least) two ways: > >> > >> 1. It is simply nonsensical if u-boot and env are on different devices > >> or not on mmc even if mmc support is enabled. > >> > >> 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. > >> At best, this is useless. If the env doesn't immediately follow > >> u-boot, it really breaks things. > >> > >> Please fix or revert, I don't really care which. > > > > The padding is not useless. It reserves space for future size expansions > > and makes it harder for the users to hurt themselves. > > > > For example, if the padded U-Boot size is 1024K while the actual size > > is only ~800K, then adventurous users might be tempted to fit some of > > their data into this gap. Yay, ~200K of storage space for free! Except > > that the next U-Boot release may grow up to 900K without any warning > > and if the users are not careful enough, then their data would be > > corrupted during upgrade. > > Do U-Boot users really need that level of hand-holding? Yes, and that's never a good argument to make, because... > > Could you please tell us what is your problem and why reverting this > > patch would fix it? > > See above. Best case, it just wastes space in the created file. If the > configuration is anything other than U-Boot immediately followed by env > on the same device, it _will_ break things horribly. A few examples: > > 1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env > in SPI NOR flash. In this case, padding the file to the env offset > makes no sense. Writing the image will corrupt anything stored > after U-Boot at a smaller (but still safe) offset. .. I could make pretty much the same one for all your cases, which would be completely useless, and wouldn't fix anything. I guess this one could be solved using an ifdef guard with ENV_IS_IN_MMC. > 2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default > value, is smaller than the offset of U-Boot in its actual device, > the padding will be too large. Writing the image file will then > corrupt a stored env. This one would be covered too. > 3. U-Boot at start of device, env at end as indicated by a negative > CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the > image to (nearly) 2^64 bytes and promptly fills up your storage > device. I'm not too sure about how to fix this one though. Any suggestion? Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Maxime Ripard <maxime.ripard@bootlin.com> writes: > 1;5201;0c > On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: >> Siarhei Siamashka <siarhei.siamashka@gmail.com> writes: >> >> > On Tue, 01 May 2018 18:25:06 +0100 >> > Måns Rullgård <mans@mansr.com> wrote: >> > >> >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: >> >> >> >> > The U-Boot binary may trip over its actual allocated size in the storage. >> >> > In such a case, the environment will not be readable anymore (because >> >> > corrupted when the new image was flashed), and any attempt at using saveenv >> >> > to reconstruct the environment will result in a corrupted U-Boot binary. >> >> > >> >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> >> > --- >> >> > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ >> >> > 1 file changed, 12 insertions(+) >> >> > >> >> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi >> >> > index 5adfd9bca2ec..72e95afd780e 100644 >> >> > --- a/arch/arm/dts/sunxi-u-boot.dtsi >> >> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi >> >> > @@ -1,5 +1,14 @@ >> >> > #include <config.h> >> >> > >> >> > +/* >> >> > + * This is the maximum size the U-Boot binary can be, which is basically >> >> > + * the start of the environment, minus the start of the U-Boot binary in >> >> > + * the MMC. This makes the assumption that the MMC is using 512-bytes >> >> > + * blocks, but devices using something other than that remains to be >> >> > + * seen. >> >> > + */ >> >> > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) >> >> > + >> >> > / { >> >> > binman { >> >> > filename = "u-boot-sunxi-with-spl.bin"; >> >> > @@ -8,6 +17,9 @@ >> >> > filename = "spl/sunxi-spl.bin"; >> >> > }; >> >> > u-boot-img { >> >> > +#ifdef CONFIG_MMC >> >> > + size = <UBOOT_MMC_MAX_SIZE>; >> >> > +#endif >> >> > pos = <CONFIG_SPL_PAD_TO>; >> >> > }; >> >> > }; >> >> > -- >> >> >> >> This is broken in (at least) two ways: >> >> >> >> 1. It is simply nonsensical if u-boot and env are on different devices >> >> or not on mmc even if mmc support is enabled. >> >> >> >> 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. >> >> At best, this is useless. If the env doesn't immediately follow >> >> u-boot, it really breaks things. >> >> >> >> Please fix or revert, I don't really care which. >> > >> > The padding is not useless. It reserves space for future size expansions >> > and makes it harder for the users to hurt themselves. >> > >> > For example, if the padded U-Boot size is 1024K while the actual size >> > is only ~800K, then adventurous users might be tempted to fit some of >> > their data into this gap. Yay, ~200K of storage space for free! Except >> > that the next U-Boot release may grow up to 900K without any warning >> > and if the users are not careful enough, then their data would be >> > corrupted during upgrade. >> >> Do U-Boot users really need that level of hand-holding? > > Yes, and that's never a good argument to make, because... > >> > Could you please tell us what is your problem and why reverting this >> > patch would fix it? >> >> See above. Best case, it just wastes space in the created file. If the >> configuration is anything other than U-Boot immediately followed by env >> on the same device, it _will_ break things horribly. A few examples: >> >> 1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env >> in SPI NOR flash. In this case, padding the file to the env offset >> makes no sense. Writing the image will corrupt anything stored >> after U-Boot at a smaller (but still safe) offset. > > .. I could make pretty much the same one for all your cases, which > would be completely useless, and wouldn't fix anything. Huh? I'm saying we shouldn't "helpfully" do things that actually break perfectly valid setups. U-Boot users are expected to know what they are doing, and shouldn't need that kind of help. In my opinion. > I guess this one could be solved using an ifdef guard with > ENV_IS_IN_MMC. Not if env is on a different mmc device. >> 2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If >> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default >> value, is smaller than the offset of U-Boot in its actual device, >> the padding will be too large. Writing the image file will then >> corrupt a stored env. > > This one would be covered too. > >> 3. U-Boot at start of device, env at end as indicated by a negative >> CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the >> image to (nearly) 2^64 bytes and promptly fills up your storage >> device. > > I'm not too sure about how to fix this one though. Any suggestion? I just don't see the point in trying to pin down the very specific case of U-Boot and env being on the same device with only a (small) amount of padding between them. There are a million other ways for users to screw up, so why should we be making a half-hearted effort to prevent this one?
On Wed, May 02, 2018 at 03:24:50PM +0100, Måns Rullgård wrote: > Maxime Ripard <maxime.ripard@bootlin.com> writes: > > > 1;5201;0c > > On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: > >> Siarhei Siamashka <siarhei.siamashka@gmail.com> writes: > >> > >> > On Tue, 01 May 2018 18:25:06 +0100 > >> > Måns Rullgård <mans@mansr.com> wrote: > >> > > >> >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: > >> >> > >> >> > The U-Boot binary may trip over its actual allocated size in the storage. > >> >> > In such a case, the environment will not be readable anymore (because > >> >> > corrupted when the new image was flashed), and any attempt at using saveenv > >> >> > to reconstruct the environment will result in a corrupted U-Boot binary. > >> >> > > >> >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > >> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >> >> > --- > >> >> > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ > >> >> > 1 file changed, 12 insertions(+) > >> >> > > >> >> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi > >> >> > index 5adfd9bca2ec..72e95afd780e 100644 > >> >> > --- a/arch/arm/dts/sunxi-u-boot.dtsi > >> >> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi > >> >> > @@ -1,5 +1,14 @@ > >> >> > #include <config.h> > >> >> > > >> >> > +/* > >> >> > + * This is the maximum size the U-Boot binary can be, which is basically > >> >> > + * the start of the environment, minus the start of the U-Boot binary in > >> >> > + * the MMC. This makes the assumption that the MMC is using 512-bytes > >> >> > + * blocks, but devices using something other than that remains to be > >> >> > + * seen. > >> >> > + */ > >> >> > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) > >> >> > + > >> >> > / { > >> >> > binman { > >> >> > filename = "u-boot-sunxi-with-spl.bin"; > >> >> > @@ -8,6 +17,9 @@ > >> >> > filename = "spl/sunxi-spl.bin"; > >> >> > }; > >> >> > u-boot-img { > >> >> > +#ifdef CONFIG_MMC > >> >> > + size = <UBOOT_MMC_MAX_SIZE>; > >> >> > +#endif > >> >> > pos = <CONFIG_SPL_PAD_TO>; > >> >> > }; > >> >> > }; > >> >> > -- > >> >> > >> >> This is broken in (at least) two ways: > >> >> > >> >> 1. It is simply nonsensical if u-boot and env are on different devices > >> >> or not on mmc even if mmc support is enabled. > >> >> > >> >> 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. > >> >> At best, this is useless. If the env doesn't immediately follow > >> >> u-boot, it really breaks things. > >> >> > >> >> Please fix or revert, I don't really care which. > >> > > >> > The padding is not useless. It reserves space for future size expansions > >> > and makes it harder for the users to hurt themselves. > >> > > >> > For example, if the padded U-Boot size is 1024K while the actual size > >> > is only ~800K, then adventurous users might be tempted to fit some of > >> > their data into this gap. Yay, ~200K of storage space for free! Except > >> > that the next U-Boot release may grow up to 900K without any warning > >> > and if the users are not careful enough, then their data would be > >> > corrupted during upgrade. > >> > >> Do U-Boot users really need that level of hand-holding? > > > > Yes, and that's never a good argument to make, because... > > > >> > Could you please tell us what is your problem and why reverting this > >> > patch would fix it? > >> > >> See above. Best case, it just wastes space in the created file. If the > >> configuration is anything other than U-Boot immediately followed by env > >> on the same device, it _will_ break things horribly. A few examples: > >> > >> 1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env > >> in SPI NOR flash. In this case, padding the file to the env offset > >> makes no sense. Writing the image will corrupt anything stored > >> after U-Boot at a smaller (but still safe) offset. > > > > .. I could make pretty much the same one for all your cases, which > > would be completely useless, and wouldn't fix anything. > > Huh? I'm saying we shouldn't "helpfully" do things that actually break > perfectly valid setups. U-Boot users are expected to know what they are > doing, and shouldn't need that kind of help. In my opinion. My point is that this is a slippery slope. Anyway.. > > I guess this one could be solved using an ifdef guard with > > ENV_IS_IN_MMC. > > Not if env is on a different mmc device. Ah, right... > >> 2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If > >> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default > >> value, is smaller than the offset of U-Boot in its actual device, > >> the padding will be too large. Writing the image file will then > >> corrupt a stored env. > > > > This one would be covered too. > > > >> 3. U-Boot at start of device, env at end as indicated by a negative > >> CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the > >> image to (nearly) 2^64 bytes and promptly fills up your storage > >> device. > > > > I'm not too sure about how to fix this one though. Any suggestion? > > I just don't see the point in trying to pin down the very specific case > of U-Boot and env being on the same device with only a (small) amount of > padding between them. There are a million other ways for users to screw > up, so why should we be making a half-hearted effort to prevent this > one? Unfortunately, this isn't a very specific case this is the default we shipped for years. And we have people relying on it in the wild. This is why we added that check in the first place, because the size of uboot increased so much that it was now overlapping with the environment. I guess now that we transitioned with the release of 2018.05, we can remove that check entirely, since the default is the first partition, wherever that is. Maxime
Maxime Ripard <maxime.ripard@bootlin.com> writes: > On Wed, May 02, 2018 at 03:24:50PM +0100, Måns Rullgård wrote: >> Maxime Ripard <maxime.ripard@bootlin.com> writes: >> >> > 1;5201;0c >> > On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: >> >> Siarhei Siamashka <siarhei.siamashka@gmail.com> writes: >> >> >> >> > On Tue, 01 May 2018 18:25:06 +0100 >> >> > Måns Rullgård <mans@mansr.com> wrote: >> >> > >> >> >> Maxime Ripard <maxime.ripard@free-electrons.com> writes: >> >> >> >> >> >> > The U-Boot binary may trip over its actual allocated size in the storage. >> >> >> > In such a case, the environment will not be readable anymore (because >> >> >> > corrupted when the new image was flashed), and any attempt at using saveenv >> >> >> > to reconstruct the environment will result in a corrupted U-Boot binary. >> >> >> > >> >> >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> >> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> >> >> > --- >> >> >> > arch/arm/dts/sunxi-u-boot.dtsi | 12 ++++++++++++ >> >> >> > 1 file changed, 12 insertions(+) >> >> >> > >> >> >> > diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi >> >> >> > index 5adfd9bca2ec..72e95afd780e 100644 >> >> >> > --- a/arch/arm/dts/sunxi-u-boot.dtsi >> >> >> > +++ b/arch/arm/dts/sunxi-u-boot.dtsi >> >> >> > @@ -1,5 +1,14 @@ >> >> >> > #include <config.h> >> >> >> > >> >> >> > +/* >> >> >> > + * This is the maximum size the U-Boot binary can be, which is basically >> >> >> > + * the start of the environment, minus the start of the U-Boot binary in >> >> >> > + * the MMC. This makes the assumption that the MMC is using 512-bytes >> >> >> > + * blocks, but devices using something other than that remains to be >> >> >> > + * seen. >> >> >> > + */ >> >> >> > +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) >> >> >> > + >> >> >> > / { >> >> >> > binman { >> >> >> > filename = "u-boot-sunxi-with-spl.bin"; >> >> >> > @@ -8,6 +17,9 @@ >> >> >> > filename = "spl/sunxi-spl.bin"; >> >> >> > }; >> >> >> > u-boot-img { >> >> >> > +#ifdef CONFIG_MMC >> >> >> > + size = <UBOOT_MMC_MAX_SIZE>; >> >> >> > +#endif >> >> >> > pos = <CONFIG_SPL_PAD_TO>; >> >> >> > }; >> >> >> > }; >> >> >> > -- >> >> >> >> >> >> This is broken in (at least) two ways: >> >> >> >> >> >> 1. It is simply nonsensical if u-boot and env are on different devices >> >> >> or not on mmc even if mmc support is enabled. >> >> >> >> >> >> 2. It causes u-boot-sunxi-with-spl.bin to be padded to the env offset. >> >> >> At best, this is useless. If the env doesn't immediately follow >> >> >> u-boot, it really breaks things. >> >> >> >> >> >> Please fix or revert, I don't really care which. >> >> > >> >> > The padding is not useless. It reserves space for future size expansions >> >> > and makes it harder for the users to hurt themselves. >> >> > >> >> > For example, if the padded U-Boot size is 1024K while the actual size >> >> > is only ~800K, then adventurous users might be tempted to fit some of >> >> > their data into this gap. Yay, ~200K of storage space for free! Except >> >> > that the next U-Boot release may grow up to 900K without any warning >> >> > and if the users are not careful enough, then their data would be >> >> > corrupted during upgrade. >> >> >> >> Do U-Boot users really need that level of hand-holding? >> > >> > Yes, and that's never a good argument to make, because... >> > >> >> > Could you please tell us what is your problem and why reverting this >> >> > patch would fix it? >> >> >> >> See above. Best case, it just wastes space in the created file. If the >> >> configuration is anything other than U-Boot immediately followed by env >> >> on the same device, it _will_ break things horribly. A few examples: >> >> >> >> 1. U-Boot and env are on different devices, e.g. U-Boot on mmc and env >> >> in SPI NOR flash. In this case, padding the file to the env offset >> >> makes no sense. Writing the image will corrupt anything stored >> >> after U-Boot at a smaller (but still safe) offset. >> > >> > .. I could make pretty much the same one for all your cases, which >> > would be completely useless, and wouldn't fix anything. >> >> Huh? I'm saying we shouldn't "helpfully" do things that actually break >> perfectly valid setups. U-Boot users are expected to know what they are >> doing, and shouldn't need that kind of help. In my opinion. > > My point is that this is a slippery slope. Anyway.. Yes, trying to be overly helpful is indeed a slippery slope. >> > I guess this one could be solved using an ifdef guard with >> > ENV_IS_IN_MMC. >> >> Not if env is on a different mmc device. > > Ah, right... > >> >> 2. U-Boot at a non-zero offset, followed by env, but _not_ on mmc. If >> >> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR, probably at its default >> >> value, is smaller than the offset of U-Boot in its actual device, >> >> the padding will be too large. Writing the image file will then >> >> corrupt a stored env. >> > >> > This one would be covered too. >> > >> >> 3. U-Boot at start of device, env at end as indicated by a negative >> >> CONFIG_ENV_OFFSET. With this configuration, binman tries to pad the >> >> image to (nearly) 2^64 bytes and promptly fills up your storage >> >> device. >> > >> > I'm not too sure about how to fix this one though. Any suggestion? >> >> I just don't see the point in trying to pin down the very specific case >> of U-Boot and env being on the same device with only a (small) amount of >> padding between them. There are a million other ways for users to screw >> up, so why should we be making a half-hearted effort to prevent this >> one? > > Unfortunately, this isn't a very specific case this is the default we > shipped for years. Your default is still just one specific case of nearly infinite possibilities. > And we have people relying on it in the wild. How can anyone *rely* on that padding being there? > This is why we added that check in the first place, because the size > of uboot increased so much that it was now overlapping with the > environment. I get that. Unfortunately, the check was flawed for all but your default use case. > I guess now that we transitioned with the release of 2018.05, we can > remove that check entirely, since the default is the first partition, > wherever that is. I don't care how you justify it as long as you get rid of the broken code.
diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi index 5adfd9bca2ec..72e95afd780e 100644 --- a/arch/arm/dts/sunxi-u-boot.dtsi +++ b/arch/arm/dts/sunxi-u-boot.dtsi @@ -1,5 +1,14 @@ #include <config.h> +/* + * This is the maximum size the U-Boot binary can be, which is basically + * the start of the environment, minus the start of the U-Boot binary in + * the MMC. This makes the assumption that the MMC is using 512-bytes + * blocks, but devices using something other than that remains to be + * seen. + */ +#define UBOOT_MMC_MAX_SIZE (CONFIG_ENV_OFFSET - (CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR * 512)) + / { binman { filename = "u-boot-sunxi-with-spl.bin"; @@ -8,6 +17,9 @@ filename = "spl/sunxi-spl.bin"; }; u-boot-img { +#ifdef CONFIG_MMC + size = <UBOOT_MMC_MAX_SIZE>; +#endif pos = <CONFIG_SPL_PAD_TO>; }; };