Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

From: Richard Weinberger <richard@nod.at>
To: chengzhihao1 <chengzhihao1@huawei.com>
Cc: ZhaoLong Wang <wangzhaolong1@huawei.com>,
	 Miquel Raynal <miquel.raynal@bootlin.com>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 dpervushin <dpervushin@embeddedalley.com>,
	 Artem Bityutskiy <Artem.Bityutskiy@nokia.com>,
	 linux-mtd <linux-mtd@lists.infradead.org>,
	 linux-kernel <linux-kernel@vger.kernel.org>,
	 yi zhang <yi.zhang@huawei.com>, yangerkun <yangerkun@huawei.com>
Subject: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier
Date: Sat, 21 Oct 2023 18:09:40 +0200 (CEST)	[thread overview]
Message-ID: <441107100.23734.1697904580252.JavaMail.zimbra@nod.at> (raw)
In-Reply-To: <891e554b-c133-6378-3a65-836fc9147e54@huawei.com>

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
>>> Such a modification currently works because the mutex "mtd_table_mutex"
>>> is held on all necessary paths, including the ftl_add_mtd() call path,
>>> open and close paths. Therefore, many race condition can be avoided.
>> 
>> I see the problem, but I'm not really satisfied by the solution.
>> Adding this hack to gluebi_read() is not nice at all.
> 
> Yes, it's jsut a workaround. At the begining, I prefer that increasing
> volume refcnt (by ubi_open_volume) in gluebi_create and releasing volume
> refcnt in gluebi_remove. It looks more reasonable that holding a refcnt
> of UBI volume when gluebi is alive. After looking through the code, the
> creation/destroying of gluebi is triggered by volume
> actions(UBI_VOLUME_ADDED/UBI_VOLUME_REMOVED), which means that:
> 1. gluebi_remove is depended on UBI_VOLUME_REMOVED(triggered by
> ubi_remove_volume)
> 2. ubi_remove_volume won't be executed until the refcnt of volume
> becomes 0(released by gluebi_remove)
> 
> If we add new ioctls to control creation/destroying of gluebi, then
> gluebi mtd won't be automatically created when UBI volume is added. I'm
> not certain whether this change will effect existing startup process
> that depends on gluebi.

Let's take a stack back. The sole purpose of gluebi is providing
a way to run JFFS2 on top of UBI.
IMHO there is no need to run an FTL on top of UBI or even mtdblock.
This kind of stacking does not make sense.

So, I'd go so far and propose the following:

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index ff18636e08897..b362a64411ebd 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -463,7 +463,7 @@ static void blktrans_notify_add(struct mtd_info *mtd)
 {
        struct mtd_blktrans_ops *tr;
 
-       if (mtd->type == MTD_ABSENT)
+       if (mtd->type == MTD_ABSENT || mtd->type == MTD_UBIVOLUME)
                return;
 
        list_for_each_entry(tr, &blktrans_majors, list)

IOW, no mtdblock (hence, also no FTLs) on top of gluebi.

What do you guys think?

Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
