[PATCH -next 0/3] cleanup of devtmpfs_*_node()

[PATCH -next 0/3] cleanup of devtmpfs_*_node()

[Longlong Xia]

In one test, when modprobe zram, no zram device was found in the /dev. 
But don't see any errors printed in jouranls/dmesg. Later we found out 
that the reason was that device_add() did not check its return value when
calling devtmpfs_create_node(). So we hope to turn devtmpfs_*_node() & 
devtmpfs_submit_req() into a function with no return value, and add some
debug info in the handle() that actually processes the request to let the
user know why the creation was not successful.

Patch [1] devtmpfs: convert to pr_fmt. 
Patch [2] devtmpfs: add debug info to handle().
Patch [3] devtmpfs: Remove return value of devtmpfs_*_node() & 
devtmpfs_submit_req().

Longlong Xia (3):
  devtmpfs: convert to pr_fmt
  devtmpfs: add debug info to handle()
  devtmpfs: remove return value of devtmpfs_*_node() &
    devtmpfs_submit_req()

 drivers/base/base.h     |  8 +++----
 drivers/base/devtmpfs.c | 48 +++++++++++++++++++++++------------------
 2 files changed, 31 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH -next 1/3] devtmpfs: convert to pr_fmt

[Longlong Xia]

Use the pr_fmt() macro to prefix all the output with "devtmpfs: ".
while at it, convert printk(<LEVEL>) to pr_<level>().

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/devtmpfs.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 03e8a95f1f35..ae72d4ba8547 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -13,6 +13,8 @@
  * overwrite the default setting if needed.
  */
 
+#define pr_fmt(fmt) "devtmpfs: " fmt
+
 #include <linux/kernel.h>
 #include <linux/syscalls.h>
 #include <linux/mount.h>
@@ -376,9 +378,9 @@ int __init devtmpfs_mount(void)
 
 	err = init_mount("devtmpfs", "dev", "devtmpfs", DEVTMPFS_MFLAGS, NULL);
 	if (err)
-		printk(KERN_INFO "devtmpfs: error mounting %i\n", err);
+		pr_info("error mounting %d\n", err);
 	else
-		printk(KERN_INFO "devtmpfs: mounted\n");
+		pr_info("mounted\n");
 	return err;
 }
 
@@ -460,14 +462,12 @@ int __init devtmpfs_init(void)
 
 	mnt = vfs_kern_mount(&internal_fs_type, 0, "devtmpfs", opts);
 	if (IS_ERR(mnt)) {
-		printk(KERN_ERR "devtmpfs: unable to create devtmpfs %ld\n",
-				PTR_ERR(mnt));
+		pr_err("unable to create devtmpfs %ld\n", PTR_ERR(mnt));
 		return PTR_ERR(mnt);
 	}
 	err = register_filesystem(&dev_fs_type);
 	if (err) {
-		printk(KERN_ERR "devtmpfs: unable to register devtmpfs "
-		       "type %i\n", err);
+		pr_err("unable to register devtmpfs type %d\n", err);
 		return err;
 	}
 
@@ -480,12 +480,12 @@ int __init devtmpfs_init(void)
 	}
 
 	if (err) {
-		printk(KERN_ERR "devtmpfs: unable to create devtmpfs %i\n", err);
+		pr_err("unable to create devtmpfs %d\n", err);
 		unregister_filesystem(&dev_fs_type);
 		thread = NULL;
 		return err;
 	}
 
-	printk(KERN_INFO "devtmpfs: initialized\n");
+	pr_info("initialized\n");
 	return 0;
 }
-- 
2.25.1

Re: [PATCH -next 1/3] devtmpfs: convert to pr_fmt

[Greg KH]

On Thu, Feb 02, 2023 at 03:32:01AM +0000, Longlong Xia wrote:
> Use the pr_fmt() macro to prefix all the output with "devtmpfs: ".
> while at it, convert printk(<LEVEL>) to pr_<level>().
> 
> Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
> ---
>  drivers/base/devtmpfs.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Nice!

I'll take this one now, the other two I've sent review comments on
already.  Please fix them up and resend them and I'll be glad to
consider taking them.

thanks,

greg k-h

[PATCH -next 2/3] devtmpfs: add debug info to handle()

[Longlong Xia]

The devtmpfs_*_node() are used to mount/unmount devices to /dev, but their
callers don't check their return value, so we don't know the reason for
the failure. Let's add some debug info in handle() to help users know
why failed.

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/devtmpfs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index ae72d4ba8547..77ca64f708ce 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -389,10 +389,18 @@ static __initdata DECLARE_COMPLETION(setup_done);
 static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid,
 		  struct device *dev)
 {
+	int ret;
+
 	if (mode)
-		return handle_create(name, mode, uid, gid, dev);
+		ret = handle_create(name, mode, uid, gid, dev);
 	else
-		return handle_remove(name, dev);
+		ret = handle_remove(name, dev);
+
+	if (ret)
+		pr_err_ratelimited("failed to %s %s, ret = %d\n",
+				   mode ? "create" : "remove", name, ret);
+
+	return ret;
 }
 
 static void __noreturn devtmpfs_work_loop(void)
-- 
2.25.1

Re: [PATCH -next 2/3] devtmpfs: add debug info to handle()

[Greg KH]

On Thu, Feb 02, 2023 at 03:32:02AM +0000, Longlong Xia wrote:
> The devtmpfs_*_node() are used to mount/unmount devices to /dev, but their
> callers don't check their return value, so we don't know the reason for
> the failure. Let's add some debug info in handle() to help users know
> why failed.
> 
> Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
> ---
>  drivers/base/devtmpfs.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index ae72d4ba8547..77ca64f708ce 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -389,10 +389,18 @@ static __initdata DECLARE_COMPLETION(setup_done);
>  static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid,
>  		  struct device *dev)
>  {
> +	int ret;
> +
>  	if (mode)
> -		return handle_create(name, mode, uid, gid, dev);
> +		ret = handle_create(name, mode, uid, gid, dev);
>  	else
> -		return handle_remove(name, dev);
> +		ret = handle_remove(name, dev);
> +
> +	if (ret)
> +		pr_err_ratelimited("failed to %s %s, ret = %d\n",
> +				   mode ? "create" : "remove", name, ret);

As you have a struct device * here, why not use dev_err() instead?

And why rate limited?  What is going to cause this to be spammed with
lots and lots of failures?  What were you doing that caused a failure
here, how is it triggered?

thanks,

greg k-h

Re: [PATCH -next 0/3] cleanup of devtmpfs_*_node()

[Longlong Xia]

The test steps:
1. Set the SElinux label of /dev to user_home_t 
2. modprobe zram num_devices=1000

The above test result is that there is no zram device was found in the 
/dev. And don't see any errors printed in jouranls/dmesg. Of course, 
it is rare to create 1000 zram devices, use dev_err may be better.

Longlong Xia (3):
  devtmpfs: add debug info to handle()
  driver core: add error handling for devtmpfs_create_node()
  devtmpfs: remove return value of devtmpfs_delete_node()

 drivers/base/base.h     |  4 ++--
 drivers/base/core.c     |  8 ++++++--
 drivers/base/devtmpfs.c | 20 ++++++++++++++------
 3 files changed, 22 insertions(+), 10 deletions(-)

Best Regards,
Longlong Xia

[PATCH -next 1/3] devtmpfs: add debug info to handle()

[Longlong Xia]

Because handle() is the core function for processing devtmpfs requests,
Let's add some debug info in handle() to help users know why failed.

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/devtmpfs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index ae72d4ba8547..7789b7be4ee5 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -389,10 +389,18 @@ static __initdata DECLARE_COMPLETION(setup_done);
 static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid,
 		  struct device *dev)
 {
+	int ret;
+
 	if (mode)
-		return handle_create(name, mode, uid, gid, dev);
+		ret = handle_create(name, mode, uid, gid, dev);
 	else
-		return handle_remove(name, dev);
+		ret = handle_remove(name, dev);
+
+	if (ret)
+		dev_err(dev, "failed to %s %s, ret = %d\n",
+			mode ? "create" : "remove", name, ret);
+
+	return ret;
 }
 
 static void __noreturn devtmpfs_work_loop(void)
-- 
2.25.1

[PATCH -next 2/3] driver core: add error handling for devtmpfs_create_node()

