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

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

[ZhaoLong Wang]

If both flt.ko and gluebi.ko are loaded, the notiier of ftl
triggers NULL pointer dereference when trying to access
‘gluebi->desc’ in gluebi_read().

ubi_gluebi_init
  ubi_register_volume_notifier
    ubi_enumerate_volumes
      ubi_notify_all
        gluebi_notify    nb->notifier_call()
          gluebi_create
            mtd_device_register
              mtd_device_parse_register
                add_mtd_device
                  blktrans_notify_add   not->add()
                    ftl_add_mtd         tr->add_mtd()
                      scan_header
                        mtd_read
                          mtd_read
                            mtd_read_oob
                              gluebi_read   mtd->read()
                                gluebi->desc - NULL

Detailed reproduction information available at the link[1],

In the normal case, obtain gluebi->desc in the gluebi_get_device(),
and accesses gluebi->desc in the gluebi_read(). However,
gluebi_get_device() is not executed in advance in the
ftl_add_mtd() process, which leads to NULL pointer dereference.

The value of gluebi->desc may also be a negative error code, which
triggers the page fault error.

This patch has the following modifications:

1. Do not assign gluebi->desc to the error code. Use the NULL instead.

2. Always check the validity of gluebi->desc in gluebi_read() If the
   gluebi->desc is NULL, try to get MTD device.

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.

Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
---
 drivers/mtd/ubi/gluebi.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
index 1b980d15d9fb..0ca7f104adbf 100644
--- a/drivers/mtd/ubi/gluebi.c
+++ b/drivers/mtd/ubi/gluebi.c
@@ -85,6 +85,7 @@ static int gluebi_get_device(struct mtd_info *mtd)
 {
 	struct gluebi_device *gluebi;
 	int ubi_mode = UBI_READONLY;
+	struct ubi_volume_desc *vdesc;
 
 	if (mtd->flags & MTD_WRITEABLE)
 		ubi_mode = UBI_READWRITE;
@@ -109,12 +110,14 @@ static int gluebi_get_device(struct mtd_info *mtd)
 	 * This is the first reference to this UBI volume via the MTD device
 	 * interface. Open the corresponding volume in read-write mode.
 	 */
-	gluebi->desc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
+	vdesc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
 				       ubi_mode);
-	if (IS_ERR(gluebi->desc)) {
+	if (IS_ERR(vdesc)) {
+		gluebi->desc = NULL;
 		mutex_unlock(&devices_mutex);
-		return PTR_ERR(gluebi->desc);
+		return PTR_ERR(vdesc);
 	}
+	gluebi->desc = vdesc;
 	gluebi->refcnt += 1;
 	mutex_unlock(&devices_mutex);
 	return 0;
@@ -134,8 +137,10 @@ static void gluebi_put_device(struct mtd_info *mtd)
 	gluebi = container_of(mtd, struct gluebi_device, mtd);
 	mutex_lock(&devices_mutex);
 	gluebi->refcnt -= 1;
-	if (gluebi->refcnt == 0)
+	if (gluebi->refcnt == 0) {
 		ubi_close_volume(gluebi->desc);
+		gluebi->desc = NULL;
+	}
 	mutex_unlock(&devices_mutex);
 }
 
@@ -154,9 +159,26 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
 		       size_t *retlen, unsigned char *buf)
 {
 	int err = 0, lnum, offs, bytes_left;
-	struct gluebi_device *gluebi;
+	struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device,
+						    mtd);
+	int no_desc = gluebi->desc == NULL ? 1 : 0;
+
+	/**
+	 * In normal case, the UBI volume desc has been initialized by
+	 * ->_get_device(). However, in the ftl notifier process, the
+	 * ->_get_device() is not executed in advance and the MTD device
+	 * is directly scanned which cause NULL pointer dereference.
+	 * Therefore, try to get the MTD device here.
+	 */
+	if (unlikely(no_desc)) {
+		err = __get_mtd_device(mtd);
+		if (err) {
+			err_msg("cannot get MTD device %d, UBI device %d, volume %d, error %d",
+				mtd->index, gluebi->ubi_num, gluebi->vol_id, err);
+			return err;
+		}
+	}
 
-	gluebi = container_of(mtd, struct gluebi_device, mtd);
 	lnum = div_u64_rem(from, mtd->erasesize, &offs);
 	bytes_left = len;
 	while (bytes_left) {
@@ -176,6 +198,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 
 	*retlen = len - bytes_left;
+
+	if (unlikely(no_desc))
+		__put_mtd_device(mtd);
 	return err;
 }
 
-- 
2.31.1

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

[Zhihao Cheng]

