Hello Heiko, Am Thu, Jul 04, 2024 at 06:28:31AM +0200 schrieb Heiko Schocher: > Hello Alexander, > > On 03.07.24 12:12, Alexander Dahl wrote: > > Hei hei, > > > > filesystem handling is different in U-Boot and beyond that UBI/UBIFS is > > different from other filesystems in U-Boot. There's UBI and UBIFS code > > ported from Linux (quite old already now, maybe someone wants to update > > that?), and there's "glue code" or "wrapper code" to interface with > > U-Boot scripts, commands, and filesystem handling. The fixes and > > improvements in this patch series are for this U-Boot specific glue > > code. > > Yes, the linux base is very old ... patches are welcome!
The last sync was back in 2015 from linux v4.2, there were 800+ changes to ubi/ubifs in Linux since then. :-/ > And for me it is not that easy, as I do not have a hardware with > current mainline U-Boot running on it... I want to update a hardware > I have to current mainline, but I had no time yet for it... Besides the custom hardware here, I used Microchip SAM9X60-Curiosity lately, which is coming with a raw NAND flash and can boot from it. > > > I'm no filesystem expert, but after days of debugging I'm quite sure the > > bug is in U-Boot since UBIFS support was added in 2009, and it was > > repeated in 2015 when generic filesystem support for UBIFS was added. > > So please review carefully! > > Which bug? The memory leak and double free fixed with patch 1 of the series. > > > The crashes were not easily reproducible, only with boards using the old > > distroboot _and_ a boot script inspired by (but not equal to) the one > > proposed by RAUC [1], which basically boils down to: > > > > ubifsmount ubi0:boot (from distroboot) > > test -e (from distroboot) > > ubifsmount ubi0:rootfs1 (this time from the boot script, > > triggering a ubifs_umount) > > So, you have a special sequence you execute to trigger the bug, good! > > In special 2 ubifsmount in a row... may not often needed for booting! > (I ask me, why that is needed? Boottime is not good than...) Using distroboot with a script here. The script is in a separate UBI volume ubi0:boot, kernel is loaded from ubi0:rootfs1 or ubi0:rootfs2 however. So there is 'ubifsmount ubi0:boot' from distroboot and in the script found, loaded, and run there is 'ubifsmount ubi0:rootfs1' (or rootfs2) later. ubifsmount calls ubifsumount internally if some volume is mounted already. > > BTW: Is this really a good bootcmd in [1] as on *every* boot your > Environment is saved? This is not good for lifetime of your > storage device ... why not using bootcounter? Well, I was not aware of bootcounter, but I had a look and the actual counter must be stored somewhere. Possible are: - pmic → has no storage possibility on my board - rtc → soc internal only, volatile in the end (if battery is empty) - i2c eeprom → missing - spi flash → missing - filesystem → ends up on the flash - nvmem → no other nvmems present - syscon or some cpu register or sram → volatile So none of these are possible in my case, I only have a raw NAND as storage and thus I have to use U-Boot env, which is stored in UBI here btw to not stress the flash too much. I could investigate if it would be possible to let RAUC use the U-Boot bootcounter infrastructure, but currently RAUC depends on U-Boot environment variables for tracking boot attempts. btw: documentation of bootcount is sparse, I only found the very short 'doc/README.bootcount' and it's not even migrated to recent U-Boot sphinx based docs. ;-) But from what I understood the concept is the same, U-Boot counts something and Linux userspace has to reset it. The counter must be stored somewhere, for example in U-Boot env if no other storage is possible. > > > Crashes can be triggered more easily, if patch order is changed and > > patch 2 (resetting pointers to NULL after free) comes first, or if patch > > 2 is applied on its own only. > > Hmm... > > > The fix is in the first patch, and on my boards I see no crashes > > anymore. I also tested all kinds of combinations of calling `ubi part`, > > `ubi detach`, `ubifsmount`, `ubifsumount`, `ubifsls`, `ubifsload`, `ls`, > > `load`, `size`, and `test -e` and got no crashes anymore after the fix. > > That sounds good! Hmm.. test -e has nothing to do with ubi/ubifs I think. Oh it has, 'test -e' calls file_exists() which calls fs_exists() which ends up calling ubifs_exists() which is one of the functions causing an immediate memory leak, see patch 1. > On what hardware do you test? Is it in mainline? Tested on custom hardware, but I'm confident it should be reproducible on any board using ubifs, especially after applying patch 2 resetting pointers of freed memory to NULL. This should trigger the bug with the simple sequence already described: > ubifsmount ubi0:anyvolume > ls ubi ubi0:anyvolume / # (or load, or test -e, or size) > ubifsumount ubifsumount will call ubifs_umount() which calls ubi_close_volume(c->ubi), that pointer is either invalid leading to a double free inside of ubi_close_volume() and it will crash only in certain conditions or the pointer is NULL after applying patch 2 of the series, then ubi_close_volume() crashes right away with a NULL pointer exception. Note: without patch 2 it very much depends on the sequence of commands, but after the first ubi_close_volume() triggered by ls/load/size/exists the pointer in ubifs_sb is invalid, but accessed later by the second ubi_close_volume() triggered by ubifs_umount(). If you do something in between those using the freed memory by something else again, the second ubi_close_volume() access might get corrupted data or access things outside of RAM. Patch 2 redirects this on a clean NULL pointer exception you can easily trigger. In my case I got a pointer variable actually containing a string "ng.." aka 0x2e2e676e which looked suspiciously similar to a valid pointer on the platform somewhere in RAM between 0x20000000 and 0x28000000 so it took me two days to realize it's not a pointer. ;-) > > > The three additional patches (2 to 4) are more or less safeguards and > > improvements for the future, and come from me trying and my debugging > > code done on the way, more or less optional, but I think nice to have. > > I will look at them .. but give me some time, as I am in holidays the > next 2 weeks ... Hmm.. and it would be good to get some Tested-by > from people with hardware... Take your time, no need to work in holidays. Would appreciate a Tested-by by anyone else though, maybe some of the raw NAND folks? Greets Alex > > bye, > Heiko > > > > Greets > > Alex > > > > [1] https://github.com/rauc/rauc/blob/master/contrib/uboot.sh > > > > Alexander Dahl (4): > > fs: ubifs: Fix memleak and double free in u-boot wrapper functions > > fs: ubifs: Set pointers to NULL after free > > fs: ubifs: Make k(z)alloc/kfree symmetric > > fs: ubifs: Add volume mounted check > > > > fs/ubifs/super.c | 8 ++++++-- > > fs/ubifs/ubifs.c | 31 +++++++++++++++++++------------ > > 2 files changed, 25 insertions(+), 14 deletions(-) > > > > > > base-commit: 65fbdab27224ee3943a89496b21862db83c34da2 > > > > -- > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: h...@denx.de
