[PATCH] driver core: Fix error handling in driver API device_rename()

[PATCH] driver core: Fix error handling in driver API device_rename()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Call failure of device_rename(@dev, @new_name) maybe unexpectedly change
link name within @dev's class directory to @new_name, fixed by correcting
error handling for the API.

Fixes: f349cf34731c ("driver core: Implement ns directory support for device classes.")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2b4c0624b704..a05b7186cf33 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4512,9 +4512,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
  */
 int device_rename(struct device *dev, const char *new_name)
 {
+	struct subsys_private *sp = NULL;
 	struct kobject *kobj = &dev->kobj;
 	char *old_device_name = NULL;
 	int error;
+	bool is_link_renamed = false;
 
 	dev = get_device(dev);
 	if (!dev)
@@ -4529,7 +4531,7 @@ int device_rename(struct device *dev, const char *new_name)
 	}
 
 	if (dev->class) {
-		struct subsys_private *sp = class_to_subsys(dev->class);
+		sp = class_to_subsys(dev->class);
 
 		if (!sp) {
 			error = -EINVAL;
@@ -4537,17 +4539,20 @@ int device_rename(struct device *dev, const char *new_name)
 		}
 
 		error = sysfs_rename_link_ns(&sp->subsys.kobj, kobj, old_device_name,
-					     new_name, kobject_namespace(kobj));
-		subsys_put(sp);
+				new_name, kobject_namespace(kobj));
 		if (error)
 			goto out;
+
+		is_link_renamed = true;
 	}
 
 	error = kobject_rename(kobj, new_name);
-	if (error)
-		goto out;
-
 out:
+	if (error && is_link_renamed)
+		sysfs_rename_link_ns(&sp->subsys.kobj, kobj, new_name,
+				old_device_name, kobject_namespace(kobj));
+	subsys_put(sp);
+
 	put_device(dev);
 
 	kfree(old_device_name);

---
base-commit: 51835949dda3783d4639cfa74ce13a3c9829de00
change-id: 20240717-device_rename_fix-ecef084784e5

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

Re: [PATCH] driver core: Fix error handling in driver API device_rename()

[Greg Kroah-Hartman]

On Wed, Jul 17, 2024 at 10:50:05PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Call failure of device_rename(@dev, @new_name) maybe unexpectedly change
> link name within @dev's class directory to @new_name, fixed by correcting
> error handling for the API.

I'm sorry, but I don't understand the text here, what exactly are you
doing?

> Fixes: f349cf34731c ("driver core: Implement ns directory support for device classes.")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/base/core.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 2b4c0624b704..a05b7186cf33 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4512,9 +4512,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
>   */
>  int device_rename(struct device *dev, const char *new_name)
>  {
> +	struct subsys_private *sp = NULL;
>  	struct kobject *kobj = &dev->kobj;
>  	char *old_device_name = NULL;
>  	int error;
> +	bool is_link_renamed = false;
>  
>  	dev = get_device(dev);
>  	if (!dev)
> @@ -4529,7 +4531,7 @@ int device_rename(struct device *dev, const char *new_name)
>  	}
>  
>  	if (dev->class) {
> -		struct subsys_private *sp = class_to_subsys(dev->class);
> +		sp = class_to_subsys(dev->class);
>  
>  		if (!sp) {
>  			error = -EINVAL;
> @@ -4537,17 +4539,20 @@ int device_rename(struct device *dev, const char *new_name)
>  		}
>  
>  		error = sysfs_rename_link_ns(&sp->subsys.kobj, kobj, old_device_name,
> -					     new_name, kobject_namespace(kobj));
> -		subsys_put(sp);
> +				new_name, kobject_namespace(kobj));

Why did you change the indentation here?

>  		if (error)
>  			goto out;
> +
> +		is_link_renamed = true;
>  	}
>  
>  	error = kobject_rename(kobj, new_name);
> -	if (error)
> -		goto out;
> -
>  out:
> +	if (error && is_link_renamed)
> +		sysfs_rename_link_ns(&sp->subsys.kobj, kobj, new_name,
> +				old_device_name, kobject_namespace(kobj));
> +	subsys_put(sp);