在 2023/10/18 20:16, ZhaoLong Wang 写道:
> If both flt.ko and gluebi.ko are loaded, the notiier of ftl
> triggers NULL pointer dereference when trying to access
> ‘gluebi->desc’ in gluebi_read().
> 
> ubi_gluebi_init
>    ubi_register_volume_notifier
>      ubi_enumerate_volumes
>        ubi_notify_all
>          gluebi_notify    nb->notifier_call()
>            gluebi_create
>              mtd_device_register
>                mtd_device_parse_register
>                  add_mtd_device
>                    blktrans_notify_add   not->add()
>                      ftl_add_mtd         tr->add_mtd()
>                        scan_header
>                          mtd_read
>                            mtd_read
>                              mtd_read_oob
>                                gluebi_read   mtd->read()
>                                  gluebi->desc - NULL
> 
> Detailed reproduction information available at the link[1],
> 
> In the normal case, obtain gluebi->desc in the gluebi_get_device(),
> and accesses gluebi->desc in the gluebi_read(). However,
> gluebi_get_device() is not executed in advance in the
> ftl_add_mtd() process, which leads to NULL pointer dereference.
> 
> The value of gluebi->desc may also be a negative error code, which
> triggers the page fault error.
> 
> This patch has the following modifications:
> 
> 1. Do not assign gluebi->desc to the error code. Use the NULL instead.
> 
> 2. Always check the validity of gluebi->desc in gluebi_read() If the
>     gluebi->desc is NULL, try to get MTD device.
> 
> 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.
> 
> Fixes: 2ba3d76a1e29 ("UBI: make gluebi a separate module")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217992 [1]
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
> ---
>   drivers/mtd/ubi/gluebi.c | 37 +++++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
> 

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>

> diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c
> index 1b980d15d9fb..0ca7f104adbf 100644
> --- a/drivers/mtd/ubi/gluebi.c
> +++ b/drivers/mtd/ubi/gluebi.c
> @@ -85,6 +85,7 @@ static int gluebi_get_device(struct mtd_info *mtd)
>   {
>   	struct gluebi_device *gluebi;
>   	int ubi_mode = UBI_READONLY;
> +	struct ubi_volume_desc *vdesc;
>   
>   	if (mtd->flags & MTD_WRITEABLE)
>   		ubi_mode = UBI_READWRITE;
> @@ -109,12 +110,14 @@ static int gluebi_get_device(struct mtd_info *mtd)
>   	 * This is the first reference to this UBI volume via the MTD device
>   	 * interface. Open the corresponding volume in read-write mode.
>   	 */
> -	gluebi->desc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
> +	vdesc = ubi_open_volume(gluebi->ubi_num, gluebi->vol_id,
>   				       ubi_mode);
> -	if (IS_ERR(gluebi->desc)) {
> +	if (IS_ERR(vdesc)) {
> +		gluebi->desc = NULL;
>   		mutex_unlock(&devices_mutex);
> -		return PTR_ERR(gluebi->desc);
> +		return PTR_ERR(vdesc);
>   	}
> +	gluebi->desc = vdesc;
>   	gluebi->refcnt += 1;
>   	mutex_unlock(&devices_mutex);
>   	return 0;
> @@ -134,8 +137,10 @@ static void gluebi_put_device(struct mtd_info *mtd)
>   	gluebi = container_of(mtd, struct gluebi_device, mtd);
>   	mutex_lock(&devices_mutex);
>   	gluebi->refcnt -= 1;
> -	if (gluebi->refcnt == 0)
> +	if (gluebi->refcnt == 0) {
>   		ubi_close_volume(gluebi->desc);
> +		gluebi->desc = NULL;
> +	}
>   	mutex_unlock(&devices_mutex);
>   }
>   
> @@ -154,9 +159,26 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
>   		       size_t *retlen, unsigned char *buf)
>   {
>   	int err = 0, lnum, offs, bytes_left;
> -	struct gluebi_device *gluebi;
> +	struct gluebi_device *gluebi = container_of(mtd, struct gluebi_device,
> +						    mtd);
> +	int no_desc = gluebi->desc == NULL ? 1 : 0;
> +
> +	/**
> +	 * In normal case, the UBI volume desc has been initialized by
> +	 * ->_get_device(). However, in the ftl notifier process, the
> +	 * ->_get_device() is not executed in advance and the MTD device
> +	 * is directly scanned which cause NULL pointer dereference.
> +	 * Therefore, try to get the MTD device here.
> +	 */
> +	if (unlikely(no_desc)) {
> +		err = __get_mtd_device(mtd);
> +		if (err) {
> +			err_msg("cannot get MTD device %d, UBI device %d, volume %d, error %d",
> +				mtd->index, gluebi->ubi_num, gluebi->vol_id, err);
> +			return err;
> +		}
> +	}
>   
> -	gluebi = container_of(mtd, struct gluebi_device, mtd);
>   	lnum = div_u64_rem(from, mtd->erasesize, &offs);
>   	bytes_left = len;
>   	while (bytes_left) {
> @@ -176,6 +198,9 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	}
>   
>   	*retlen = len - bytes_left;
> +
> +	if (unlikely(no_desc))
> +		__put_mtd_device(mtd);
>   	return err;
>   }
>   
> 

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

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com>
> An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>,
> dpervushin@embeddedalley.com, "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com>
> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "chengzhihao1"
> <chengzhihao1@huawei.com>, "ZhaoLong Wang" <wangzhaolong1@huawei.com>, "yi zhang" <yi.zhang@huawei.com>, "yangerkun"
> <yangerkun@huawei.com>
> Gesendet: Mittwoch, 18. Oktober 2023 14:16:18
> Betreff: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

