[PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[Jianyu Zhan]

From: Jianyu Zhan <Jianyu.Zhan@emc.com>

I've met such a race conditon the same as what commit 3a198886
("sysfs: handle 'parent deleted before child added'") tackled.

The senario got triggered under a torturing test of quick disk
removal and plugging.

The forementioned commit 3a198886 didn't really fix this race,
it just narrows down the racy window.

A proper fix for this is to pin the parent kernfs_node on the
dir_entry creation point (sysfs_create_dir_ns), and to de-pin the parent
on the dir_entry deletion point(sysfs_remove_dir).

Though Tejun has added a comment in sysfs_remove_dir to warn such a
race and leave the option to upper layer. But wrt correct logic point,
I think it is good to add such pin behavior.

Note. another commit 26ea12de("kobject: grab an extra reference on kobject->sd
to allow duplicate deletes") also adds such a pin at the kobject layer.
But this commit tackles different issue - it pins an entry(not its parent)
after its creation, to allow duplicate deletions.

Signed-off-by: Jianyu Zhan <nasa4836@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 0b45ff4..0ac82cd 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -49,7 +49,12 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
 	else
 		parent = sysfs_root_kn;
 
-	if (!parent)
+	/*
+	 * We might race with the parent node removal.
+	 * To avoid such race and for logic correctness,
+	 * we pin the parent node here.
+	 */
+	if (!sysfs_get(parent))
 		return -ENOENT;
 
 	kn = kernfs_create_dir_ns(parent, kobject_name(kobj),
@@ -94,6 +99,10 @@ void sysfs_remove_dir(struct kobject *kobj)
 
 	if (kn) {
 		WARN_ON_ONCE(kernfs_type(kn) != KERNFS_DIR);
+		/*
+		 * Drop our pin on parent
+		 */
+		sysfs_put(kn->parent);
 		kernfs_remove(kn);
 	}
 }
-- 
2.0.1.472.g6f92e5f

Re: [PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[Tejun Heo]

Hello,

On Fri, Aug 01, 2014 at 07:18:12PM +0800, Jianyu Zhan wrote:
> A proper fix for this is to pin the parent kernfs_node on the
> dir_entry creation point (sysfs_create_dir_ns), and to de-pin the parent
> on the dir_entry deletion point(sysfs_remove_dir).

The *only* reason we have the warning at all is because the driver
model wants to enforce that destruction is performed in the proper
order and what you're doing is just circumventing the detection logic.
We might as well just remove it.  Please track down why the specific
warning that you saw happened.  We wanna learn about them and fix
them.

Thanks.

-- 
tejun

Re: [PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[Jianyu Zhan]

Hi, tj,

On Fri, Aug 1, 2014 at 8:50 PM, Tejun Heo <tj@kernel.org> wrote:
> The *only* reason we have the warning at all is because the driver
> model wants to enforce that destruction is performed in the proper
> order and what you're doing is just circumventing the detection logic.
> We might as well just remove it.  Please track down why the specific
> warning that you saw happened.  We wanna learn about them and fix
> them.

The problem I met is in an old kernel, in sysfs_create_dir(),  at thie line:

   sysfs_create_dir()
       if (kobj->parent)
            parent = kobj->parent->sd;

I found kobj->parent is valid, so parent == kobj->parent->sd,
then it is passed into create_dir() function, in which it is dereferenced,
however  the parent passed in is NULL, so a panic.

Apprently, there is a race, as my case is a test of fast removal and plugging
of a block device.

The race is that the kerfs_node(was sysfs_dirent) is disassociated with
parent kobject, but parent kobject is still alive, so we saw it.

And the commit 3a198886 ("sysfs: handle 'parent deleted before child added'")
add a parent NULLness check before calling into create_dir(),  but I think this
isn't the real fix, it just narrow down the racy window.


Thanks,
Jianyu Zhan

Re: [PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[Tejun Heo]

(cc'ing Dan)

Hello, Jianyu, Dan.

On Fri, Aug 01, 2014 at 10:31:00PM +0800, Jianyu Zhan wrote:
> The problem I met is in an old kernel, in sysfs_create_dir(),  at thie line:

Which old kernel?

>    sysfs_create_dir()
>        if (kobj->parent)
>             parent = kobj->parent->sd;
> 
> I found kobj->parent is valid, so parent == kobj->parent->sd,
> then it is passed into create_dir() function, in which it is dereferenced,
> however  the parent passed in is NULL, so a panic.

Which node?

> Apprently, there is a race, as my case is a test of fast removal and plugging
> of a block device.
> 
> The race is that the kerfs_node(was sysfs_dirent) is disassociated with
> parent kobject, but parent kobject is still alive, so we saw it.

What is the race condition?  You're just describing symptoms.

> And the commit 3a198886 ("sysfs: handle 'parent deleted before child added'")
> add a parent NULLness check before calling into create_dir(),  but I think this
> isn't the real fix, it just narrow down the racy window.

Suspicions aren't good enough justifications.  If you think there's a
race window, please track it down and then determine what the proper
fix is.  You're now trying to change the basic objection lifetime
rules of the driver model without root causing what's actually going
on.  Please don't do things like this.

Thanks.

-- 
tejun

Re: [PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[Jianyu Zhan]

Hi, all,

On Fri, Aug 1, 2014 at 11:18 PM, Tejun Heo <tj@kernel.org> wrote:
>
> Which old kernel?
>

  Quite old , 2.6.32.

>>    sysfs_create_dir()
>>        if (kobj->parent)
>>             parent = kobj->parent->sd;
>>
>> I found kobj->parent is valid, so parent == kobj->parent->sd,
>> then it is passed into create_dir() function, in which it is dereferenced,
>> however  the parent passed in is NULL, so a panic.
>
> Which node?
>

           /sys/block
               |
               |
             dm-111           <======      kobj->parent
               |
               |
             dm-111p1        <======     kobj



>> Apprently, there is a race, as my case is a test of fast removal and plugging
>> of a block device.
>>
>> The race is that the kerfs_node(was sysfs_dirent) is disassociated with
>> parent kobject, but parent kobject is still alive, so we saw it.
>
> What is the race condition?  You're just describing symptoms.

In this scenario, an user behavior that removes the block device dm-111 ,
and in the meanwhile, I don't know what trigger this, but a
rescan_partition() runs on dm-111,
in turn it caused an add_partition().

And after the crash dump analysis,  I found that the parent node is
removed (kobj->parent->sd == NULL),
but kobj-parent is still alive, as my previous mail described.

So this seems an invalid add_partition() of dm-111p1 on  non-existent dm-111.

And the race is that the parent is on its removal path, but the child
is on its creating path,
and it could observe parent's middle status -- sysfs_dirent is gone,
but kobject is alive.

>> And the commit 3a198886 ("sysfs: handle 'parent deleted before child added'")
>> add a parent NULLness check before calling into create_dir(),  but I think this
>> isn't the real fix, it just narrow down the racy window.
>
> Suspicions aren't good enough justifications.  If you think there's a
> race window, please track it down and then determine what the proper
> fix is.  You're now trying to change the basic objection lifetime
> rules of the driver model without root causing what's actually going
> on.  Please don't do things like this.

Yep, you are right.

Thanks,
Jianyu Zhan

Re: [PATCH RFC] sysfs: fix the race of "parent deleted before child added"

[Dan Williams]

On Fri, Aug 1, 2014 at 4:18 AM, Jianyu Zhan <nasa4836@gmail.com> wrote:
> From: Jianyu Zhan <Jianyu.Zhan@emc.com>
>
> I've met such a race conditon the same as what commit 3a198886
> ("sysfs: handle 'parent deleted before child added'") tackled.
>
> The senario got triggered under a torturing test of quick disk
> removal and plugging.
>
> The forementioned commit 3a198886 didn't really fix this race,
> it just narrows down the racy window.

You misread the changelog.  It was never meant to be a fix, its there
to prevent abuses from crashing this system and rather return an
error.

The commit that fixed the race I was seeing is here:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3b661a92

Perhaps you have found another occasion where a device is being
device_del()'d right before a device_add() attempt?