Some comments inline below ...
> 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. Note also, e.g. tplink,tl-wdr3600-v1: [ 0.428860] m25p80 spi0.0: en25q64 (8192 Kbytes) [ 0.433638] 3 fixed-partitions partitions found on MTD device spi0.0 [ 0.440112] Creating 3 MTD partitions on "spi0.0": [ 0.444991] 0x000000000000-0x000000020000 : "u-boot" [ 0.450914] 0x000000020000-0x0000007f0000 : "firmware" [ 0.459935] 2 tplink-fw partitions found on MTD device firmware [ 0.465951] Creating 2 MTD partitions on "firmware": [ 0.471047] 0x000000000000-0x0000001b6b1b : "kernel" [ 0.476924] 0x0000001b6b1c-0x0000007d0000 : "rootfs" netgear,wndr3800: [ 0.671227] m25p80 spi0.0: mx25l12805d (16384 Kbytes) [ 0.676366] 4 fixed-partitions partitions found on MTD device spi0.0 [ 0.682724] Creating 4 MTD partitions on "spi0.0": [ 0.687508] 0x000000000000-0x000000050000 : "u-boot" [ 0.693223] 0x000000050000-0x000000070000 : "u-boot-env" [ 0.699163] 0x000000070000-0x000000ff0000 : "firmware" [ 0.707493] 2 netgear-fw partitions found on MTD device firmware [ 0.713550] Creating 2 MTD partitions on "firmware": [ 0.718507] 0x000000000000-0x0000001bd440 : "kernel" [ 0.724195] 0x0000001bd440-0x000000f80000 : "rootfs" and netgear wgt634u: [ 1.245465] 3 bcm47xxpart partitions found on MTD device physmap-flash.0 [ 1.252454] Creating 3 MTD partitions on "physmap-flash.0": [ 1.258364] 0x000000000000-0x0000000a0000 : "boot" [ 1.286600] 0x0000000a0000-0x0000007e0000 : "firmware" [ 1.298172] 3 trx partitions found on MTD device firmware [ 1.303639] Creating 3 MTD partitions on "firmware": [ 1.308944] 0x00000000001c-0x000000000948 : "loader" [ 1.331486] 0x000000000948-0x000000139800 : "linux" [ 1.348577] 0x000000139800-0x000000740000 : "rootfs" as examples where the rootfs starts at unaligned addresses. If you padded the rootfs individually, the combination of kernel+rootfs would unnecessarily waste space. > 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 trivial spelling fix, that should be "accepts", I think... > 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/> AFAICT, the PAGE_SIZE on Meraki MR24 is 4k. In the kernel's include/asm-generic/page.h, we have: /* PAGE_SHIFT determines the page size */ #define PAGE_SHIFT 12 #ifdef __ASSEMBLY__ #define PAGE_SIZE (1 << PAGE_SHIFT) #else #define PAGE_SIZE (1UL << PAGE_SHIFT) #endif > > 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!) When Jonas and I were discussing this, we noted that sysupgrade changes won't be installed the first time you do this, so a lack of padding to the root.squashfs can still result in boot looping. Since Meraki MR24 (afaict) doesn't use the ubinize-image.sh script, it won't be protected. > > 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 -- Russell Senior, President [email protected] _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