> If both flt.ko and gluebi.ko are loaded, the notiier of ftl
> triggers NULL pointer dereference when trying to access
> ‘gluebi->desc’ in gluebi_read().
> 
> ubi_gluebi_init
>  ubi_register_volume_notifier
>    ubi_enumerate_volumes
>      ubi_notify_all
>        gluebi_notify    nb->notifier_call()
>          gluebi_create
>            mtd_device_register
>              mtd_device_parse_register
>                add_mtd_device
>                  blktrans_notify_add   not->add()
>                    ftl_add_mtd         tr->add_mtd()
>                      scan_header
>                        mtd_read
>                          mtd_read
>                            mtd_read_oob
>                              gluebi_read   mtd->read()
>                                gluebi->desc - NULL
> 
> Detailed reproduction information available at the link[1],
> 
> In the normal case, obtain gluebi->desc in the gluebi_get_device(),
> and accesses gluebi->desc in the gluebi_read(). However,
> gluebi_get_device() is not executed in advance in the
> ftl_add_mtd() process, which leads to NULL pointer dereference.
> 
> The value of gluebi->desc may also be a negative error code, which
> triggers the page fault error.
> 
> This patch has the following modifications:
> 
> 1. Do not assign gluebi->desc to the error code. Use the NULL instead.
> 
> 2. Always check the validity of gluebi->desc in gluebi_read() If the
>   gluebi->desc is NULL, try to get MTD device.
> 
> 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.

Is there a strong reason why have to defer ubi_open_volume() to
gluebi_get_device()?

Miquel, what do you think?

Thanks,
//richard

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

[Zhihao Cheng]

在 2023/10/20 4:27, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----
>> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com>
>> An: "richard" <richard@nod.at>, "Miquel Raynal" <miquel.raynal@bootlin.com>, "Vignesh Raghavendra" <vigneshr@ti.com>,
>> dpervushin@embeddedalley.com, "Artem Bityutskiy" <Artem.Bityutskiy@nokia.com>
>> CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org>, "chengzhihao1"
>> <chengzhihao1@huawei.com>, "ZhaoLong Wang" <wangzhaolong1@huawei.com>, "yi zhang" <yi.zhang@huawei.com>, "yangerkun"
>> <yangerkun@huawei.com>
>> Gesendet: Mittwoch, 18. Oktober 2023 14:16:18
>> Betreff: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier
> 
>> If both flt.ko and gluebi.ko are loaded, the notiier of ftl
>> triggers NULL pointer dereference when trying to access
>> ‘gluebi->desc’ in gluebi_read().
>>
>> ubi_gluebi_init
>>   ubi_register_volume_notifier
>>     ubi_enumerate_volumes
>>       ubi_notify_all
>>         gluebi_notify    nb->notifier_call()
>>           gluebi_create
>>             mtd_device_register
>>               mtd_device_parse_register
>>                 add_mtd_device
>>                   blktrans_notify_add   not->add()
>>                     ftl_add_mtd         tr->add_mtd()
>>                       scan_header
>>                         mtd_read
>>                           mtd_read
>>                             mtd_read_oob
>>                               gluebi_read   mtd->read()
>>                                 gluebi->desc - NULL
>>
>> Detailed reproduction information available at the link[1],
>>
>> In the normal case, obtain gluebi->desc in the gluebi_get_device(),
>> and accesses gluebi->desc in the gluebi_read(). However,
>> gluebi_get_device() is not executed in advance in the
>> ftl_add_mtd() process, which leads to NULL pointer dereference.
>>
>> The value of gluebi->desc may also be a negative error code, which
>> triggers the page fault error.
>>
>> This patch has the following modifications:
>>
>> 1. Do not assign gluebi->desc to the error code. Use the NULL instead.
>>
>> 2. Always check the validity of gluebi->desc in gluebi_read() If the
>>    gluebi->desc is NULL, try to get MTD device.
>>
>> 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.

