[PATCH v2 0/4] ubi: Fix serious of resource leaking problems

[PATCH v2 0/4] ubi: Fix serious of resource leaking problems

[Zhihao Cheng]

v1->v2:
  Combine Ben[1] and my[2] patches.
  Modify second patch in [1], don't drop warning message if debugfs
  failed to be initialized.
  Modify second patch in [2], move ubiblock_init to the end of ubi_init.
  Add new patch to replace IS_ERR_OR_NULL with IS_ERR.

[1] https://lore.kernel.org/linux-mtd/cover.1712788087.git.ben.hutchings@mind.be/
[2] https://lore.kernel.org/lkml/20240410074033.2523399-3-chengzhihao1@huawei.com/T/

Ben Hutchings (1):
  mtd: ubi: Ignore all debugfs initialisation failures

Zhihao Cheng (3):
  ubi: ubi_init: Fix missed debugfs cleanup in error handling path
  ubi: ubi_init: Fix missed ubiblock cleanup in error handling path
  ubi: debugfs: Replace IS_ERR_OR_NULL with IS_ERR in error checking
    path

 drivers/mtd/ubi/build.c | 61 ++++++++++++++++++++---------------------
 drivers/mtd/ubi/debug.c | 35 +++++++++--------------
 drivers/mtd/ubi/debug.h |  4 +--
 3 files changed, 44 insertions(+), 56 deletions(-)

-- 
2.39.2

[PATCH v2 1/4] ubi: ubi_init: Fix missed debugfs cleanup in error handling path

[Zhihao Cheng]

The debugfs dir is created in ubi_init, but it is not destroyed in error
handling path of ubi_init when ubi is loaded by inserting module (eg.
attaching failure), which leads to subsequent ubi_init calls failed.
Fix it by destroying debugfs dir in corresponding error handling path.

Fixes: 927c145208b0 ("mtd: ubi: attach from device tree")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/mtd/ubi/build.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index a7e3a6246c0e..7f95fd7968a8 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1372,7 +1372,7 @@ static int __init ubi_init(void)
 
 		/* See comment above re-ubi_is_module(). */
 		if (ubi_is_module())
-			goto out_slab;
+			goto out_debugfs;
 	}
 
 	register_mtd_user(&ubi_mtd_notifier);
@@ -1387,6 +1387,8 @@ static int __init ubi_init(void)
 
 out_mtd_notifier:
 	unregister_mtd_user(&ubi_mtd_notifier);
+out_debugfs:
+	ubi_debugfs_exit();
 out_slab:
 	kmem_cache_destroy(ubi_wl_entry_slab);
 out_dev_unreg:
-- 
2.39.2

[PATCH v2 2/4] ubi: ubi_init: Fix missed ubiblock cleanup in error handling path

[Zhihao Cheng]

The ubiblock_init called by ubi_init will register device number, but
device number is not released in error handling path of ubi_init when
ubi is loaded by inserting module (eg. attaching failure), which leads
to subsequent ubi_init calls failed by running out of device number
(dmesg shows that "__register_blkdev: failed to get major for ubiblock").
Since ubiblock_init() registers notifier and invokes notification
functions, so we can move it after ubi_init_attach() to fix the problem.

Fixes: 927c145208b0 ("mtd: ubi: attach from device tree")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/mtd/ubi/build.c | 51 +++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 7f95fd7968a8..bc63fbf5e947 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1263,9 +1263,21 @@ static struct mtd_notifier ubi_mtd_notifier = {
 	.remove = ubi_notify_remove,
 };
 
