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

[Bruno Pena]

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/20200121/cfc3e3f8/attachment.htm>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel