* [PATCH] race condition fixing in sysfs_create_dir
@ 2013-07-26 9:49 Dennis Chen
2013-07-26 9:59 ` Dennis Chen
0 siblings, 1 reply; 6+ messages in thread
From: Dennis Chen @ 2013-07-26 9:49 UTC (permalink / raw)
To: linux-kernel; +Cc: gregkh, teheo, xiyou.wangcong, 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->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...
Signed-off-by: Dennis Chen <xschen@tnsoft.com.cn>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Tejun Heo <teheo@suse.de>
Cc: Wang Cong <xiyou.wangcong@gmail.com>
---
fs/sysfs/dir.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index e068e74..114073d 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] race condition fixing in sysfs_create_dir
2013-07-26 9:49 [PATCH] race condition fixing in sysfs_create_dir Dennis Chen
@ 2013-07-26 9:59 ` Dennis Chen
2013-07-26 13:38 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Dennis Chen @ 2013-07-26 9:59 UTC (permalink / raw)
To: linux-kernel, gregkh; +Cc: Tejun Heo, xiyou.wangcong, Dennis Chen
On 07/26/2013 05:49 PM, Dennis Chen wrote:
> 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...
> Signed-off-by: Dennis Chen <xschen@tnsoft.com.cn>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Tejun Heo <teheo@suse.de>
should be Tejun Heo <tj@kernel.org>
> Cc: Wang Cong <xiyou.wangcong@gmail.com>
> ---
> fs/sysfs/dir.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index e068e74..114073d 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -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);
Re CC Tejun whose email addess <@suse.de> is obsolete :)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] race condition fixing in sysfs_create_dir
2013-07-26 9:59 ` Dennis Chen
@ 2013-07-26 13:38 ` Tejun Heo
2013-07-26 13:41 ` Tejun Heo
2013-07-30 6:34 ` Dennis Chen
0 siblings, 2 replies; 6+ messages in thread
From: Tejun Heo @ 2013-07-26 13:38 UTC (permalink / raw)
To: Dennis Chen; +Cc: linux-kernel, gregkh, xiyou.wangcong
Hello,
On Fri, Jul 26, 2013 at 05:59:00PM +0800, Dennis Chen wrote:
> On 07/26/2013 05:49 PM, Dennis Chen wrote:
>
> >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...
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.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] race condition fixing in sysfs_create_dir
2013-07-26 13:38 ` Tejun Heo
@ 2013-07-26 13:41 ` Tejun Heo
2013-07-30 6:34 ` Dennis Chen
1 sibling, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2013-07-26 13:41 UTC (permalink / raw)
To: Dennis Chen; +Cc: linux-kernel, gregkh, xiyou.wangcong
On Fri, Jul 26, 2013 at 09:38:56AM -0400, Tejun Heo wrote:
> 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.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] race condition fixing in sysfs_create_dir
2013-07-26 13:38 ` Tejun Heo
2013-07-26 13:41 ` Tejun Heo
@ 2013-07-30 6:34 ` Dennis Chen
2013-07-30 14:10 ` Tejun Heo
1 sibling, 1 reply; 6+ messages in thread
From: Dennis Chen @ 2013-07-30 6:34 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, gregkh, xiyou.wangcong
On 07/26/2013 09:38 PM, Tejun Heo wrote:
> Hello,
>
> On Fri, Jul 26, 2013 at 05:59:00PM +0800, Dennis Chen wrote:
>> On 07/26/2013 05:49 PM, Dennis Chen wrote:
>>
>>> 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...
> 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?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] race condition fixing in sysfs_create_dir
2013-07-30 6:34 ` Dennis Chen
@ 2013-07-30 14:10 ` Tejun Heo
0 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2013-07-30 14:10 UTC (permalink / raw)
To: Dennis Chen; +Cc: linux-kernel, gregkh, xiyou.wangcong
Hello,
On Tue, Jul 30, 2013 at 02:34:56PM +0800, Dennis Chen wrote:
> >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->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
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.
> when its sub-dir is creating by sysfs_create_dir, then the similar
> action should be taken as sysfs_create_link does. 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.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-30 14:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-26 9:49 [PATCH] race condition fixing in sysfs_create_dir Dennis Chen
2013-07-26 9:59 ` Dennis Chen
2013-07-26 13:38 ` Tejun Heo
2013-07-26 13:41 ` Tejun Heo
2013-07-30 6:34 ` Dennis Chen
2013-07-30 14:10 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox