[PATCH 1/2] kobject: don't block for each kobject_uevent

[PATCH 1/2] kobject: don't block for each kobject_uevent

[Vladimir Davydov]

Currently kobject_uevent has somewhat unpredictable semantics. The point
is, since it may call a usermode helper and wait for it to execute
(UMH_WAIT_EXEC), it is impossible to say for sure what lock dependencies
it will introduce for the caller - strictly speaking it depends on what
fs the binary is located on and the set of locks fork may take. There
are quite a few kobject_uevent's users that do not take this into
account and call it with various mutexes taken, e.g. rtnl_mutex,
net_mutex, which might potentially lead to a deadlock.

Since there is actually no reason to wait for the usermode helper to
execute there, let's make kobject_uevent start the helper asynchronously
with the aid of the UMH_NO_WAIT flag.

Personally, I'm interested in this, because I really want kobject_uevent
to be called under the slab_mutex in the slub implementation as it used
to be some time ago, because it greatly simplifies synchronization and
automatically fixes a kmemcg-related race. However, there was a deadlock
detected on an attempt to call kobject_uevent under the slab_mutex (see
https://lkml.org/lkml/2012/1/14/45), which was reported to be fixed by
releasing the slab_mutex for kobject_uevent. Unfortunately, there was no
information about who exactly blocked on the slab_mutex causing the
usermode helper to stall, neither have I managed to find this out or
reproduce the issue.

BTW, this is not the first attempt to make kobject_uevent use
UMH_NO_WAIT. Previous one was made by commit f520360d93c, but it was
wrong (it passed arguments allocated on stack to async thread) so it was
reverted (commit 05f54c13cd0c). It targeted on speeding up the boot
process though.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 include/linux/kobject.h |    1 +
 lib/kobject_uevent.c    |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 926afb6f6b5f..f896a33e8341 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -119,6 +119,7 @@ struct kobj_type {
 };
 
 struct kobj_uevent_env {
+	char *argv[3];
 	char *envp[UEVENT_NUM_ENVP];
 	int envp_idx;
 	char buf[UEVENT_BUFFER_SIZE];
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 5f72767ddd9b..21c39b32e2aa 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -124,6 +124,30 @@ static int kobj_usermode_filter(struct kobject *kobj)
 	return 0;
 }
 
+static int init_uevent_argv(struct kobj_uevent_env *env, const char *subsystem)
+{
+	int len;
+
+	len = strlcpy(&env->buf[env->buflen], subsystem,
+		      sizeof(env->buf) - env->buflen);
+	if (len >= (sizeof(env->buf) - env->buflen)) {
+		WARN(1, KERN_ERR "init_uevent_argv: buffer size too small\n");
+		return -ENOMEM;
+	}
+
+	env->argv[0] = uevent_helper;
+	env->argv[1] = &env->buf[env->buflen];
+	env->argv[2] = NULL;
+
+	env->buflen += len + 1;
+	return 0;
+}
+
+static void cleanup_uevent_env(struct subprocess_info *info)
+{
+	kfree(info->data);
+}
+
 /**
  * kobject_uevent_env - send an uevent with environmental data
  *
@@ -301,11 +325,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 
 	/* call uevent_helper, usually only enabled during early boot */
 	if (uevent_helper[0] && !kobj_usermode_filter(kobj)) {
-		char *argv [3];
+		struct subprocess_info *info;
 
-		argv [0] = uevent_helper;
-		argv [1] = (char *)subsystem;
-		argv [2] = NULL;
 		retval = add_uevent_var(env, "HOME=/");
 		if (retval)
 			goto exit;
@@ -313,9 +334,18 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 					"PATH=/sbin:/bin:/usr/sbin:/usr/bin");
 		if (retval)
 			goto exit;
+		retval = init_uevent_argv(env, subsystem);
+		if (retval)
+			goto exit;
 
-		retval = call_usermodehelper(argv[0], argv,
-					     env->envp, UMH_WAIT_EXEC);
+		retval = -ENOMEM;
+		info = call_usermodehelper_setup(env->argv[0], env->argv,
+						 env->envp, GFP_KERNEL,
+						 NULL, cleanup_uevent_env, env);
+		if (info)
+			retval = call_usermodehelper_exec(info, UMH_NO_WAIT);
+		if (!retval)
+			env = NULL;	/* will be freed by cleanup_uevent_env */
 	}
 
 exit:
-- 
1.7.10.4

[PATCH 2/2] slub: do not drop slab_mutex for sysfs_slab_add

[Vladimir Davydov]

We release the slab_mutex while calling sysfs_slab_add from
__kmem_cache_create since commit 66c4c35c6bc5, because kobject_uevent
called by sysfs_slab_add might block waiting for the usermode helper to
exec, which would result in a deadlock if we took the slab_mutex while
executing it.

However, apart from complicating synchronization rules, releasing the
slab_mutex on kmem cache creation can result in a kmemcg-related race.
The point is that we check if the memcg cache exists before going to
__kmem_cache_create, but register the new cache in memcg subsys after
it. Since we can drop the mutex there, several threads can see that the
memcg cache does not exist and proceed to creating it, which is wrong.

Fortunately, recently kobject_uevent was patched to call the usermode
helper with the UMH_NO_WAIT flag, making the deadlock impossible.
Therefore there is no point in releasing the slab_mutex while calling
sysfs_slab_add, so let's simplify kmem_cache_create synchronization and
fix the kmemcg-race mentioned above by holding the slab_mutex during the
whole cache creation path.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slub.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3d3a8a7a0f8c..0625fed32ce9 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3231,8 +3231,9 @@ int __kmem_cache_shutdown(struct kmem_cache *s)
 
 	if (!rc) {
 		/*
-		 * We do the same lock strategy around sysfs_slab_add, see
-		 * __kmem_cache_create. Because this is pretty much the last
+		 * Since slab_attr_store may take the slab_mutex, we should
+		 * release the lock while removing the sysfs entry in order to
+		 * avoid a deadlock. Because this is pretty much the last
 		 * operation we do and the lock will be released shortly after
 		 * that in slab_common.c, we could just move sysfs_slab_remove
 		 * to a later point in common code. We should do that when we
@@ -3772,10 +3773,7 @@ int __kmem_cache_create(struct kmem_cache *s, unsigned long flags)
 		return 0;
 
 	memcg_propagate_slab_attrs(s);
-	mutex_unlock(&slab_mutex);
 	err = sysfs_slab_add(s);
-	mutex_lock(&slab_mutex);
-
 	if (err)
 		kmem_cache_close(s);
 
-- 
1.7.10.4

Re: [PATCH 2/2] slub: do not drop slab_mutex for sysfs_slab_add

[Christoph Lameter]

On Sun, 9 Feb 2014, Vladimir Davydov wrote:

> Fortunately, recently kobject_uevent was patched to call the usermode
> helper with the UMH_NO_WAIT flag, making the deadlock impossible.

Great!!!! If you can get that other patch merged then

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH 1/2] kobject: don't block for each kobject_uevent

[Andrew Morton]

On Sun, 9 Feb 2014 14:56:15 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:

> Currently kobject_uevent has somewhat unpredictable semantics. The point
> is, since it may call a usermode helper and wait for it to execute
> (UMH_WAIT_EXEC), it is impossible to say for sure what lock dependencies
> it will introduce for the caller - strictly speaking it depends on what
> fs the binary is located on and the set of locks fork may take. There
> are quite a few kobject_uevent's users that do not take this into
> account and call it with various mutexes taken, e.g. rtnl_mutex,
> net_mutex, which might potentially lead to a deadlock.
> 
> Since there is actually no reason to wait for the usermode helper to
> execute there, let's make kobject_uevent start the helper asynchronously
> with the aid of the UMH_NO_WAIT flag.
> 
> Personally, I'm interested in this, because I really want kobject_uevent
> to be called under the slab_mutex in the slub implementation as it used
> to be some time ago, because it greatly simplifies synchronization and
> automatically fixes a kmemcg-related race. However, there was a deadlock
> detected on an attempt to call kobject_uevent under the slab_mutex (see
> https://lkml.org/lkml/2012/1/14/45), which was reported to be fixed by
> releasing the slab_mutex for kobject_uevent. Unfortunately, there was no
> information about who exactly blocked on the slab_mutex causing the
> usermode helper to stall, neither have I managed to find this out or
> reproduce the issue.
> 
> BTW, this is not the first attempt to make kobject_uevent use
> UMH_NO_WAIT. Previous one was made by commit f520360d93c, but it was
> wrong (it passed arguments allocated on stack to async thread) so it was
> reverted (commit 05f54c13cd0c). It targeted on speeding up the boot
> process though.

The patches look good to me.  One is kobject (Greg) and the other is
slub (Pekka), so I grabbed them ;) Reviews-and-acks, please?



btw, when referring to commits, please use the form

f520360d93c ("kobject: don't block for each kobject_uevent")

because the same commit can have different hashes in different trees.

(Although I suspect the amount of convenience this provides others
doesn't match the amount of time I spend fixing changelogs!)

Re: [PATCH 1/2] kobject: don't block for each kobject_uevent

[Vladimir Davydov]

On 02/12/2014 03:03 AM, Andrew Morton wrote:
> On Sun, 9 Feb 2014 14:56:15 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:
>
>> Currently kobject_uevent has somewhat unpredictable semantics. The point
>> is, since it may call a usermode helper and wait for it to execute
>> (UMH_WAIT_EXEC), it is impossible to say for sure what lock dependencies
>> it will introduce for the caller - strictly speaking it depends on what
>> fs the binary is located on and the set of locks fork may take. There
>> are quite a few kobject_uevent's users that do not take this into
>> account and call it with various mutexes taken, e.g. rtnl_mutex,
>> net_mutex, which might potentially lead to a deadlock.
>>
>> Since there is actually no reason to wait for the usermode helper to
>> execute there, let's make kobject_uevent start the helper asynchronously
>> with the aid of the UMH_NO_WAIT flag.
>>
>> Personally, I'm interested in this, because I really want kobject_uevent
>> to be called under the slab_mutex in the slub implementation as it used
>> to be some time ago, because it greatly simplifies synchronization and
>> automatically fixes a kmemcg-related race. However, there was a deadlock
>> detected on an attempt to call kobject_uevent under the slab_mutex (see
>> https://lkml.org/lkml/2012/1/14/45), which was reported to be fixed by
>> releasing the slab_mutex for kobject_uevent. Unfortunately, there was no
>> information about who exactly blocked on the slab_mutex causing the
>> usermode helper to stall, neither have I managed to find this out or
>> reproduce the issue.
>>
>> BTW, this is not the first attempt to make kobject_uevent use
>> UMH_NO_WAIT. Previous one was made by commit f520360d93c, but it was
>> wrong (it passed arguments allocated on stack to async thread) so it was
>> reverted (commit 05f54c13cd0c). It targeted on speeding up the boot
>> process though.
> The patches look good to me.  One is kobject (Greg) and the other is
> slub (Pekka), so I grabbed them ;) Reviews-and-acks, please?
>
>
>
> btw, when referring to commits, please use the form
>
> f520360d93c ("kobject: don't block for each kobject_uevent")
>
> because the same commit can have different hashes in different trees.

Oh, sorry about that. I will take this into account.

Thank you.

>
> (Although I suspect the amount of convenience this provides others
> doesn't match the amount of time I spend fixing changelogs!)
>

Re: [PATCH 1/2] kobject: don't block for each kobject_uevent

[Andrew Morton]

On Sun, 9 Feb 2014 14:56:15 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:

> Currently kobject_uevent has somewhat unpredictable semantics. The point
> is, since it may call a usermode helper and wait for it to execute
> (UMH_WAIT_EXEC), it is impossible to say for sure what lock dependencies
> it will introduce for the caller - strictly speaking it depends on what
> fs the binary is located on and the set of locks fork may take. There
> are quite a few kobject_uevent's users that do not take this into
> account and call it with various mutexes taken, e.g. rtnl_mutex,
> net_mutex, which might potentially lead to a deadlock.
> 
> Since there is actually no reason to wait for the usermode helper to
> execute there, let's make kobject_uevent start the helper asynchronously
> with the aid of the UMH_NO_WAIT flag.
> 
> Personally, I'm interested in this, because I really want kobject_uevent
> to be called under the slab_mutex in the slub implementation as it used
> to be some time ago, because it greatly simplifies synchronization and
> automatically fixes a kmemcg-related race. However, there was a deadlock
> detected on an attempt to call kobject_uevent under the slab_mutex (see
> https://lkml.org/lkml/2012/1/14/45), which was reported to be fixed by
> releasing the slab_mutex for kobject_uevent. Unfortunately, there was no
> information about who exactly blocked on the slab_mutex causing the
> usermode helper to stall, neither have I managed to find this out or
> reproduce the issue.
> 
> BTW, this is not the first attempt to make kobject_uevent use
> UMH_NO_WAIT. Previous one was made by commit f520360d93c, but it was
> wrong (it passed arguments allocated on stack to async thread) so it was
> reverted (commit 05f54c13cd0c). It targeted on speeding up the boot
> process though.

Am not a huge fan of this patch.  My test box gets an early oops in

initcalls
->...
  ->register_pernet_operations
    ->rtnetlink_net_init
      ->__netlink_kernel_create
        ->sock_create_lite
          ->sock_alloc
            ->new_inode_pseudo
              ->alloc_inode+0xe

I expect that sock_mnt->mnt_sb is null.  Or perhaps sb->s_op.  Perhaps
sockfs hasn't mounted yet.

The oops doesn't happen on mainline - it only happens on linux-next. 
So there may be some interaction there, but it may only be timing
related.

config: http://ozlabs.org/~akpm/stuff/config-akpm2

Re: [PATCH 1/2] kobject: don't block for each kobject_uevent

[Vladimir Davydov]

[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]

On 02/13/2014 11:53 PM, Andrew Morton wrote:
> On Sun, 9 Feb 2014 14:56:15 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote:
>
>> Currently kobject_uevent has somewhat unpredictable semantics. The point
>> is, since it may call a usermode helper and wait for it to execute
>> (UMH_WAIT_EXEC), it is impossible to say for sure what lock dependencies
>> it will introduce for the caller - strictly speaking it depends on what
>> fs the binary is located on and the set of locks fork may take. There
>> are quite a few kobject_uevent's users that do not take this into
>> account and call it with various mutexes taken, e.g. rtnl_mutex,
>> net_mutex, which might potentially lead to a deadlock.
>>
>> Since there is actually no reason to wait for the usermode helper to
>> execute there, let's make kobject_uevent start the helper asynchronously
>> with the aid of the UMH_NO_WAIT flag.
>>
>> Personally, I'm interested in this, because I really want kobject_uevent
>> to be called under the slab_mutex in the slub implementation as it used
>> to be some time ago, because it greatly simplifies synchronization and
>> automatically fixes a kmemcg-related race. However, there was a deadlock
>> detected on an attempt to call kobject_uevent under the slab_mutex (see
>> https://lkml.org/lkml/2012/1/14/45), which was reported to be fixed by
>> releasing the slab_mutex for kobject_uevent. Unfortunately, there was no
>> information about who exactly blocked on the slab_mutex causing the
>> usermode helper to stall, neither have I managed to find this out or
>> reproduce the issue.
>>
>> BTW, this is not the first attempt to make kobject_uevent use
>> UMH_NO_WAIT. Previous one was made by commit f520360d93c, but it was
>> wrong (it passed arguments allocated on stack to async thread) so it was
>> reverted (commit 05f54c13cd0c). It targeted on speeding up the boot
>> process though.
> Am not a huge fan of this patch.  My test box gets an early oops in
>
> initcalls
> ->...
>   ->register_pernet_operations
>     ->rtnetlink_net_init
>       ->__netlink_kernel_create
>         ->sock_create_lite
>           ->sock_alloc
>             ->new_inode_pseudo
>               ->alloc_inode+0xe
>
> I expect that sock_mnt->mnt_sb is null.  Or perhaps sb->s_op.  Perhaps
> sockfs hasn't mounted yet.
>
> The oops doesn't happen on mainline - it only happens on linux-next. 
> So there may be some interaction there, but it may only be timing
> related.
>
> config: http://ozlabs.org/~akpm/stuff/config-akpm2

Oh, that's because I missed that call_usermodehelper_exec() calls
cleanup not only on success, but also on failure resulting in a bunch of
double frees at early boot when khelper hasn't been initialized yet :-(

Please sorry for such a silly mistake. The fixed version is attached.

Thank you.

[-- Attachment #2: 0001-kobject-don-t-block-for-each-kobject_uevent.patch --]
[-- Type: text/x-patch, Size: 4673 bytes --]

>From 6f08b7e4b59245f4f5b859991931c74e9fcf713a Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@parallels.com>
Date: Fri, 14 Feb 2014 12:32:13 +0400
Subject: [PATCH v2] kobject: don't block for each kobject_uevent

Currently kobject_uevent has somewhat unpredictable semantics.  The point
is, since it may call a usermode helper and wait for it to execute
(UMH_WAIT_EXEC), it is impossible to say for sure what lock dependencies
it will introduce for the caller - strictly speaking it depends on what fs
the binary is located on and the set of locks fork may take.  There are
quite a few kobject_uevent's users that do not take this into account and
call it with various mutexes taken, e.g.  rtnl_mutex, net_mutex, which
might potentially lead to a deadlock.

Since there is actually no reason to wait for the usermode helper to
execute there, let's make kobject_uevent start the helper asynchronously
with the aid of the UMH_NO_WAIT flag.

Personally, I'm interested in this, because I really want kobject_uevent
to be called under the slab_mutex in the slub implementation as it used to
be some time ago, because it greatly simplifies synchronization and
automatically fixes a kmemcg-related race.  However, there was a deadlock
detected on an attempt to call kobject_uevent under the slab_mutex (see
https://lkml.org/lkml/2012/1/14/45), which was reported to be fixed by
releasing the slab_mutex for kobject_uevent.  Unfortunately, there was no
information about who exactly blocked on the slab_mutex causing the
usermode helper to stall, neither have I managed to find this out or
reproduce the issue.

BTW, this is not the first attempt to make kobject_uevent use UMH_NO_WAIT.
Previous one was made by commit f520360d93c ("kobject: don't block for
each kobject_uevent"), but it was wrong (it passed arguments allocated on
stack to async thread) so it was reverted in 05f54c13cd0c ("Revert
"kobject: don't block for each kobject_uevent".").  It targeted on
speeding up the boot process though.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: Greg KH <greg@kroah.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
---
 v2: fix env double free on call_usermodehelper_exec failure

 include/linux/kobject.h |    1 +
 lib/kobject_uevent.c    |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 926afb6f6b5f..f896a33e8341 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -119,6 +119,7 @@ struct kobj_type {
 };
 
 struct kobj_uevent_env {
+	char *argv[3];
 	char *envp[UEVENT_NUM_ENVP];
 	int envp_idx;
 	char buf[UEVENT_BUFFER_SIZE];
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 5f72767ddd9b..4e3bd71bd949 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -124,6 +124,30 @@ static int kobj_usermode_filter(struct kobject *kobj)
 	return 0;
 }
 
+static int init_uevent_argv(struct kobj_uevent_env *env, const char *subsystem)
+{
+	int len;
+
+	len = strlcpy(&env->buf[env->buflen], subsystem,
+		      sizeof(env->buf) - env->buflen);
+	if (len >= (sizeof(env->buf) - env->buflen)) {
+		WARN(1, KERN_ERR "init_uevent_argv: buffer size too small\n");
+		return -ENOMEM;
+	}
+
+	env->argv[0] = uevent_helper;
+	env->argv[1] = &env->buf[env->buflen];
+	env->argv[2] = NULL;
+
+	env->buflen += len + 1;
+	return 0;
+}
+
+static void cleanup_uevent_env(struct subprocess_info *info)
+{
+	kfree(info->data);
+}
+
 /**
  * kobject_uevent_env - send an uevent with environmental data
  *
@@ -301,11 +325,8 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 
 	/* call uevent_helper, usually only enabled during early boot */
 	if (uevent_helper[0] && !kobj_usermode_filter(kobj)) {
-		char *argv [3];
+		struct subprocess_info *info;
 
-		argv [0] = uevent_helper;
-		argv [1] = (char *)subsystem;
-		argv [2] = NULL;
 		retval = add_uevent_var(env, "HOME=/");
 		if (retval)
 			goto exit;
@@ -313,9 +334,18 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 					"PATH=/sbin:/bin:/usr/sbin:/usr/bin");
 		if (retval)
 			goto exit;
+		retval = init_uevent_argv(env, subsystem);
+		if (retval)
+			goto exit;
 
-		retval = call_usermodehelper(argv[0], argv,
-					     env->envp, UMH_WAIT_EXEC);
+		retval = -ENOMEM;
+		info = call_usermodehelper_setup(env->argv[0], env->argv,
+						 env->envp, GFP_KERNEL,
+						 NULL, cleanup_uevent_env, env);
+		if (info) {
+			retval = call_usermodehelper_exec(info, UMH_NO_WAIT);
+			env = NULL;	/* freed by cleanup_uevent_env */
+		}
 	}
 
 exit:
-- 
1.7.10.4