Maxime Ripard <maxime.rip...@bootlin.com> writes: > On Wed, May 02, 2018 at 03:24:50PM +0100, Måns Rullgård wrote: >> Maxime Ripard <maxime.rip...@bootlin.com> writes: >> >> > 1;5201;0c >> > On Wed, May 02, 2018 at 10:34:49AM +0100, Måns Rullgård wrote: >> >> Siarhei Siamashka <siarhei.siamas...@gmail.com> writes: >> >> >> >> > On Tue, 01 May 2018 18:25:06 +0100 >> >> > Måns Rullgård <m...@mansr.com> wrote: >> >> > >> >> >> Maxime Ripard <maxime.rip...@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.przyw...@arm.com> >> >> >> > Signed-off-by: Maxime Ripard <maxime.rip...@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. -- Måns Rullgård _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot