On Saturday, August 24, 2019 2:18:55 AM CEST Russell Senior wrote: > >>>>> "Christian" == Christian Lamparter <[email protected]> writes: > > > I've posted a similar message to the bugreport: > > <https://bugs.openwrt.org/index.php?do=details&task_id=2460> > > I should have filed the bug first and linked it in my patch. I think it's fine. It depends on whenever there will be a discussion and where it will take place... But yeah, nobody can tell in advance how this will go.
On Saturday, August 24, 2019 2:59:31 AM CEST Russell Senior wrote: > >>>>> "Russell" == Russell Senior <[email protected]> writes: > > >>>>> "Christian" == Christian Lamparter <[email protected]> writes: > > Russell> It's mostly inferred from the fact that I saw the error and > Russell> kernel panic, and glancing at the kernel squashfs code. I am > Russell> not pretending to understand completely how the squashfs kernel > Russell> code works, but the position of the error relative to the size > Russell> of the rootfs in my panic case strongly suggests it was trying > Russell> to read past the end of the ubi volume. > > Oh, and I got important help finding this from Jonas Gorski. I was > remiss in not mentioning that. > Ok, Let's add him to the CC then. As well as some of the "ramips: Fix and tidy up IMAGE_SIZE #2226" and "[RFC] Use DTS firmware partition to obsolete IMAGE_SIZE #2310" https://github.com/openwrt/openwrt/pull/2226 https://github.com/openwrt/openwrt/pull/2310 crowd. Because this will likely affect them as well... But they might not know it. In any case: "Welcome everyone! :-D". > > What's happening here is that mksquashfs4 is being told > > through the "-nopad" option to "do not pad filesystem to a > > multiple of 4K" [0]. > > > |define Image/mkfs/squashfs | > > $(STAGING_DIR_HOST)/bin/mksquashfs4 $(call > > mkfs_target_dir,$(1)) $@ \ | -nopad -noappend -root-owned \ | > > -comp $(SQUASHFSCOMP) $(SQUASHFSOPT) \ | -processors 1 |endef > > > My guess is that this affects more than just the MR24 (I'm > > looking at you too: pad2jffs and various pad-to $value) > > . I've tried tracking down the change that added the "-nopad" > > and found it in a commit from 2005 titled: "add some changes > > from whiterussian to head" [1] [2]: > > I agree that other devices where rootfs is squashfs and lives on a ubi > volume are probaby also vulnerable to this problem. Regrettably, I haven't > thought of any other of those devices that I have on hand to test. > > > | $(KDIR)/root.squashfs: | @mkdir -p $(KDIR)/root/jffs |- > > $(STAGING_DIR)/bin/mksquashfs-lzma $(KDIR)/root $@ -noappend > > -root-owned -le |+ $(STAGING_DIR)/bin/mksquashfs-lzma > > $(KDIR)/root $@ -nopad -noappend -root-owned -le > > > > So, this is really old... > > > Question is, should we just drop the -nopad? Since as you > > established, in the described corner-case (see above) > > squashfs needs this 4k padding and the generated squashfs > > could be considered broken otherwise? > > I'm under the impression that the -nopad makes sense for NOR flash where > the kernel and rootfs are glued together, padding the isolated rootfs > would cause alignment problems or wasted space in the combined blobs. Yes, that's the nod to padjffs2. That said, <https://sourceforge.net/p/squashfs/mailman/message/28307755/> makes it sound like that apart from the BLOCKSIZE, we also need to PAGE_SIZE? (I think the APM821XX is a special case, since it can do 64KiB Pages as well as it's 32MiB SLC NAND that uses 16 KiB erase-blocks. So a PAGE can span up to 4 pages. > > > (Judging from your > > message, you went through the kernel code. Can you please > > share the place where the lack of the padding is breaking the > > fs?) > > It's mostly inferred from the fact that I saw the error and kernel > panic, and glancing at the kernel squashfs code. I am not pretending to > understand completely how the squashfs kernel code works, but the > position of the error relative to the size of the rootfs in my panic > case strongly suggests it was trying to read past the end of the ubi > volume. > > The error comes in the kernel's fs/squashfs/block.c in the > squashfs_read_data() function. There are a bunch of conditions (at least > 5) within the function (see "goto read_failure;") that will lead to that > message being printed. > Well, that's a pity this could have saved a lot of time. I've cobbered together a patch that deals with some of the padding issues at "ubimkvol" and "ubinize" time. The idea is that ideally we want to do the padding when we know PAGE_SIZE and the BLOCKSIZE/Erasesize (MR24 blocksize in image/Makefile seems wrong as well...). But for now, it's set to 64KiB. If this is the way forward we add enable getconf and get the PAGESIZE at runtime. If not, we need to come up with something else. (It's also possible to do some changes in ubi's block code or squashfs kernel code to mitigate the padding, but I don't think the maintainers will even look at it). Regards, Christian --- >From 803cab7d585ebb85362357d008067caf645a7f17 Mon Sep 17 00:00:00 2001 From: Christian Lamparter <[email protected]> Date: Sat, 24 Aug 2019 12:55:40 +0200 Subject: [PATCH] base-files: pad root.squashfs to 64KiB in ubi volumes SquashFS's HOWTO states in the section "Using mksquashfs" <https://elinux.org/Squash_FS_Howto#Using_mksquashfs> that a padding is necessary "for the filesystem to be used on block devices." OpenWrt's mksquashfs for the rootfs (which is usually squashfs) is instructed to skip the padding via the nopad option because the rootfs will be padded by functions like pad-rootfs to the eraseblocksize which is usually much bigger at somewhere 64KiB. But this is a problem for squashfs as rootfs in ubi partitions. Currently no explicit padding is being set and it currently works for the most time because ubi volumes are always aligned to LEBs which could be close enough for 4KiB paddings... Digging down deeper revealed that squashfs excepts blocks to be up to PAGE_SIZE. This is explained in this bug report that states that the 4k padding for ARCHs with 64KiB pages resulted in "attempt access beyond end of device" errors: <https://sourceforge.net/p/squashfs/mailman/message/28307755/> This patch changes sysupgrade to follow fstools with its ROOTDEV_OVERLAY_ALIGN (=64KiB) and aligns squashfs rootfs filesystem to the same amount, while also changing the ubinize script to apply the alignment in the same manner. (More additions would be welcome. Note: ubinize and ubimkvol don't support alignment values that are bigger than a LEB!) Reported-by: Russell Senior <[email protected]> Signed-off-by: Christian Lamparter <[email protected]> --- package/base-files/files/lib/upgrade/nand.sh | 12 +++++++++--- scripts/ubinize-image.sh | 12 +++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/package/base-files/files/lib/upgrade/nand.sh b/package/base-files/files/lib/upgrade/nand.sh index fbc2b5c19a..7eb9632a06 100644 --- a/package/base-files/files/lib/upgrade/nand.sh +++ b/package/base-files/files/lib/upgrade/nand.sh @@ -174,11 +174,17 @@ nand_upgrade_prepare_ubi() { # update rootfs local root_size_param - if [ "$rootfs_type" = "ubifs" ]; then + case "$rootfs_type" in + "squashfs") + root_size_param="-s $(( ($rootfs_length + 65535) & ~65535))" + ;; + "ubifs") root_size_param="-m" - else + ;; + *) root_size_param="-s $rootfs_length" - fi + ;; + esac if ! ubimkvol /dev/$ubidev -N $CI_ROOTPART $root_size_param; then echo "cannot create rootfs volume" return 1; diff --git a/scripts/ubinize-image.sh b/scripts/ubinize-image.sh index a18d6dc428..06f4a3b995 100755 --- a/scripts/ubinize-image.sh +++ b/scripts/ubinize-image.sh @@ -18,6 +18,12 @@ is_ubifs() { fi } +is_squashfs() { + if [ "$( get_magic_word "$1" )" = "6873" ]; then + echo "1" + fi +} + ubivol() { volid=$1 name=$2 @@ -69,7 +75,11 @@ ubilayout() { ubivol $vol_id kernel "$3" vol_id=$(( $vol_id + 1 )) fi - ubivol $vol_id rootfs "$2" $root_is_ubifs + size="" + if [ -n "$( is_squashfs "$2" )" ]; then + size=$(( ($(wc -c < "$2") + 65535) & ~65535)) + fi + ubivol $vol_id rootfs "$2" "$root_is_ubifs" "$size" vol_id=$(( $vol_id + 1 )) [ "$root_is_ubifs" ] || ubivol $vol_id rootfs_data "" 1 } -- 2.23.0 --- _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
