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

Bruno Pena brunompena at gmail.com
Wed Jan 22 04:22:01 EST 2020


Hi Steve,

I just noticed that I have just suggested you to change the wrong
section... sorry!
It's clearly stated on target/linux/ath79/image/generic-tp-link.mk that the
A7-v5 uses the tplink-safeloader-uimage section but I somehow managed to
copy the wrong one (doh!).
    define Device/tplink_archer-a7-v5
      $(Device/tplink-safeloader-uimage)
      SOC := qca9563
      [...]

On the bright side, today I actually got some free minutes to spare and I
used them to test some changes to the makefiles.
I have added the following entries to get an overview of final partition
size:
    IMAGES += kernel_1.bin kernel_2.bin kernel_3.bin kernel_4.bin
kernel_5.bin kernel_6.bin
    IMAGE/kernel_1.bin := kernel-bin
    IMAGE/kernel_2.bin := kernel-bin | append-dtb
    IMAGE/kernel_3.bin := kernel-bin | append-dtb | lzma
    IMAGE/kernel_4.bin := kernel-bin | append-dtb | lzma | uImageArcher lzma
    IMAGE/kernel_5.bin := kernel-bin | append-dtb | lzma | pad-to 64k |
uImageArcher lzma
    IMAGE/kernel_6.bin := kernel-bin | append-dtb | lzma | uImageArcher
lzma | pad-to 64k

And these were the results:
    kernel_1.bin : 1804719 bytes [0x1b89af]
    kernel_2.bin : 1813487 bytes [0x1babef]
    kernel_3.bin : 1831548 bytes [0x1bf27c]
    kernel_4.bin : 1831612 bytes [0x1bf2bc]
    kernel_5.bin : 1835072 bytes [0x1c0040]
    kernel_6.bin : 1835008 bytes [0x1c0000]

I have then proceeded to generate the corresponding sysupgrade.bin image
with the settings of kernel_5 and kernel_6 so I could manually inspect the
resulting files.

Original sysupgrade.bin (rootfs starts at 0x001b89af):
    001b89a0  68 81 9d 3b bd c9 86 98  76 c4 f0 1e 0e 94 e0 68
|h..;....v......h|
    001b89b0  73 71 73 57 03 00 00 00  6e 27 5e 00 00 04 00 0f
|sqsW....n'^.....|
    001b89c0  00 00 00 04 00 12 00 c0  06 01 00 04 00 00 00 ff
|................|

Settings from kernel_5 sysupgrade.bin (rootfs starts at 0x001c0040):
    001b89b0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
|................|
    *
    001c0040  68 73 71 73 57 03 00 00  00 6e 27 5e 00 00 04 00
|hsqsW....n'^....|

Settings from kernel_6 sysupgrade.bin (rootfs starts at 0x001c0000):
    001b89b0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
|................|
    *
    001c0000  68 73 71 73 57 03 00 00  00 6e 27 5e 00 00 04 00
|hsqsW....n'^....|

Based on these I would ask if you could test again with the following
change to target/linux/ath79/image/common-tp-link.mk (append "| pad-to 64k"
to the KERNEL line):
    define Device/tplink-safeloader-uimage
      $(Device/tplink-safeloader)
      KERNEL := kernel-bin | append-dtb | lzma | uImageArcher lzma | pad-to
64k
    endef

I would also like to take the opportunity to ask if it's worth pursuing
this patch if it means that all devices (using mtd) will require their
partitions to be padded to the blocksize?

Thank you and best regards,
Bruno Pena

On Wed, Jan 22, 2020, 07:13 Bruno Pena <brunompena at gmail.com> wrote:

> 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/36208a9f/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