+static void detach_mtd_devs(int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++)
+		if (ubi_devices[i]) {
+			mutex_lock(&ubi_devices_mutex);
+			ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
+			mutex_unlock(&ubi_devices_mutex);
+		}
+}
+
 static int __init ubi_init_attach(void)
 {
-	int err, i, k;
+	int err, i;
 
 	/* Attach MTD devices */
 	for (i = 0; i < mtd_devs; i++) {
@@ -1317,12 +1329,7 @@ static int __init ubi_init_attach(void)
 	return 0;
 
 out_detach:
-	for (k = 0; k < i; k++)
-		if (ubi_devices[k]) {
-			mutex_lock(&ubi_devices_mutex);
-			ubi_detach_mtd_dev(ubi_devices[k]->ubi_num, 1);
-			mutex_unlock(&ubi_devices_mutex);
-		}
+	detach_mtd_devs(i);
 	return err;
 }
 #ifndef CONFIG_MTD_UBI_MODULE
@@ -1366,15 +1373,6 @@ static int __init ubi_init(void)
 	if (err)
 		goto out_slab;
 
-	err = ubiblock_init();
-	if (err) {
-		pr_err("UBI error: block: cannot initialize, error %d\n", err);
-
-		/* See comment above re-ubi_is_module(). */
-		if (ubi_is_module())
-			goto out_debugfs;
-	}
-
 	register_mtd_user(&ubi_mtd_notifier);
 
 	if (ubi_is_module()) {
@@ -1383,11 +1381,21 @@ static int __init ubi_init(void)
 			goto out_mtd_notifier;
 	}
 
+	err = ubiblock_init();
+	if (err) {
+		pr_err("UBI error: block: cannot initialize, error %d\n", err);
+
+		/* See comment above re-ubi_is_module(). */
+		if (ubi_is_module())
+			goto out_detach;
+	}
+
 	return 0;
 
+out_detach:
+	detach_mtd_devs(mtd_devs);
 out_mtd_notifier:
 	unregister_mtd_user(&ubi_mtd_notifier);
-out_debugfs:
 	ubi_debugfs_exit();
 out_slab:
 	kmem_cache_destroy(ubi_wl_entry_slab);
@@ -1403,17 +1411,10 @@ device_initcall(ubi_init);
 
 static void __exit ubi_exit(void)
 {
-	int i;
-
 	ubiblock_exit();
 	unregister_mtd_user(&ubi_mtd_notifier);
 
-	for (i = 0; i < UBI_MAX_DEVICES; i++)
-		if (ubi_devices[i]) {
-			mutex_lock(&ubi_devices_mutex);
-			ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
-			mutex_unlock(&ubi_devices_mutex);
-		}
+	detach_mtd_devs(UBI_MAX_DEVICES);
 	ubi_debugfs_exit();
 	kmem_cache_destroy(ubi_wl_entry_slab);
 	misc_deregister(&ubi_ctrl_cdev);
-- 
2.39.2

[PATCH v2 3/4] mtd: ubi: Ignore all debugfs initialisation failures

[Zhihao Cheng]

From: Ben Hutchings <ben.hutchings@mind.be>

Drivers should never stop initialising or probing because of a failure
to create debugfs entries.  Such failures are harmless and could be
the intended result of setting the kernel parameter debugfs=off.

This was partially fixed in 2019, but some error checks were missed
and another one was been added since.

Signed-off-by: Ben Hutchings <ben.hutchings@mind.be>
Fixes: 2a734bb8d502 ("UBI: use debugfs for the extra checks knobs")
Fixes: c2d73ba892ea ("mtd: no need to check return value of ...")
Fixes: 6931fb44858c ("ubi: Use the fault injection framework to enhance ...")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/mtd/ubi/build.c | 10 ++--------
 drivers/mtd/ubi/debug.c | 25 ++++++++++---------------
 drivers/mtd/ubi/debug.h |  4 ++--
 3 files changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index bc63fbf5e947..6d8212a51fec 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1018,9 +1018,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	if (err)
 		goto out_detach;
 
-	err = ubi_debugfs_init_dev(ubi);
-	if (err)
-		goto out_uif;
+	ubi_debugfs_init_dev(ubi);
 
 	ubi->bgt_thread = kthread_create(ubi_thread, ubi, "%s", ubi->bgt_name);
 	if (IS_ERR(ubi->bgt_thread)) {
@@ -1064,7 +1062,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 
 out_debugfs:
 	ubi_debugfs_exit_dev(ubi);
-out_uif:
 	uif_close(ubi);
 out_detach:
 	ubi_wl_close(ubi);
@@ -1369,9 +1366,7 @@ static int __init ubi_init(void)
 		goto out_dev_unreg;
 	}
 
-	err = ubi_debugfs_init();
-	if (err)
-		goto out_slab;
+	ubi_debugfs_init();
 
 	register_mtd_user(&ubi_mtd_notifier);
 
@@ -1397,7 +1392,6 @@ static int __init ubi_init(void)
 out_mtd_notifier:
 	unregister_mtd_user(&ubi_mtd_notifier);
 	ubi_debugfs_exit();
-out_slab:
 	kmem_cache_destroy(ubi_wl_entry_slab);
 out_dev_unreg:
 	misc_deregister(&ubi_ctrl_cdev);
diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index d57f52bd2ff3..989744eb85a5 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -291,28 +291,25 @@ static void dfs_create_fault_entry(struct dentry *parent)
 /**
  * ubi_debugfs_init - create UBI debugfs directory.
  *
- * Create UBI debugfs directory. Returns zero in case of success and a negative
- * error code in case of failure.
+ * Try to create UBI debugfs directory.
  */
-int ubi_debugfs_init(void)
+void ubi_debugfs_init(void)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
-		return 0;
+		return;
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
 	if (IS_ERR_OR_NULL(dfs_rootdir)) {
 		int err = dfs_rootdir ? PTR_ERR(dfs_rootdir) : -ENODEV;
 
-		pr_err("UBI error: cannot create \"ubi\" debugfs directory, error %d\n",
-		       err);
-		return err;
+		pr_warn("UBI error: cannot create \"ubi\" debugfs directory, error %d\n",
+			err);
+		return;
 	}
 
 #ifdef CONFIG_MTD_UBI_FAULT_INJECTION
 	dfs_create_fault_entry(dfs_rootdir);
 #endif
-
-	return 0;
 }
 
 /**
@@ -585,10 +582,9 @@ static const struct file_operations eraseblk_count_fops = {
  * ubi_debugfs_init_dev - initialize debugfs for an UBI device.
  * @ubi: UBI device description object
  *
- * This function creates all debugfs files for UBI device @ubi. Returns zero in
- * case of success and a negative error code in case of failure.
+ * This function tries to create all debugfs files for UBI device @ubi.
  */
-int ubi_debugfs_init_dev(struct ubi_device *ubi)
+void ubi_debugfs_init_dev(struct ubi_device *ubi)
 {
 	unsigned long ubi_num = ubi->ubi_num;
 	struct ubi_debug_info *d = &ubi->dbg;
@@ -596,13 +592,13 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
 	int n;
 
 	if (!IS_ENABLED(CONFIG_DEBUG_FS))
-		return 0;
+		return;
 
 	n = snprintf(d->dfs_dir_name, UBI_DFS_DIR_LEN + 1, UBI_DFS_DIR_NAME,
 		     ubi->ubi_num);
 	if (n > UBI_DFS_DIR_LEN) {
 		/* The array size is too small */
-		return -EINVAL;
+		return;
 	}
 
 	d->dfs_dir = debugfs_create_dir(d->dfs_dir_name, dfs_rootdir);
@@ -653,7 +649,6 @@ int ubi_debugfs_init_dev(struct ubi_device *ubi)
 						       (void *)ubi_num,
 						       &dfs_fops);
 #endif
-	return 0;
 }
 
 /**
diff --git a/drivers/mtd/ubi/debug.h b/drivers/mtd/ubi/debug.h
index b2fd97548808..1dc430d3f482 100644
--- a/drivers/mtd/ubi/debug.h
+++ b/drivers/mtd/ubi/debug.h
@@ -47,9 +47,9 @@ void ubi_dump_aeb(const struct ubi_ainf_peb *aeb, int type);
 void ubi_dump_mkvol_req(const struct ubi_mkvol_req *req);
 int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset,
 			  int len);
-int ubi_debugfs_init(void);
+void ubi_debugfs_init(void);
 void ubi_debugfs_exit(void);
-int ubi_debugfs_init_dev(struct ubi_device *ubi);
+void ubi_debugfs_init_dev(struct ubi_device *ubi);
 void ubi_debugfs_exit_dev(struct ubi_device *ubi);
 
 /**
-- 
2.39.2

[PATCH v2 4/4] ubi: debugfs: Replace IS_ERR_OR_NULL with IS_ERR in error checking path

[Zhihao Cheng]

Function debugfs_create_dir will return error pointer rather than NULL
if it fails, replace IS_ERR_OR_NULL with IS_ERR just like other places
(eg. fault_create_debugfs_attr).

Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 drivers/mtd/ubi/debug.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/ubi/debug.c b/drivers/mtd/ubi/debug.c
index 989744eb85a5..3bcdf4e70a0a 100644
--- a/drivers/mtd/ubi/debug.c
+++ b/drivers/mtd/ubi/debug.c
@@ -248,11 +248,9 @@ static void dfs_create_fault_entry(struct dentry *parent)
 	struct dentry *dir;
 
 	dir = debugfs_create_dir("fault_inject", parent);
-	if (IS_ERR_OR_NULL(dir)) {
-		int err = dir ? PTR_ERR(dir) : -ENODEV;
-
+	if (IS_ERR(dir)) {
 		pr_warn("UBI error: cannot create \"fault_inject\" debugfs directory, error %d\n",
-			 err);
+			(int)PTR_ERR(dir));
 		return;
 	}
 
@@ -299,11 +297,9 @@ void ubi_debugfs_init(void)
 		return;
 
 	dfs_rootdir = debugfs_create_dir("ubi", NULL);
-	if (IS_ERR_OR_NULL(dfs_rootdir)) {
-		int err = dfs_rootdir ? PTR_ERR(dfs_rootdir) : -ENODEV;
-
+	if (IS_ERR(dfs_rootdir)) {
 		pr_warn("UBI error: cannot create \"ubi\" debugfs directory, error %d\n",
-			err);
+			(int)PTR_ERR(dfs_rootdir));
 		return;
 	}
 
-- 
2.39.2

Re: [PATCH v2 2/4] ubi: ubi_init: Fix missed ubiblock cleanup in error handling path

[Daniel Golle]

On Thu, Apr 11, 2024 at 11:19:01AM +0800, Zhihao Cheng wrote:
> The ubiblock_init called by ubi_init will register device number, but
> device number is not released in error handling path of ubi_init when
> ubi is loaded by inserting module (eg. attaching failure), which leads
> to subsequent ubi_init calls failed by running out of device number
> (dmesg shows that "__register_blkdev: failed to get major for ubiblock").
> Since ubiblock_init() registers notifier and invokes notification
> functions, so we can move it after ubi_init_attach() to fix the problem.
> 
> Fixes: 927c145208b0 ("mtd: ubi: attach from device tree")
> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> ---
>  drivers/mtd/ubi/build.c | 51 +++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 7f95fd7968a8..bc63fbf5e947 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1263,9 +1263,21 @@ static struct mtd_notifier ubi_mtd_notifier = {
>  	.remove = ubi_notify_remove,
>  };
>  
> +static void detach_mtd_devs(int count)

Missing __init to avoid section missmatch.

See also: https://lore.kernel.org/oe-kbuild-all/202404112327.158HJfAw-lkp@intel.com/

> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++)
> +		if (ubi_devices[i]) {
> +			mutex_lock(&ubi_devices_mutex);
> +			ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
> +			mutex_unlock(&ubi_devices_mutex);
> +		}
> +}
> +
>  static int __init ubi_init_attach(void)
>  {
> -	int err, i, k;
> +	int err, i;
>  
>  	/* Attach MTD devices */
>  	for (i = 0; i < mtd_devs; i++) {
> @@ -1317,12 +1329,7 @@ static int __init ubi_init_attach(void)
>  	return 0;
>  
>  out_detach:
> -	for (k = 0; k < i; k++)
> -		if (ubi_devices[k]) {
> -			mutex_lock(&ubi_devices_mutex);
> -			ubi_detach_mtd_dev(ubi_devices[k]->ubi_num, 1);
> -			mutex_unlock(&ubi_devices_mutex);
> -		}
> +	detach_mtd_devs(i);
>  	return err;
>  }
>  #ifndef CONFIG_MTD_UBI_MODULE
> @@ -1366,15 +1373,6 @@ static int __init ubi_init(void)
>  	if (err)
>  		goto out_slab;
>  
> -	err = ubiblock_init();
> -	if (err) {
> -		pr_err("UBI error: block: cannot initialize, error %d\n", err);
> -
> -		/* See comment above re-ubi_is_module(). */
> -		if (ubi_is_module())
> -			goto out_debugfs;
> -	}
> -
>  	register_mtd_user(&ubi_mtd_notifier);
>  
>  	if (ubi_is_module()) {
> @@ -1383,11 +1381,21 @@ static int __init ubi_init(void)
>  			goto out_mtd_notifier;
>  	}
>  
> +	err = ubiblock_init();
> +	if (err) {
> +		pr_err("UBI error: block: cannot initialize, error %d\n", err);
> +
> +		/* See comment above re-ubi_is_module(). */
> +		if (ubi_is_module())
> +			goto out_detach;
> +	}
> +
>  	return 0;
>  
> +out_detach:
> +	detach_mtd_devs(mtd_devs);
>  out_mtd_notifier:
>  	unregister_mtd_user(&ubi_mtd_notifier);
> -out_debugfs:
>  	ubi_debugfs_exit();
>  out_slab:
>  	kmem_cache_destroy(ubi_wl_entry_slab);
> @@ -1403,17 +1411,10 @@ device_initcall(ubi_init);
>  
>  static void __exit ubi_exit(void)
>  {
> -	int i;
> -
>  	ubiblock_exit();
>  	unregister_mtd_user(&ubi_mtd_notifier);
>  
> -	for (i = 0; i < UBI_MAX_DEVICES; i++)
> -		if (ubi_devices[i]) {
> -			mutex_lock(&ubi_devices_mutex);
> -			ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
> -			mutex_unlock(&ubi_devices_mutex);
> -		}
> +	detach_mtd_devs(UBI_MAX_DEVICES);
>  	ubi_debugfs_exit();
>  	kmem_cache_destroy(ubi_wl_entry_slab);
>  	misc_deregister(&ubi_ctrl_cdev);
> -- 
> 2.39.2
> 

Re: [PATCH v2 2/4] ubi: ubi_init: Fix missed ubiblock cleanup in error handling path

[Markus Elfring]

…
> Since ubiblock_init() registers notifier and invokes notification
> functions, so we can move it after ubi_init_attach() to fix the problem.

I find this change description improvable.
Would an imperative wording be also more desirable here?

Regards,
Markus

Re: [PATCH v2 2/4] ubi: ubi_init: Fix missed ubiblock cleanup in error handling path

[Zhihao Cheng]

在 2024/4/11 23:48, Daniel Golle 写道:
> On Thu, Apr 11, 2024 at 11:19:01AM +0800, Zhihao Cheng wrote:
>> The ubiblock_init called by ubi_init will register device number, but
>> device number is not released in error handling path of ubi_init when
>> ubi is loaded by inserting module (eg. attaching failure), which leads
>> to subsequent ubi_init calls failed by running out of device number
>> (dmesg shows that "__register_blkdev: failed to get major for ubiblock").
>> Since ubiblock_init() registers notifier and invokes notification
>> functions, so we can move it after ubi_init_attach() to fix the problem.
>>
>> Fixes: 927c145208b0 ("mtd: ubi: attach from device tree")
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>> ---
>>   drivers/mtd/ubi/build.c | 51 +++++++++++++++++++++--------------------
>>   1 file changed, 26 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
>> index 7f95fd7968a8..bc63fbf5e947 100644
>> --- a/drivers/mtd/ubi/build.c
>> +++ b/drivers/mtd/ubi/build.c
>> @@ -1263,9 +1263,21 @@ static struct mtd_notifier ubi_mtd_notifier = {
>>   	.remove = ubi_notify_remove,
>>   };
>>   
>> +static void detach_mtd_devs(int count)
> 
> Missing __init to avoid section missmatch.
> 
> See also: https://lore.kernel.org/oe-kbuild-all/202404112327.158HJfAw-lkp@intel.com/

I think function without '__init' attributes can be called in ubi_init, 
for example misc_register, kmem_cache_create, and I verify it by make 
W=1 in local machine. And above warning(in your link) is only detected 
in my v1 series.
After investigating the '__init' and '__exit', I understand that there 
are two independent text section for these functions, for example, 
__init text section will be removed from memory after it is finished, so 
we cannot call __exit function in __init function.
> 
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < count; i++)
>> +		if (ubi_devices[i]) {
>> +			mutex_lock(&ubi_devices_mutex);
>> +			ubi_detach_mtd_dev(ubi_devices[i]->ubi_num, 1);
>> +			mutex_unlock(&ubi_devices_mutex);
>> +		}
>> +}