[OpenWrt-Devel] [PATCH v2] fstools: Add support to read-only MTD partitions (eg. recovery images)
Bruno Pena
brunompena at gmail.com
Wed Jan 22 01:13:20 EST 2020
Hi Steve,
Thank you for testing.
You implemented my suggestion correctly but it seems that it didn't
actually pad anything (the sizes of the partitions should be rounded to the
next 0x10000 block).
Please allow me a few days (real-life constraints) to test some changes to
the common-tp-link.mk before I waste more of your time with these tests.
Best regards,
Bruno Pena
On Wed, Jan 22, 2020 at 12:16 AM Steve Brown <sbrown at ewol.com> wrote:
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20200122/abb31f03/attachment.htm>
-------------- next part --------------
_______________________________________________
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