drivers/base/core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
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>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
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
© 2016 - 2025 Red Hat, Inc.