How was this found?  What in-kernel code causes this problem?  And how
was this tested?

thanks,

greg k-h

Re: [PATCH] driver core: Fix error handling in driver API device_rename()

[Zijun Hu]

On 2024/7/17 23:03, Greg Kroah-Hartman wrote:
> On Wed, Jul 17, 2024 at 10:50:05PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Call failure of device_rename(@dev, @new_name) maybe unexpectedly change
>> link name within @dev's class directory to @new_name, fixed by correcting
>> error handling for the API.
> 
> I'm sorry, but I don't understand the text here, what exactly are you
> doing?
> 

let me explain what is the issue by inline comments within present code
firstly, i will make commit message clear for v2 when necessary

@@ -4528,29 +4528,30 @@ int device_rename(struct device *dev, const char
*new_name)
                goto out;
        }

        if (dev->class) {
                struct subsys_private *sp = class_to_subsys(dev->class);

                if (!sp) {
                        error = -EINVAL;
                        goto out;
                }
-
+               /* 1) rename the link name to new name */
                error = sysfs_rename_link_ns(&sp->subsys.kobj, kobj,
old_device_name,
                                             new_name,
kobject_namespace(kobj));
                subsys_put(sp);
                if (error)
                        goto out;
        }

        error = kobject_rename(kobj, new_name);
+       /* but forget to revert the jobs done by 1) if below error
really happens */
        if (error)
                goto out;

 out:
        put_device(dev);

        kfree(old_device_name);

        return error;
 }


what i am doing is to undo the 1) job when the given error happens.

>> Fixes: f349cf34731c ("driver core: Implement ns directory support for device classes.")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/base/core.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2b4c0624b704..a05b7186cf33 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -4512,9 +4512,11 @@ EXPORT_SYMBOL_GPL(device_destroy);
>>   */
>>  int device_rename(struct device *dev, const char *new_name)
>>  {
>> +	struct subsys_private *sp = NULL;
>>  	struct kobject *kobj = &dev->kobj;
>>  	char *old_device_name = NULL;
>>  	int error;
>> +	bool is_link_renamed = false;
>>  
>>  	dev = get_device(dev);
>>  	if (!dev)
>> @@ -4529,7 +4531,7 @@ int device_rename(struct device *dev, const char *new_name)
>>  	}
>>  
>>  	if (dev->class) {
>> -		struct subsys_private *sp = class_to_subsys(dev->class);
>> +		sp = class_to_subsys(dev->class);
>>  
>>  		if (!sp) {
>>  			error = -EINVAL;
>> @@ -4537,17 +4539,20 @@ int device_rename(struct device *dev, const char *new_name)
>>  		}
>>  
>>  		error = sysfs_rename_link_ns(&sp->subsys.kobj, kobj, old_device_name,
>> -					     new_name, kobject_namespace(kobj));
>> -		subsys_put(sp);
>> +				new_name, kobject_namespace(kobj));
> 
> Why did you change the indentation here?
> 

it is caused by that my ~/.vim/plugin/linuxsty.vim is broken, it is a
link but the target is loss.
will fix this indentation issue within v2.

>>  		if (error)
>>  			goto out;
>> +
>> +		is_link_renamed = true;
>>  	}
>>  
>>  	error = kobject_rename(kobj, new_name);
>> -	if (error)
>> -		goto out;
>> -
>>  out:
>> +	if (error && is_link_renamed)
>> +		sysfs_rename_link_ns(&sp->subsys.kobj, kobj, new_name,
>> +				old_device_name, kobject_namespace(kobj));
>> +	subsys_put(sp);
> 
> How was this found?  What in-kernel code causes this problem?  And how
> was this tested?
> 
find it by reading code, no other in-kernel code cause this issue but
the API itself, i wrote a simple code to test it.
> thanks,
> 
> greg k-h