[OpenWrt-Devel] [PATCH v2] fstools: Add support to read-only MTD partitions (eg. recovery images)

Steve Brown sbrown at ewol.com
Tue Jan 21 18:16:03 EST 2020


Hi Bruno,

That didn't seem to solve the problem. The padding didn't seem to take
effect.

I reverted 0f81a0979 and 0c707d37b.

dev:    size   erasesize  name
mtd0: 00020000 00010000 "factory-uboot"
mtd1: 00020000 00010000 "u-boot"
mtd2: 00ec0000 00010000 "firmware"
mtd3: 001a3a07 00010000 "kernel"
mtd4: 00d1c5f9 00010000 "rootfs"
mtd5: 009f0000 00010000 "rootfs_data"
mtd6: 00020000 00010000 "info"
mtd7: 00050000 00010000 "config"
mtd8: 00010000 00010000 "partition-table"
mtd9: 00010000 00010000 "art"

[    0.414677] Creating 7 MTD partitions on "spi0.0":
[    0.419655] 0x000000000000-0x000000020000 : "factory-uboot"
[    0.426092] 0x000000020000-0x000000040000 : "u-boot"
[    0.431906] 0x000000040000-0x000000f00000 : "firmware"
[    0.439772] 2 uimage-fw partitions found on MTD device firmware
[    0.445891] Creating 2 MTD partitions on "firmware":
[    0.451065] 0x000000000000-0x0000001a3a07 : "kernel"
[    0.456840] 0x0000001a3a07-0x000000ec0000 : "rootfs"
[    0.462643] mtd: device 4 (rootfs) set to be root filesystem
[    0.469746] 1 squashfs-split partitions found on MTD device rootfs
[    0.476142] 0x0000004d0000-0x000000ec0000 : "rootfs_data"
[    0.482441] 0x000000f40000-0x000000f60000 : "info"
[    0.488033] 0x000000f60000-0x000000fb0000 : "config"
[    0.493856] 0x000000fc0000-0x000000fd0000 : "partition-table"
[    0.500494] 0x000000ff0000-0x000001000000 : "art"


diff --git a/target/linux/ath79/image/common-tp-link.mk b/target/linux/ath79/image/common-tp-link.mk
index 8aa6a5a2be..89e73a85f3 100644
--- a/target/linux/ath79/image/common-tp-link.mk
+++ b/target/linux/ath79/image/common-tp-link.mk
@@ -63,7 +63,7 @@ endef
 
 define Device/tplink-safeloader
   $(Device/tplink)
-  KERNEL := kernel-bin | append-dtb | lzma | tplink-v1-header -O
+  KERNEL := kernel-bin | append-dtb | lzma | pad-to $$$$(BLOCKSIZE) | tplink-v1-header -O
   IMAGE/sysupgrade.bin := append-rootfs | tplink-safeloader sysupgrade | \
     append-metadata | check-size $$$$(IMAGE_SIZE)
   IMAGE/factory.bin := append-rootfs | tplink-safeloader factory

Can you verify that I did this right?

Steve


