race condition fixing in sysfs_create_dir
PATCH 1/1 race condition fixing in sysfs_create_dir
The patch is trying its best to avoid creating a dir under a parent dir which is removing from the system:
PATH0 (create a dir under 'PARENT/...') PATH1 (remove the 'PARENT/...')
sysfs_create_dir() { sysfs_remove_dir() {
... ...
if (kobj->parent) spin_lock(&sysfs_assoc_lock);
parent_sd = kobj->parent->sd; <----- kobj->sd = NULL;
else spin_unlock(&sysfs_assoc_lock);
parent_sd = &sysfs_root;Suppose PATH1 enter the critical section first, then PATH0 begin to execute before kobj->sd has been reset to NULL, possibly PATH0 will get a non-NULL parent_sd since lack of the sysfs_assoc_lock protection in PATH0. In this case, PATH0 think it has a valid parent_sd which can be freed by PATH1 in the followed, refer to the comments in the patch. Maybe we need to figure out a perfect solution to solve the race condition, although the codes in question are in slow path...
---fs/sysfs/dir.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
fs/sysfs/dir.c +1 -0
@@ -746,13 +746,22 @@ int sysfs_create_dir(struct kobject * kobj)BUG_ON (!kobj); +spin_lock (&sysfs_assoc_lock);
if (kobj->parent) parent_sd = kobj->parent->sd; else parent_sd = &sysfs_root;
- if (!parent_sd)
+ if (!parent_sd) {
+ spin_unlock(&sysfs_assoc_lock);
return -ENOENT;
+ }
+ spin_unlock(&sysfs_assoc_lock);
+ /* TODO: although the sysfs is in a slow path, but in the operation
+ * followed, we still have a window to let the sysfs_remove_dir to
+ * free the memory space pointered by parent_sd till we inc its ref
+ * count in __sysfs_add_one()
+ */ if (sysfs_ns_type(parent_sd))
ns = kobj->ktype->namespace(kobj);Signature · 1 line
1.7.10.4
Quoted 15 lines · Dennis Chen
The patch is trying its best to avoid creating a dir under a parent dir which is removing from the system:
PATH0 (create a dir under 'PARENT/...') PATH1 (remove the 'PARENT/...') sysfs_create_dir() { sysfs_remove_dir() { ... ... if (kobj->parent) spin_lock(&sysfs_assoc_lock); parent_sd = kobj->parent->sd; <----- kobj->sd = NULL; else spin_unlock(&sysfs_assoc_lock); parent_sd = &sysfs_root;Suppose PATH1 enter the critical section first, then PATH0 begin to execute before
kobj->sdhas been reset to NULL, possibly PATH0 will get a non-NULLparent_sdsince lack of thesysfs_assoc_lockprotection in PATH0. In this case, PATH0 think it has a validparent_sdwhich can be freed by PATH1 in the followed, refer to the comments in the patch. Maybe we need to figure out a perfect solution to solve the race condition, although the codes in question are in slow path...
Quoted 3 lines
Signed-off-by Dennis Chen <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Tejun Heo <[email protected]>
should be Tejun Heo <[email protected]>
Quoted 18 lines
Cc Wang Cong <[email protected]>---
fs/sysfs/dir.c| 11 ++++++++++-1 file changed, 10 insertions(+), 1 deletion(-)fs/sysfs/dir.c +3
-1@@ -746,13 +746,22 @@ int sysfs_create_dir(struct kobject * kobj)BUG_ON (!kobj); +spin_lock (&sysfs_assoc_lock);if (kobj->parent) parent_sd = kobj->parent->sd;else parent_sd = &sysfs_root; -if (!parent_sd) +if (!parent_sd) { +spin_unlock (&sysfs_assoc_lock);
fs/sysfs/dir.c int sysfs_create_dir(struct kobject * kobj) · line 763
Quoted 10 lines
fs/sysfs/dir.c +7
-0return -ENOENT ; + } +spin_unlock (&sysfs_assoc_lock); +/* TODO: although the sysfs is in a slow path, but in the operation +* followed, we still have a window to let the sysfs_remove_dir to +* free the memory space pointered by parent_sd till we inc its ref +* count in __sysfs_add_one() +*/ if (sysfs_ns_type (parent_sd)) ns = kobj->ktype->namespace (kobj);
Re CC Tejun whose email addess <@suse.de> is obsolete :)
Hello,
Quoted 17 lines · Dennis Chen
Quoted 9 lines · Dennis Chen
The patch is trying its best to avoid creating a dir under a parent dir which is removing from the system:
PATH0 (create a dir under 'PARENT/...') PATH1 (remove the 'PARENT/...') sysfs_create_dir() { sysfs_remove_dir() { ... ... if (kobj->parent) spin_lock(&sysfs_assoc_lock); parent_sd = kobj->parent->sd; <----- kobj->sd = NULL; else spin_unlock(&sysfs_assoc_lock); parent_sd = &sysfs_root;Quoted 6 lines
Suppose PATH1 enter the critical section first, then PATH0 begin to execute before
kobj->sdhas been reset to NULL, possibly PATH0 will get a non-NULLparent_sdsince lack of thesysfs_assoc_lockprotection in PATH0. In this case, PATH0 think it has a validparent_sdwhich can be freed by PATH1 in the followed, refer to the comments in the patch. Maybe we need to figure out a perfect solution to solve the race condition, although the codes in question are in slow path...
I don't think sysfs is supposed to handle multiple actors trying to populate and destroy the directory at the same time at all, so this seems kinda moot. Do you have a case where this actually matters?
Thanks.
Signature · 1 line
tejun
Quoted 2 lines · Tejun Heo
I don't think sysfs is supposed to handle multiple actors trying to populate and destroy the directory at the same time at all, so this
e.g. kobj->sd assignment in sysfs_create_dir() isn't synchronized and doesn't have memory barrier either. Nothing prevents subdirectory creation from accessing half-initialized sysfs_dirent on architectures which require read dependency barriers. The caller is responsible for synchronizing these operations.
Thanks.
Signature · 1 line
tejun
Quoted 20 lines · Tejun Heo
Hello,
Quoted 17 lines · Dennis Chen
Quoted 9 lines · Dennis Chen
The patch is trying its best to avoid creating a dir under a parent dir which is removing from the system:
PATH0 (create a dir under 'PARENT/...') PATH1 (remove the 'PARENT/...') sysfs_create_dir() { sysfs_remove_dir() { ... ... if (kobj->parent) spin_lock(&sysfs_assoc_lock); parent_sd = kobj->parent->sd; <----- kobj->sd = NULL; else spin_unlock(&sysfs_assoc_lock); parent_sd = &sysfs_root;Quoted 6 lines
Suppose PATH1 enter the critical section first, then PATH0 begin to execute before
kobj->sdhas been reset to NULL, possibly PATH0 will get a non-NULLparent_sdsince lack of thesysfs_assoc_lockprotection in PATH0. In this case, PATH0 think it has a validparent_sdwhich can be freed by PATH1 in the followed, refer to the comments in the patch. Maybe we need to figure out a perfect solution to solve the race condition, although the codes in question are in slow path...
Quoted 6 lines
I don't think sysfs is supposed to handle multiple actors trying to populate and destroy the directory at the same time at all, so this seems kinda moot. Do you have a case where this actually matters?
Thanks.
hello,Tejun. Nice. But seems I still have different opinion :). If you look at the 'sysfs_do_create_link_sd()' code, you will find a comment "target->sd can go away beneath us but is protected with sysfs_assoc_lock. Fetch target_sd from it", don't you think the sysfs_create_dir is the same as the sysfs_do_create_link_sd() essentially? if the answer is yes meaning the parent dir can go away when its sub-dir is creating by sysfs_create_dir, then the similar action should be taken as sysfs_create_link does. right?
Hello,
Quoted 10 lines · Dennis Chen
Quoted 3 lines
I don't think sysfs is supposed to handle multiple actors trying to populate and destroy the directory at the same time at all, so this seems kinda moot. Do you have a case where this actually matters?
hello,Tejun. Nice. But seems I still have different opinion :). If you look at the '
sysfs_do_create_link_sd()' code, you will find a comment "target->sdcan go away beneath us but is protected withsysfs_assoc_lock. Fetchtarget_sdfrom it", don't you think thesysfs_create_diris the same as thesysfs_do_create_link_sd()essentially? if the answer is yes meaning the parent dir can go away
No, one is targetting an unrelated directory wherever in the hierarchy and the other one is targetting its direct parent. They aren't the same.
Quoted 2 lines
when its sub-dir is creating by
sysfs_create_dir, then the similar action should be taken assysfs_create_linkdoes. right?
If you own a sysfs directory, it's your responsibility to prevent creation of new entries under it against your own removal. The implementation implicitly assumes that in many places. Do you have a use case where this is an actual problem?
Thanks.
Signature · 1 line
tejun