Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1279556 > unrolled thread

[PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

Back to article view | Back to linux.kernel

Contents

  [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch() Li Bin <huawei.libin@huawei.com> - 2015-11-30 05:00 +0100
    Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Petr Mladek <pmladek@suse.com> - 2015-11-30 15:00 +0100
    Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-01 02:20 +0100
      Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() libin <huawei.libin@huawei.com> - 2015-12-01 03:50 +0100
      Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Jiri Slaby <jslaby@suse.cz> - 2015-12-01 10:00 +0100
        Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Petr Mladek <pmladek@suse.com> - 2015-12-01 15:20 +0100
          Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Jiri Slaby <jslaby@suse.cz> - 2015-12-01 15:30 +0100
            Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Petr Mladek <pmladek@suse.com> - 2015-12-01 18:00 +0100
        Re: [PATCH] livepatch: fix race between enabled_store() and  klp_unregister_patch() Josh Poimboeuf <jpoimboe@redhat.com> - 2015-12-01 17:00 +0100

#1279556 — [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

There is a potential race as following:

CPU0                         |  CPU1
-----------------------------|-----------------------------------
enabled_store()              |  klp_unregister_patch()
                             |  |-mutex_lock(&klp_mutex);
|-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
                             |  |-mutex_unlock(&klp_mutex);
|-[process the patch's state]|
|-mutex_unlock(&klp_mutex)   |

Fix this race condition by adding klp_is_patch_registered() check in
enabled_store() after get the lock klp_mutex.

Signed-off-by: Li Bin <huawei.libin@huawei.com>
---
 kernel/livepatch/core.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index db545cb..50af971 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -614,6 +614,11 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 	mutex_lock(&klp_mutex);
 
+	if (!klp_is_patch_registered(patch)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
 	if (val == patch->state) {
 		/* already in requested state */
 		ret = -EINVAL;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1279895 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On Mon 2015-11-30 11:54:37, Li Bin wrote:
> There is a potential race as following:
> 
> CPU0                         |  CPU1
> -----------------------------|-----------------------------------
> enabled_store()              |  klp_unregister_patch()
>                              |  |-mutex_lock(&klp_mutex);
> |-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
>                              |  |-mutex_unlock(&klp_mutex);
> |-[process the patch's state]|
> |-mutex_unlock(&klp_mutex)   |
> 
> Fix this race condition by adding klp_is_patch_registered() check in
> enabled_store() after get the lock klp_mutex.

It seems that you are right but the situation is more complicated.
See below.

> 
> Signed-off-by: Li Bin <huawei.libin@huawei.com>
> ---
>  kernel/livepatch/core.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index db545cb..50af971 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -614,6 +614,11 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	mutex_lock(&klp_mutex);
>  
> +	if (!klp_is_patch_registered(patch)) {

How is it guaranteed that "patch" is still a valid pointer, please?

I think that we also need to modify klp_unregister_patch().
It must not finish until the patch->kobj is destroyed.
Otherwise, the patch module would get removed and
the "patch" would be invalid.

I guess that enabled_store() takes reference of the patch->kobj.
Then kobject_put(&patch->kobj) in klp_free_patch() decrements
the refcount but it does not free the object. It means that
it does not wait for  enabled_store() to finish. I am not 100%
sure because the kobject/sysfs code is quite complex.


Anyway, we should refuse to call klp_unregister_patch() at all
in the current implementation. It is not safe because we
do not know if the patch is still used or not.

Note that if you disable the patch, you are only sure that
the new function will not longer be called. But you
do _not_ know if all the existing calls have already finished.
A process might sleep inside the function from the patch.
I guess that it will be possible after we introduce
a more complex consistency algorithm for switching
the patches.

Best Regards,
Petr



> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	if (val == patch->state) {
>  		/* already in requested state */
>  		ret = -EINVAL;
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280458 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On Mon, Nov 30, 2015 at 11:54:37AM +0800, Li Bin wrote:
> There is a potential race as following:
> 
> CPU0                         |  CPU1
> -----------------------------|-----------------------------------
> enabled_store()              |  klp_unregister_patch()
>                              |  |-mutex_lock(&klp_mutex);
> |-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
>                              |  |-mutex_unlock(&klp_mutex);
> |-[process the patch's state]|
> |-mutex_unlock(&klp_mutex)   |
> 
> Fix this race condition by adding klp_is_patch_registered() check in
> enabled_store() after get the lock klp_mutex.

I'm thinking this race isn't possible, and that instead it would
deadlock.

When I try to recreate something similar by putting a delay in
enabled_store(), klp_free_patch() just sleeps on its call to
kobject_put() until enabled_store() returns.  The unregister stack looks
like:

  [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
  [<ffffffff812ea273>] kernfs_remove+0x23/0x40
  [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
  [<ffffffff81407fb8>] kobject_del+0x18/0x50
  [<ffffffff8140804a>] kobject_release+0x5a/0x190
  [<ffffffff81407f27>] kobject_put+0x27/0x50
  [<ffffffff81128d41>] klp_unregister_patch+0x71/0x80

It seems to be waiting on a lock which is held by the kernfs code which
called enabled_store().  So when enabled_store() tries to get the
klp_mutex, it deadlocks.

Miroslav and I previously discussed a few options to fix this:

 https://lkml.kernel.org/r/alpine.LNX.2.00.1502171728520.29490@pobox.suse.cz

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280508 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

Hi Josh,
on 2015/12/1 9:11, Josh Poimboeuf wrote:
> On Mon, Nov 30, 2015 at 11:54:37AM +0800, Li Bin wrote:
>> There is a potential race as following:
>>
>> CPU0                         |  CPU1
>> -----------------------------|-----------------------------------
>> enabled_store()              |  klp_unregister_patch()
>>                              |  |-mutex_lock(&klp_mutex);
>> |-mutex_lock(&klp_mutex);    |  |-klp_free_patch();
>>                              |  |-mutex_unlock(&klp_mutex);
>> |-[process the patch's state]|
>> |-mutex_unlock(&klp_mutex)   |
>>
>> Fix this race condition by adding klp_is_patch_registered() check in
>> enabled_store() after get the lock klp_mutex.
> I'm thinking this race isn't possible, and that instead it would
> deadlock.

Good point.  I did not consider the kernfs lock.

> When I try to recreate something similar by putting a delay in
> enabled_store(), klp_free_patch() just sleeps on its call to
> kobject_put() until enabled_store() returns.  The unregister stack looks
> like:
>
>   [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
>   [<ffffffff812ea273>] kernfs_remove+0x23/0x40
>   [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
>   [<ffffffff81407fb8>] kobject_del+0x18/0x50
>   [<ffffffff8140804a>] kobject_release+0x5a/0x190
>   [<ffffffff81407f27>] kobject_put+0x27/0x50
>   [<ffffffff81128d41>] klp_unregister_patch+0x71/0x80
>
> It seems to be waiting on a lock which is held by the kernfs code which
> called enabled_store().  So when enabled_store() tries to get the
> klp_mutex, it deadlocks.
>
> Miroslav and I previously discussed a few options to fix this:
>
>  https://lkml.kernel.org/r/alpine.LNX.2.00.1502171728520.29490@pobox.suse.cz

Both the options seem good to me. I look forward to the patch. :)
Thanks,
Li Bin



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280656 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On 12/01/2015, 02:11 AM, Josh Poimboeuf wrote:
> When I try to recreate something similar by putting a delay in
> enabled_store(), klp_free_patch() just sleeps on its call to
> kobject_put() until enabled_store() returns.  The unregister stack looks
> like:
> 
>   [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
>   [<ffffffff812ea273>] kernfs_remove+0x23/0x40
>   [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
>   [<ffffffff81407fb8>] kobject_del+0x18/0x50
>   [<ffffffff8140804a>] kobject_release+0x5a/0x190
>   [<ffffffff81407f27>] kobject_put+0x27/0x50

What about _put outside of klp_mutex in klp_unregister_patch (and maybe
the other _put's as well)? Plus Li Bin's patch.

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280900 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On Tue 2015-12-01 09:50:23, Jiri Slaby wrote:
> On 12/01/2015, 02:11 AM, Josh Poimboeuf wrote:
> > When I try to recreate something similar by putting a delay in
> > enabled_store(), klp_free_patch() just sleeps on its call to
> > kobject_put() until enabled_store() returns.  The unregister stack looks
> > like:
> > 
> >   [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
> >   [<ffffffff812ea273>] kernfs_remove+0x23/0x40
> >   [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
> >   [<ffffffff81407fb8>] kobject_del+0x18/0x50
> >   [<ffffffff8140804a>] kobject_release+0x5a/0x190
> >   [<ffffffff81407f27>] kobject_put+0x27/0x50
> 
> What about _put outside of klp_mutex in klp_unregister_patch (and maybe
> the other _put's as well)? Plus Li Bin's patch.

This might work. But I am pretty sure that we would need to put also
all the other kobject_puts outside of the lock.

I wondered how the approach with mutex_trylock() would look like
and got the patch below.

It is not trivial but it still might be easier than moving all
the kobject_put() calls. Also it should be easier to review
because all the logic is on a single place.

What do you think?

From 3eaec912f2700cd2a886ada1e7b4361ae192ef25 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Tue, 1 Dec 2015 13:25:34 +0100
Subject: [PATCH] livepatch: Avoid deadlock when unregistering and enabling a
 patch

There is a possible deadlock between kobject_put() calls and
enabled_store(), see the lockdep report below.

A solution would be to put all kobject without the klp_mutex
and check if the patch is registered in enabled_store().
But this would make the unregister/free code even more
scattered.

This patch takes the other possible approach. It uses trylock
in enabled_store(). It the lock is not available and the patch
is not registered, it is probably being removed. Anyway, there
is nothing to do and enable_store() returns -EINVAL.
If the lock is not available and the patch is registered,
it tries harder to get it. It uses mutex_is_locked()
in the busy loop to avoid the cache bouncing.

Lockdep report:

[   69.512196] ======================================================
[   69.513139] [ INFO: possible circular locking dependency detected ]
[   69.513437] 4.4.0-rc3-4-default+ #2079 Tainted: G        W   E K
[   69.513437] -------------------------------------------------------
[   69.513437] rmmod/3786 is trying to acquire lock:
[   69.513437]  (s_active#99){++++.+}, at: [<ffffffff8127d4a3>] kernfs_remove+0x23/0x40
[   69.513437]
but task is already holding lock:
[   69.513437]  (klp_mutex){+.+.+.}, at: [<ffffffff810de383>] klp_unregister_patch+0x23/0xc0
[   69.513437]
which lock already depends on the new lock.

[   69.513437]
the existing dependency chain (in reverse order) is:
[   69.513437]
-> #1 (klp_mutex){+.+.+.}:
[   69.513437]        [<ffffffff810bfd5d>] lock_acquire+0xad/0x130
[   69.513437]        [<ffffffff81916f04>] mutex_lock_nested+0x44/0x380
[   69.513437]        [<ffffffff810debe0>] enabled_store+0x50/0xc0
[   69.513437]        [<ffffffff813fa4ef>] kobj_attr_store+0xf/0x20
[   69.513437]        [<ffffffff8127ef44>] sysfs_kf_write+0x44/0x60
[   69.513437]        [<ffffffff8127e564>] kernfs_fop_write+0x144/0x190
[   69.513437]        [<ffffffff811fb648>] __vfs_write+0x28/0xe0
[   69.513437]        [<ffffffff811fbd02>] vfs_write+0xa2/0x1a0
[   69.513437]        [<ffffffff811fca19>] SyS_write+0x49/0xa0
[   69.513437]        [<ffffffff8191a2f2>] entry_SYSCALL_64_fastpath+0x12/0x76
[   69.513437]
-> #0 (s_active#99){++++.+}:
[   69.513437]        [<ffffffff810beead>] __lock_acquire+0x14fd/0x1bd0
[   69.513437]        [<ffffffff810bfd5d>] lock_acquire+0xad/0x130
[   69.513437]        [<ffffffff8127c745>] __kernfs_remove+0x1f5/0x2b0
[   69.513437]        [<ffffffff8127d4a3>] kernfs_remove+0x23/0x40
[   69.513437]        [<ffffffff8127f881>] sysfs_remove_dir+0x51/0x80
[   69.513437]        [<ffffffff813fa6e8>] kobject_del+0x18/0x50
[   69.513437]        [<ffffffff813fa774>] kobject_cleanup+0x54/0x70
[   69.513437]        [<ffffffff813fa645>] kobject_put+0x25/0x50
[   69.513437]        [<ffffffff810de3f8>] klp_unregister_patch+0x98/0xc0
[   69.513437]        [<ffffffffa00000c5>] livepatch_exit+0x25/0xf60 [livepatch_sample]
[   69.513437]        [<ffffffff810fd21c>] SyS_delete_module+0x16c/0x1d0
[   69.513437]        [<ffffffff8191a2f2>] entry_SYSCALL_64_fastpath+0x12/0x76
[   69.513437]
other info that might help us debug this:

[   69.513437]  Possible unsafe locking scenario:

[   69.513437]        CPU0                    CPU1
[   69.513437]        ----                    ----
[   69.513437]   lock(klp_mutex);
[   69.513437]                                lock(s_active#99);
[   69.513437]                                lock(klp_mutex);
[   69.513437]   lock(s_active#99);
[   69.513437]
 *** DEADLOCK ***

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 27712384c69e..e2f00f32bb00 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -612,7 +612,19 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 	patch = container_of(kobj, struct klp_patch, kobj);
 
-	mutex_lock(&klp_mutex);
+	/*
+	 * Avoid a deadlock with kobject_put(&patch->kobj) that is
+	 * called under klp_mutex. Bail out when the patch is not
+	 * longer registered.
+	 */
+	if (!mutex_trylock(&klp_mutex)) {
+		if (!klp_is_patch_registered(patch))
+			return -EINVAL;
+		/* Do not spin with trylock that bounce cache lines. */
+		while (mutex_is_locked(&klp_mutex) &&
+		       klp_is_patch_registered(patch))
+			cond_resched();
+	}
 
 	if (val == patch->state) {
 		/* already in requested state */
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280905 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On 12/01/2015, 03:13 PM, Petr Mladek wrote:
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -612,7 +612,19 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  	patch = container_of(kobj, struct klp_patch, kobj);
>  
> -	mutex_lock(&klp_mutex);
> +	/*
> +	 * Avoid a deadlock with kobject_put(&patch->kobj) that is
> +	 * called under klp_mutex. Bail out when the patch is not
> +	 * longer registered.
> +	 */
> +	if (!mutex_trylock(&klp_mutex)) {

This introduces false positives.
Deleting/enabling/disabling/other_op_under_klp_mutex of an unrelated
patch may now cause enabled_store to fail. Hence I don't like this
approach at all.

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281044 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On Tue 2015-12-01 15:28:19, Jiri Slaby wrote:
> On 12/01/2015, 03:13 PM, Petr Mladek wrote:
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -612,7 +612,19 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  
> >  	patch = container_of(kobj, struct klp_patch, kobj);
> >  
> > -	mutex_lock(&klp_mutex);
> > +	/*
> > +	 * Avoid a deadlock with kobject_put(&patch->kobj) that is
> > +	 * called under klp_mutex. Bail out when the patch is not
> > +	 * longer registered.
> > +	 */
> > +	if (!mutex_trylock(&klp_mutex)) {
>
> This introduces false positives.
> Deleting/enabling/disabling/other_op_under_klp_mutex of an unrelated
> patch may now cause enabled_store to fail. Hence I don't like this
> approach at all.

Ah, there should have been

	while (!mutex_trylock(&klp_mutex)) {
		if (!klp_is_patch_registered(patch))
			return -EINVAL;
		/* Do not spin with trylock that bounce cache lines. */
		while (mutex_is_locked(&klp_mutex) &&
		       klp_is_patch_registered(patch))
			cond_resched();
	}

, so it should not produce false positives.

But I do not have a strong opinion about it.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280982 — Re: [PATCH] livepatch: fix race between enabled_store() and klp_unregister_patch()

On Tue, Dec 01, 2015 at 09:50:23AM +0100, Jiri Slaby wrote:
> On 12/01/2015, 02:11 AM, Josh Poimboeuf wrote:
> > When I try to recreate something similar by putting a delay in
> > enabled_store(), klp_free_patch() just sleeps on its call to
> > kobject_put() until enabled_store() returns.  The unregister stack looks
> > like:
> > 
> >   [<ffffffff812e966b>] __kernfs_remove+0x1fb/0x380
> >   [<ffffffff812ea273>] kernfs_remove+0x23/0x40
> >   [<ffffffff812ec601>] sysfs_remove_dir+0x51/0x80
> >   [<ffffffff81407fb8>] kobject_del+0x18/0x50
> >   [<ffffffff8140804a>] kobject_release+0x5a/0x190
> >   [<ffffffff81407f27>] kobject_put+0x27/0x50
> 
> What about _put outside of klp_mutex in klp_unregister_patch (and maybe
> the other _put's as well)? Plus Li Bin's patch.

This approach sounds the best to me.  I think all _put's for the patch
kobj need to be outside the mutex.  There's also a _put for the patch
kobj in the klp_init_patch() error path which needs to be moved out.

I think the rest of the _put's (for object and func kobjs) are fine as
they are, because they don't have corresponding sysfs functions which
get the mutex.

-- 
Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web