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

From: Zhihao Cheng <chengzhihao1@huawei.com>
To: Richard Weinberger <richard@nod.at>
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: Mon, 23 Oct 2023 15:09:26 +0800	[thread overview]
Message-ID: <93b2d5ab-e36c-be08-7343-ef4ca16f991d@huawei.com> (raw)
In-Reply-To: <441107100.23734.1697904580252.JavaMail.zimbra@nod.at>

在 2023/10/22 0:09, Richard Weinberger 写道:
> ----- 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.

Is it possible that someone runs ext4 on mtdblock based on gluebi, for 
the advantage of wear-leveling?

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