Message-ID: <ZnCcsPA-flVcxiAT@makrotopia.org>
Date: Mon, 17 Jun 2024 21:29:36 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Richard Weinberger <richard@....at>
Cc: Gagan Sidhu <broly@....com>, ZhaoLong Wang <wangzhaolong1@...wei.com>,
	chengzhihao1 <chengzhihao1@...wei.com>,
	dpervushin <dpervushin@...eddedalley.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-mtd <linux-mtd@...ts.infradead.org>,
	Miquel Raynal <miquel.raynal@...tlin.com>,
	Vignesh Raghavendra <vigneshr@...com>,
	yangerkun <yangerkun@...wei.com>, yi zhang <yi.zhang@...wei.com>
Subject: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by
 ftl notifier

Hi Richard,

On Mon, Jun 17, 2024 at 08:52:55PM +0200, Richard Weinberger wrote:
> [CC'ing Daniel]
> 
> ----- Ursprüngliche Mail -----
> > Von: "Gagan Sidhu" <broly@....com>
> > An: "richard" <richard@....at>
> > CC: "ZhaoLong Wang" <wangzhaolong1@...wei.com>, "chengzhihao1" <chengzhihao1@...wei.com>, "dpervushin"
> > <dpervushin@...eddedalley.com>, "linux-kernel" <linux-kernel@...r.kernel.org>, "linux-mtd"
> > <linux-mtd@...ts.infradead.org>, "Miquel Raynal" <miquel.raynal@...tlin.com>, "Vignesh Raghavendra" <vigneshr@...com>,
> > "yangerkun" <yangerkun@...wei.com>, "yi zhang" <yi.zhang@...wei.com>
> > Gesendet: Montag, 17. Juni 2024 20:46:10
> > Betreff: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier
> 
> >> On Jun 17, 2024, at 12:32 PM, Richard Weinberger <richard@....at> wrote:
> >> 
> >> ----- Ursprüngliche Mail -----
> >>> Von: "Gagan Sidhu" <broly@....com>
> >>>> AFAICT, this log line is not part of the mainline kernel.
> >>> 
> >>> this is mainline. it’s just not 6.x. it’s 4.14.
> >> 
> >> I've double checked and disagree.
> >> This line comes from:
> >> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/pending-4.14/480-mtd-set-rootfs-to-be-root-dev.patch;h=6cddaf01b75cb58cfb377f568f2c375af87e2f1b;hb=c3bd1321de1e0d814f5cfc4f494f6b2fb1f5133b
> > 
> > no i know that, that’s the patch i showed you. i meant the rest of it is
> > mainline. the patch obviously is not.
> >> 
> >> In recent OpenWRT kernels I see:
> >> https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/generic/pending-5.15/493-ubi-set-ROOT_DEV-to-ubiblock-rootfs-if-unset.patch;h=266a6331c2acc0f7c17d9ac72f54659d31b56249;hb=HEAD
> >> 
> >> Looks like in recent versions the patch in question does *not* cause a
> >> regression.
> > 
> > that patch is also applied in my version as well, so i don’t see how this avoids
> > the regression.
> > 
> > https://github.com/torvalds/linux/blob/master/drivers/mtd/mtdcore.c#L774
> > 
> > mine says "[6.051426] mtd: device 12 (rootfs) set to be root filesystem"
> > 
> > which is simply the call from drivers/mtd/mtdcore.c
> > 
> > so the rootfs device is set correctly. it’s just not booting from it.
> > 
> > the regression comes from having GLUEBI+BLOCK enabled, it seems, are they
> > fighting for/operating on the same partition?
> 
> I don't know. Let's ask Daniel.

I've tried to follow up with this thread and to understand what this is all
about.

Let me add a few things:

 * In OpenWrt we are trying to get rid of the downstream patches
   mentioned here. They were introduced at a time when we did not have
   Device Tree and hence no option to use kernel cmdline to attach UBI,
   or use root= parameter. Nowadays they are legacy and we should not
   use the various auto-rootfs hacks in favor of doing the same thing
   using /chosen/bootargs in DT and the like.

 * Shortly after the introduction of ubiblock we happily got rid of gluebi.
   In OpenWrt gluebi has not been enabled for a very long time, and
   hence I can tell little about potential problems it may cause, or at
   least I can't do more than anyone else can which is reading the code
   and the logs supplied by the user.

All that being said there are of course cases where one may simply
require to use a very old kernel (4.14 here) in order to be able to run
proprietary out-of-tree drivers (rt2860 wifi here). And, of course, if
you can compile a kernel with gluebi and ubiblock both enabled, then
that should work as well and not result in either of them going south.

So reading the logs I do understand what has happened:
Note that the error for unknown-block(31,12) is ENXIO ("No such device
or address"), and that makes sense as that mtdblock12 device indeed
doesn't exist:

> [    6.457518] 1f0b            4608 mtdblock11
> [    6.457523]  (driver?)
> [    6.470720] fe00           33604 ubiblock0_0
> [    6.470724]  (driver?)

And yes, that's due to the added tests for mtd->type != MTD_UBIVOLUME
which prevent mtdblock from being created for gluebi devices.

I understand that the board depends on OpenWrt's patches, as back in the
days of Linux 4.14 MT7621 was still using platform C code for each board
and there was no way to set, append or replace bootargs from DT, simply
because there isn't DT.

As at the time also ubiblock wasn't used and also most likely won't work
with that semi-proprietary firmware based Linux 4.14 which expects JFFS2
to be used, my suggestion is to disabled CONFIG_FTL (if it isn't already
disabled anyway) and apply this patch (which should be further
discussed) in order to fix commit ("mtd: Fix gluebi NULL pointer
dereference caused by ftl notifier"):

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 3caa0717d46c..3ef57dd56288 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -461,7 +461,7 @@ static void blktrans_notify_add(struct mtd_info *mtd)
 {
 	struct mtd_blktrans_ops *tr;
 
-	if (mtd->type == MTD_ABSENT || mtd->type == MTD_UBIVOLUME)
+	if (mtd->type == MTD_ABSENT || (IS_ENABLED(CONFIG_FTL) && mtd->type == MTD_UBIVOLUME))
 		return;
 
 	list_for_each_entry(tr, &blktrans_majors, list)
@@ -501,7 +501,7 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
 	mutex_lock(&mtd_table_mutex);
 	list_add(&tr->list, &blktrans_majors);
 	mtd_for_each_device(mtd)
-		if (mtd->type != MTD_ABSENT && mtd->type != MTD_UBIVOLUME)
+		if (mtd->type != MTD_ABSENT && (!IS_ENABLED(CONFIG_FTL || mtd->type != MTD_UBIVOLUME)))
 			tr->add_mtd(tr, mtd);
 	mutex_unlock(&mtd_table_mutex);
 	return 0;


Let me know if that helps and lets discuss if it could be considered for
being applied in upstream Linux.

> 
> > 
> >> 
> >>>> (31, 12) would be mtdblock12.
> >>>> How does your kernel know that mtdblock12 shall be the rootfs?
> >>> 
> >>> this is an openwrt approach: https://openwrt.org/docs/techref/filesystems (under
> >>> “technical details”, third paragraph)
> >>> 
> >>> essentially there’s a feature they add to the kernel (via patch) where you can
> >>> enable a feature that sets the root device based on the name of the partition.
> >> 
> >> So, this is all not mainline. :-/
> > 
> > i did say openwrt at the start, and i think that’s pretty close to mainline as
> > it gets.
> > 
> > sometimes these patches aren’t appropriate to push upstream. this one is not the
> > one causing the issue.
> > 
> > it seems to me that there is a problem with GLUEBI+BLOCK playing together.
> > 
> > as far as i can see, the setting of the device is being doing by mtdcore.c
> > 
> > it’s just not working with gluebi and block are enabled, and i need to know
> > whether disabling gluebi will allow it to work.
> > 
> > in other words, is it possible for gluebi to use the partition created by
> > ubi_block, and add the MTD_UBIVOLUME flag?
> 
> No. UBIBlock works on top of UBI volumes and creates a block device.
> 
> We'll sort this out. :)
> 
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Powered by blists - more mailing lists