On Tue, 2020-01-21 at 22:10 +0100, Bruno Pena wrote:
> Hi everyone,
> 
> I was finally able to replicate the issue you are observing.
> 
> The problem comes from the fact that the kernel partition on the TP-
> Link images is not padded to the write blocksize - which can be
> observed on the dmesg from Steve:
> [    0.450987] 0x000000000000-0x0000001a39ea : "kernel"
> [    0.456772] 0x0000001a39ea-0x000000ec0000 : "rootfs"
> The blocksize can be seen observed on the /proc/mtd and for that
> device is 0x10000:
> mtd3: 001a38de 00010000 "kernel"
> mtd4: 00d1c722 00010000 "rootfs"
> This triggers the following code on drivers/mtd/mtdpart.c that
> removes the write flag from the rootfs partition:
>         tmp = part_absolute_offset(parent) + slave->offset;
>         remainder = do_div(tmp, wr_alignment);
>         if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {
>                 /* Doesn't start on a boundary of major erase size */
>                 slave->mtd.flags |= MTD_ERASE_PARTIAL;
>                 if (((u32)slave->mtd.size) > parent->erasesize)
>                         slave->mtd.flags &= ~MTD_WRITEABLE;
>                 else
>                         slave->mtd.erasesize = slave->mtd.size;
>         }
> 
>         tmp = part_absolute_offset(parent) + slave->offset + slave-
> >mtd.size;
>         remainder = do_div(tmp, wr_alignment);
>         if ((slave->mtd.flags & MTD_WRITEABLE) && remainder) {
>                 slave->mtd.flags |= MTD_ERASE_PARTIAL;
> 
>                 if ((u32)slave->mtd.size > parent->erasesize)
>                         slave->mtd.flags &= ~MTD_WRITEABLE;
>                 else
>                         slave->mtd.erasesize = slave->mtd.size;
>         }
> Now we have a rootfs partition that is set to read-only, and with the
> kernel patch that forces sub-partitions to match the access mode of
> the parent, when the split runs it will force the rootfs_data
> partition to match the parent access mode (read-only).
> 
> My suggestion is to change the target/linux/ath79/image/common-tp-
> link.mk so it pads the kernel partition to the blocksize (untested
> suggestion below):
> define Device/tplink-safeloader
>   $(Device/tplink)
>   KERNEL := kernel-bin | append-dtb | lzma | pad-to $$$$(BLOCKSIZE) |
> tplink-v1-header -O
> [...]
> Would any of you be willing to spend some time testing this change on
> your TP-Link?
> 
> Thank you and best regards,
> Bruno Pena
> 
> On Tue, Jan 21, 2020 at 8:15 PM Bruno Pena <brunompena at gmail.com>
> wrote:
> > Hi Petr,
> > 
> > Thank you for reverting the patches.
> > 
> > I'm trying to replicate the issue but so far I haven't had any
> > luck, and just by looking at the code I'm not seeing where/what is
> > could be breaking.
> > 
> > Regarding the upstream patch, that one is just an enabler (read: it
> > only extends the "mtd_add_partition" function but it does not add
> > any code to force the access mode on sub-partitions).
> > The reason for this is because this enforcement is done on the mtd
> > parser code that is OpenWrt specific and implemented via kernel
> > patches (not present on upstream).
> > 
> > Best regards,
> > Bruno Pena
> > 
> > On Tue, Jan 21, 2020 at 7:57 PM Petr Štetiar <ynezz at true.cz> wrote:
> > > Bruno Pena <brunompena at gmail.com> [2020-01-21 14:53:54]:
> > > 
> > > Hi,
> > > 
> > > > Based on the last comment from Steve (fstools patch was not
> > > reverted), I'm
> > > > not sure if that's the root cause.
> > > 
> > > you need to find out, but that Daniel's remark seems legit to me,
> > > your patch
> > > likely changed the mtd device open order/flags.
> > > 
> > > > The kernel patch (which when reverted makes rootfs_data
> > > writable again)
> > > > only enforces the parent mtd access mode on the sub-partitions.
> > > 
> > > FYI I currently dont have time to fix that regression myself and
> > > since its
> > > likely affecting a lot of users, so I've reverted those changes.
> > > I think, that
> > > this change is useful, so I'm still willing to merge it once
> > > fixed, no
> > > worries. Having some upstream feedback beforehand would be a
> > > plus.
> > > 
> > > BTW it would be fair to inform upstream about this possible issue
> > > as well, so
> > > the patch wont get merged in its current state, unless its double
> > > checked (or
> > > just write them, that you're planning v2, so the patch is removed
> > > from their
> > > Patchwork).
> > > 
> > > -- ynezz


_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel


More information about the openwrt-devel mailing list