> 
> Is there a strong reason why have to defer ubi_open_volume() to
> gluebi_get_device()?
> 
> Miquel, what do you think?
> 
> Thanks,
> //richard
> 
> .
> 

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

[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.
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

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

[ZhaoLong Wang]

> 
> IOW, no mtdblock (hence, also no FTLs) on top of gluebi.
> 
> What do you guys think?
> 
> Thanks,
> //richard
> 


JFFS2 needs to work on the block device, so the mtdblock needs to work
on the top of gluebi.

This is directly reflected in the jffs2 mount command, such as

  # mount -t jffs2 /dev/mtdblockX /mnt/jffs2

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

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com>
> An: "richard" <richard@nod.at>, "chengzhihao1" <chengzhihao1@huawei.com>
> CC: "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>
> Gesendet: Montag, 23. Oktober 2023 08:41:12
> Betreff: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier

>> 
>> IOW, no mtdblock (hence, also no FTLs) on top of gluebi.
>> 
>> What do you guys think?
>> 
>> Thanks,
>> //richard
>> 
> 
> 
> JFFS2 needs to work on the block device, so the mtdblock needs to work
> on the top of gluebi.
> 
> This is directly reflected in the jffs2 mount command, such as
> 
>   # mount -t jffs2 /dev/mtdblockX /mnt/jffs2

The limitation is long gone. It comes from the dark and old times
where Linux was only able to mount block devices.

You can do: mount -t jffs2 mtdX /mnt/jffs2

Thanks,
//richard

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

[Zhihao Cheng]

在 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
> 
> .
> 

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

[ZhaoLong Wang]

> ----- Ursprüngliche Mail -----
>> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com>
>> An: "richard" <richard@nod.at>, "chengzhihao1" <chengzhihao1@huawei.com>
>> CC: "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>
>> Gesendet: Montag, 23. Oktober 2023 08:41:12
>> Betreff: Re: [PATCH v2] ubi: gluebi: Fix NULL pointer dereference caused by ftl notifier
> 
>>>
>>> IOW, no mtdblock (hence, also no FTLs) on top of gluebi.
>>>
>>> What do you guys think?
>>>
>>> Thanks,
>>> //richard
>>>
>>
>>
>> JFFS2 needs to work on the block device, so the mtdblock needs to work
>> on the top of gluebi.
>>
>> This is directly reflected in the jffs2 mount command, such as
>>
>>    # mount -t jffs2 /dev/mtdblockX /mnt/jffs2
> 
> The limitation is long gone. It comes from the dark and old times
> where Linux was only able to mount block devices.
> 
> You can do: mount -t jffs2 mtdX /mnt/jffs2
> 
> Thanks,
> //richard
> 

Yes, I tried it and it worked. Thanks for the reminder.

But this modification rejects this usage. and rejects other block device
file systems (such as ext4) from working on gluebi.

Thank you.
Zhaolong

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

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
>>> 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?

Unless they want to kill their NAND immediately, no.
UBIblock hat initially an RW-Mode but it was rejected because
UBI is not an FTL and will all dangerous setups.

What I'm trying to say is, I'd like to avoid adding complicated solutions
or workarounds just to make artificial stacking of outdated MTD components
possible.
Let's keep the big picture in mind.

Thanks,
//richard

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

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "ZhaoLong Wang" <wangzhaolong1@huawei.com>
> But this modification rejects this usage. and rejects other block device
> file systems (such as ext4) from working on gluebi.

Are you using ext4 on top of gluebi?
Please explain in detail why.

Thanks,
//richard

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

[Zhihao Cheng]

在 2023/10/23 15:15, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----
>> Von: "chengzhihao1" <chengzhihao1@huawei.com>
>>>> 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?
> Unless they want to kill their NAND immediately, no.
> UBIblock hat initially an RW-Mode but it was rejected because
> UBI is not an FTL and will all dangerous setups.
>
> What I'm trying to say is, I'd like to avoid adding complicated solutions
> or workarounds just to make artificial stacking of outdated MTD components
> possible.
> Let's keep the big picture in mind.

Makes sense. Zhaolong and I didn't make clear the boundary between FTL 
and ubiblock/gluebi. Now, I believe that UBI needn't be responsible much 
for the extension mechanism(eg. mtdblock based on gluebi). So, let's 
pick your solution.

> Thanks,
> //richard
>
> .