[Longlong Xia]

In some cases, devtmpfs_create_node() can return error value.
So, make use of it.

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7dab705f2937..aaa3088e5456 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3405,7 +3405,9 @@ int device_add(struct device *dev)
 		if (error)
 			goto SysEntryError;
 
-		devtmpfs_create_node(dev);
+		error = devtmpfs_create_node(dev);
+		if (error)
+			goto DevtmpfsError;
 	}
 
 	/* Notify clients of device addition.  This call must come
@@ -3461,6 +3463,8 @@ int device_add(struct device *dev)
 done:
 	put_device(dev);
 	return error;
+ DevtmpfsError:
+	device_remove_sys_dev_entry(dev);
  SysEntryError:
 	if (MAJOR(dev->devt))
 		device_remove_file(dev, &dev_attr_dev);
-- 
2.25.1

[PATCH -next 3/3] devtmpfs: remove return value of devtmpfs_delete_node()

[Longlong Xia]

The only caller of device_del() does not check the return value. And
there's nothing we can do when cleaning things up on a remove path.
Let's make it a void function.

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/base.h     | 4 ++--
 drivers/base/devtmpfs.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 0e806f641079..f7996bf8d28b 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -201,10 +201,10 @@ void device_pm_move_to_tail(struct device *dev);
 
 #ifdef CONFIG_DEVTMPFS
 int devtmpfs_create_node(struct device *dev);
-int devtmpfs_delete_node(struct device *dev);
+void devtmpfs_delete_node(struct device *dev);
 #else
 static inline int devtmpfs_create_node(struct device *dev) { return 0; }
-static inline int devtmpfs_delete_node(struct device *dev) { return 0; }
+static inline void devtmpfs_delete_node(struct device *dev) { return 0; }
 #endif
 
 void software_node_notify(struct device *dev);
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 7789b7be4ee5..e5c4d3e98ec9 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -147,22 +147,22 @@ int devtmpfs_create_node(struct device *dev)
 	return devtmpfs_submit_req(&req, tmp);
 }
 
-int devtmpfs_delete_node(struct device *dev)
+void devtmpfs_delete_node(struct device *dev)
 {
 	const char *tmp = NULL;
 	struct req req;
 
 	if (!thread)
-		return 0;
+		return;
 
 	req.name = device_get_devnode(dev, NULL, NULL, NULL, &tmp);
 	if (!req.name)
-		return -ENOMEM;
+		return;
 
 	req.mode = 0;
 	req.dev = dev;
 
-	return devtmpfs_submit_req(&req, tmp);
+	devtmpfs_submit_req(&req, tmp);
 }
 
 static int dev_mkdir(const char *name, umode_t mode)
-- 
2.25.1

Re: [PATCH -next 0/3] cleanup of devtmpfs_*_node()

[Greg KH]

On Fri, Feb 10, 2023 at 07:33:06AM +0000, Longlong Xia wrote:
> The test steps:
> 1. Set the SElinux label of /dev to user_home_t 
> 2. modprobe zram num_devices=1000
> 
> The above test result is that there is no zram device was found in the 
> /dev. And don't see any errors printed in jouranls/dmesg. Of course, 
> it is rare to create 1000 zram devices, use dev_err may be better.
> 
> Longlong Xia (3):
>   devtmpfs: add debug info to handle()
>   driver core: add error handling for devtmpfs_create_node()
>   devtmpfs: remove return value of devtmpfs_delete_node()
> 
>  drivers/base/base.h     |  4 ++--
>  drivers/base/core.c     |  8 ++++++--
>  drivers/base/devtmpfs.c | 20 ++++++++++++++------
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> Best Regards,
> Longlong Xia

This is a v2 series, please mark them as such, saying what changed from
the previous submission.  Otherwise our tools get totally confused and I
can not apply them.

Also just send it as a new thread, don't reply to the old one, there's
no need for making it that complex.

thanks,

greg k-h

[PATCH -next 3/3] devtmpfs: remove return value of devtmpfs_*_node() & devtmpfs_submit_req()

[Longlong Xia]

Because the return value of devtmpfs_*_node() and devtmpfs_submit_req()
are not used by their callers, change them into void functions.

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/base.h     |  8 ++++----
 drivers/base/devtmpfs.c | 20 +++++++++-----------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 2208af509ce8..ffb7321e39cf 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -198,11 +198,11 @@ extern void fw_devlink_drivers_done(void);
 void device_pm_move_to_tail(struct device *dev);
 
 #ifdef CONFIG_DEVTMPFS
-int devtmpfs_create_node(struct device *dev);
-int devtmpfs_delete_node(struct device *dev);
+void devtmpfs_create_node(struct device *dev);
+void devtmpfs_delete_node(struct device *dev);
 #else
-static inline int devtmpfs_create_node(struct device *dev) { return 0; }
-static inline int devtmpfs_delete_node(struct device *dev) { return 0; }
+static inline void devtmpfs_create_node(struct device *dev) { }
+static inline void devtmpfs_delete_node(struct device *dev) { }
 #endif
 
 void software_node_notify(struct device *dev);
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 77ca64f708ce..3c4e61c99b77 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -103,7 +103,7 @@ static inline int is_blockdev(struct device *dev)
 static inline int is_blockdev(struct device *dev) { return 0; }
 #endif
 
-static int devtmpfs_submit_req(struct req *req, const char *tmp)
+static void devtmpfs_submit_req(struct req *req, const char *tmp)
 {
 	init_completion(&req->done);
 
@@ -116,24 +116,22 @@ static int devtmpfs_submit_req(struct req *req, const char *tmp)
 	wait_for_completion(&req->done);
 
 	kfree(tmp);
-
-	return req->err;
 }
 
-int devtmpfs_create_node(struct device *dev)
+void devtmpfs_create_node(struct device *dev)
 {
 	const char *tmp = NULL;
 	struct req req;
 
 	if (!thread)
-		return 0;
+		return;
 
 	req.mode = 0;
 	req.uid = GLOBAL_ROOT_UID;
 	req.gid = GLOBAL_ROOT_GID;
 	req.name = device_get_devnode(dev, &req.mode, &req.uid, &req.gid, &tmp);
 	if (!req.name)
-		return -ENOMEM;
+		return;
 
 	if (req.mode == 0)
 		req.mode = 0600;
@@ -144,25 +142,25 @@ int devtmpfs_create_node(struct device *dev)
 
 	req.dev = dev;
 
-	return devtmpfs_submit_req(&req, tmp);
+	devtmpfs_submit_req(&req, tmp);
 }
 
-int devtmpfs_delete_node(struct device *dev)
+void devtmpfs_delete_node(struct device *dev)
 {
 	const char *tmp = NULL;
 	struct req req;
 
 	if (!thread)
-		return 0;
+		return;
 
 	req.name = device_get_devnode(dev, NULL, NULL, NULL, &tmp);
 	if (!req.name)
-		return -ENOMEM;
+		return;
 
 	req.mode = 0;
 	req.dev = dev;
 
-	return devtmpfs_submit_req(&req, tmp);
+	devtmpfs_submit_req(&req, tmp);
 }
 
 static int dev_mkdir(const char *name, umode_t mode)
-- 
2.25.1

Re: [PATCH -next 3/3] devtmpfs: remove return value of devtmpfs_*_node() & devtmpfs_submit_req()

[Greg KH]

On Thu, Feb 02, 2023 at 03:32:03AM +0000, Longlong Xia wrote:
> Because the return value of devtmpfs_*_node() and devtmpfs_submit_req()
> are not used by their callers, change them into void functions.

Why not just fix this up properly and have the callers handle the errors
if they happen?  Failures at this level should cause the device to not
be registered, as you have found out.

But only do this for devtmpfs_create_node(), there's no reason that
devtmpfs_delete_node() needs to return a value as there's nothing we can
do when cleaning things up on a remove path.  So if you split this patch
into half, I'll be glad to take that now while you work on fixing up the
return path of devtmpfs_create_node().

thanks,

greg k-h

[PATCH -next 0/3] cleanup of devtmpfs_*_node()

[Longlong Xia]

In one test, when modprobe zram, no zram device was found in the /dev. 
But don't see any errors printed in jouranls/dmesg. Later we found out 
that the reason was that device_add() did not check its return value when
calling devtmpfs_create_node(). So we hope to turn devtmpfs_*_node() & 
devtmpfs_submit_req() into a function with no return value, and add some
debug info in the handle() that actually processes the request to let the
user know why the creation was not successful.

Patch [1] devtmpfs: convert to pr_fmt. 
Patch [2] devtmpfs: add debug info to handle().
Patch [3] devtmpfs: Remove return value of devtmpfs_*_node() & 
devtmpfs_submit_req().

Longlong Xia (3):
  devtmpfs: convert to pr_fmt
  devtmpfs: add debug info to handle()
  devtmpfs: remove return value of devtmpfs_*_node() &
    devtmpfs_submit_req()

 drivers/base/base.h     |  8 +++----
 drivers/base/devtmpfs.c | 48 +++++++++++++++++++++++------------------
 2 files changed, 31 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH -next 3/3] devtmpfs: remove return value of devtmpfs_*_node() & devtmpfs_submit_req()

[Longlong Xia]

Because the return value of devtmpfs_*_node() and devtmpfs_submit_req()
are not used by their callers, change them into void functions.

Signed-off-by: Longlong Xia <xialonglong1@huawei.com>
---
 drivers/base/base.h     |  8 ++++----
 drivers/base/devtmpfs.c | 20 +++++++++-----------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 2208af509ce8..ffb7321e39cf 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -198,11 +198,11 @@ extern void fw_devlink_drivers_done(void);
 void device_pm_move_to_tail(struct device *dev);
 
 #ifdef CONFIG_DEVTMPFS
-int devtmpfs_create_node(struct device *dev);
-int devtmpfs_delete_node(struct device *dev);
+void devtmpfs_create_node(struct device *dev);
+void devtmpfs_delete_node(struct device *dev);
 #else
-static inline int devtmpfs_create_node(struct device *dev) { return 0; }
-static inline int devtmpfs_delete_node(struct device *dev) { return 0; }
+static inline void devtmpfs_create_node(struct device *dev) { }
+static inline void devtmpfs_delete_node(struct device *dev) { }
 #endif
 
 void software_node_notify(struct device *dev);
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 77ca64f708ce..3c4e61c99b77 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -103,7 +103,7 @@ static inline int is_blockdev(struct device *dev)
 static inline int is_blockdev(struct device *dev) { return 0; }
 #endif
 
-static int devtmpfs_submit_req(struct req *req, const char *tmp)
+static void devtmpfs_submit_req(struct req *req, const char *tmp)
 {
 	init_completion(&req->done);
 
@@ -116,24 +116,22 @@ static int devtmpfs_submit_req(struct req *req, const char *tmp)
 	wait_for_completion(&req->done);
 
 	kfree(tmp);
-
-	return req->err;
 }
 
-int devtmpfs_create_node(struct device *dev)
+void devtmpfs_create_node(struct device *dev)
 {
 	const char *tmp = NULL;
 	struct req req;
 
 	if (!thread)
-		return 0;
+		return;
 
 	req.mode = 0;
 	req.uid = GLOBAL_ROOT_UID;
 	req.gid = GLOBAL_ROOT_GID;
 	req.name = device_get_devnode(dev, &req.mode, &req.uid, &req.gid, &tmp);
 	if (!req.name)
-		return -ENOMEM;
+		return;
 
 	if (req.mode == 0)
 		req.mode = 0600;
@@ -144,25 +142,25 @@ int devtmpfs_create_node(struct device *dev)
 
 	req.dev = dev;
 
-	return devtmpfs_submit_req(&req, tmp);
+	devtmpfs_submit_req(&req, tmp);
 }
 
-int devtmpfs_delete_node(struct device *dev)
+void devtmpfs_delete_node(struct device *dev)
 {
 	const char *tmp = NULL;
 	struct req req;
 
 	if (!thread)
-		return 0;
+		return;
 
 	req.name = device_get_devnode(dev, NULL, NULL, NULL, &tmp);
 	if (!req.name)
-		return -ENOMEM;
+		return;
 
 	req.mode = 0;
 	req.dev = dev;
 
-	return devtmpfs_submit_req(&req, tmp);
+	devtmpfs_submit_req(&req, tmp);
 }
 
 static int dev_mkdir(const char *name, umode_t mode)
-- 
2.25.1