kmod: Optional timeout on the wait in call_usermodehelper_exec

Both kernel/sys.c && security/keys/request_key.c where inlining the exact same
code as call_usermodehelper_fns(); So simply convert these sites to directly use call_usermodehelper_fns().

Signed-off-by Boaz Harrosh <[email protected]>

In the blasphemous occasions that a the Kernel must call a user-mode program half of the times it is more robust to not wait forever but limit the wait for a specified timeout.

So add a new call_usermodehelper_timeout() that implements that. (Users of this new API will be added once this API is in mainline)

call_usermodehelper_fns() is added an extra timeout parameter which is then implemented in call_usermodehelper_exec. The few users of call_usermodehelper_fns() are also changed in this patch.

Since now the executing threads might come back after the waiting thread has returned, do to a timeout. I used a simple kref pattern to govern the life time of the subprocess_info struct.

Should some wait-forever callers today, be converted to this new schema?

CC Trond Myklebust <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exec.c                   |    2 +-
 include/linux/kmod.h        |   15 +++++++++++++--
 kernel/kmod.c               |   29 +++++++++++++++++++++--------
 kernel/sys.c                |    2 +-
 security/keys/request_key.c |    2 +-
 5 files changed, 37 insertions(+), 13 deletions(-)

fs/exec.c +1 -1

index 92ce83a..e696081 100644
@@ -2197,7 +2197,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		}
 
 		retval = call_usermodehelper_fns(helper_argv[0], helper_argv,
-					NULL, UMH_WAIT_EXEC, umh_pipe_setup,
+					NULL, UMH_WAIT_EXEC, 0, umh_pipe_setup,
 					NULL, &cprm);
 		argv_free(helper_argv);
 		if (retval) {

include/linux/kmod.h +13 -2

index 5a89c00..eccc3f5 100644
@@ -25,6 +25,7 @@
 #include <linux/compiler.h>
 #include <linux/workqueue.h>
 #include <linux/sysctl.h>
+#include <linux/kref.h>
 
 #define KMOD_PATH_LEN 256
 

@@ -55,12 +56,14 @@ enum umh_wait {
 };
 
 struct subprocess_info {
+	struct kref kref;
 	struct work_struct work;
 	struct completion *complete;
 	char *path;
 	char **argv;
 	char **envp;
 	enum umh_wait wait;
+	unsigned long wait_timeout;
 	int retval;
 	int (*init)(struct subprocess_info *info, struct cred *new);
 	void (*cleanup)(struct subprocess_info *info);

@@ -69,14 +72,22 @@ struct subprocess_info {
 
 extern int
 call_usermodehelper_fns(char *path, char **argv, char **envp,
-			enum umh_wait wait,
+			enum umh_wait wait, unsigned long timeout,
 			int (*init)(struct subprocess_info *info, struct cred *new),
 			void (*cleanup)(struct subprocess_info *), void *data);
 
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
+	return call_usermodehelper_fns(path, argv, envp, wait, 0,
+				       NULL, NULL, NULL);
+}
+
+static inline int
+call_usermodehelper_timeout(char *path, char **argv, char **envp,
+			    enum umh_wait wait, unsigned long timeout)
+{
+	return call_usermodehelper_fns(path, argv, envp, wait, timeout,
 				       NULL, NULL, NULL);
 }
 

kernel/kmod.c +21 -8

index 3cb7457..a72eefa 100644
@@ -129,7 +129,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	trace_module_request(module_name, wait, _RET_IP_);
 
 	ret = call_usermodehelper_fns(modprobe_path, argv, envp,
-			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC,
+			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC, 0,
 			NULL, NULL, NULL);
 
 	atomic_dec(&kmod_concurrent);

@@ -191,8 +191,11 @@ fail:
 	do_exit(0);
 }
 
-static void call_usermodehelper_freeinfo(struct subprocess_info *info)
+static void call_usermodehelper_freeinfo(struct kref *kref)
 {
+	struct subprocess_info *info =
+		container_of(kref, struct subprocess_info, kref);
+
 	if (info->cleanup)
 		(*info->cleanup)(info);
 	kfree(info);

@@ -235,6 +238,7 @@ static int wait_for_helper(void *data)
 	}
 
 	complete(sub_info->complete);
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 	return 0;
 }
 

@@ -258,7 +262,8 @@ static void __call_usermodehelper(struct work_struct *work)
 
 	switch (wait) {
 	case UMH_NO_WAIT:
-		call_usermodehelper_freeinfo(sub_info);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
 		break;
 
 	case UMH_WAIT_PROC:

@@ -269,6 +274,7 @@ static void __call_usermodehelper(struct work_struct *work)
 		if (pid < 0)
 			sub_info->retval = pid;
 		complete(sub_info->complete);
+		kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 	}
 }
 

@@ -387,6 +393,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	if (!sub_info)
 		goto out;
 
+	kref_init(&sub_info->kref);
 	INIT_WORK(&sub_info->work, __call_usermodehelper);
 	sub_info->path = path;
 	sub_info->argv = argv;

@@ -452,22 +459,27 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info,
 
 	sub_info->complete = &done;
 	sub_info->wait = wait;
+	if (!sub_info->wait_timeout)
+		sub_info->wait_timeout = MAX_SCHEDULE_TIMEOUT;
 
+	/* Balanced in __call_usermodehelper or wait_for_helper */
+	kref_get(&sub_info->kref);
 	queue_work(khelper_wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
-	wait_for_completion(&done);
-	retval = sub_info->retval;
-
+	if (likely(wait_for_completion_timeout(&done, sub_info->wait_timeout)))
+		retval = sub_info->retval;
+	else
+		retval = -ETIMEDOUT;
 out:
-	call_usermodehelper_freeinfo(sub_info);
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 unlock:
 	helper_unlock();
 	return retval;
 }
 
 int call_usermodehelper_fns(char *path, char **argv, char **envp,
-			enum umh_wait wait,
+			enum umh_wait wait, unsigned long timeout,
 			int (*init)(struct subprocess_info *info, struct cred *new),
 			void (*cleanup)(struct subprocess_info *), void *data)
 {

@@ -480,6 +492,7 @@ int call_usermodehelper_fns(char *path, char **argv, char **envp,
 		return -ENOMEM;
 
 	call_usermodehelper_setfns(info, init, cleanup, data);
+	info->wait_timeout = timeout;
 
 	return call_usermodehelper_exec(info, wait);
 }

kernel/sys.c +1 -1

index 9947fb0..a9079d1 100644
@@ -2013,7 +2013,7 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
+	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT, 0,
 				      NULL, argv_cleanup, NULL);
 
   out:

security/keys/request_key.c +0 -0

index b8cc38c..28050ea 100644

In the blasphemous occasions that a the Kernel must call a user-mode program half of the times it is more robust to not wait forever but limit the wait for a specified timeout.

So add a new call_usermodehelper_timeout() that implements that. (Users of this new API will be added once this API is in mainline)

call_usermodehelper_fns() is added an extra timeout parameter which is then implemented in call_usermodehelper_exec. The few users of call_usermodehelper_fns() are also changed in this patch.

Since now the executing threads might come back after the waiting thread has returned, do to a timeout. I used a simple kref pattern to govern the life time of the subprocess_info struct.

Should some wait-forever callers today, be converted to this new schema?

CC Trond Myklebust <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exec.c                   |    2 +-
 include/linux/kmod.h        |   15 +++++++++++++--
 kernel/kmod.c               |   29 +++++++++++++++++++++--------
 kernel/sys.c                |    2 +-
 security/keys/request_key.c |    2 +-
 5 files changed, 37 insertions(+), 13 deletions(-)

fs/exec.c +1 -1

index 92ce83a..e696081 100644
@@ -2197,7 +2197,7 @@ void do_coredump(long signr, int exit_code, struct pt_regs *regs)
 		}
 
 		retval = call_usermodehelper_fns(helper_argv[0], helper_argv,
-					NULL, UMH_WAIT_EXEC, umh_pipe_setup,
+					NULL, UMH_WAIT_EXEC, 0, umh_pipe_setup,
 					NULL, &cprm);
 		argv_free(helper_argv);
 		if (retval) {

include/linux/kmod.h +13 -2

index 5a89c00..eccc3f5 100644
@@ -25,6 +25,7 @@
 #include <linux/compiler.h>
 #include <linux/workqueue.h>
 #include <linux/sysctl.h>
+#include <linux/kref.h>
 
 #define KMOD_PATH_LEN 256
 

@@ -55,12 +56,14 @@ enum umh_wait {
 };
 
 struct subprocess_info {
+	struct kref kref;
 	struct work_struct work;
 	struct completion *complete;
 	char *path;
 	char **argv;
 	char **envp;
 	enum umh_wait wait;
+	unsigned long wait_timeout;
 	int retval;
 	int (*init)(struct subprocess_info *info, struct cred *new);
 	void (*cleanup)(struct subprocess_info *info);

@@ -69,14 +72,22 @@ struct subprocess_info {
 
 extern int
 call_usermodehelper_fns(char *path, char **argv, char **envp,
-			enum umh_wait wait,
+			enum umh_wait wait, unsigned long timeout,
 			int (*init)(struct subprocess_info *info, struct cred *new),
 			void (*cleanup)(struct subprocess_info *), void *data);
 
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
+	return call_usermodehelper_fns(path, argv, envp, wait, 0,
+				       NULL, NULL, NULL);
+}
+
+static inline int
+call_usermodehelper_timeout(char *path, char **argv, char **envp,
+			    enum umh_wait wait, unsigned long timeout)
+{
+	return call_usermodehelper_fns(path, argv, envp, wait, timeout,
 				       NULL, NULL, NULL);
 }
 

kernel/kmod.c +21 -8

index 3cb7457..a72eefa 100644
@@ -129,7 +129,7 @@ int __request_module(bool wait, const char *fmt, ...)
 	trace_module_request(module_name, wait, _RET_IP_);
 
 	ret = call_usermodehelper_fns(modprobe_path, argv, envp,
-			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC,
+			wait ? UMH_WAIT_PROC : UMH_WAIT_EXEC, 0,
 			NULL, NULL, NULL);
 
 	atomic_dec(&kmod_concurrent);

@@ -191,8 +191,11 @@ fail:
 	do_exit(0);
 }
 
-static void call_usermodehelper_freeinfo(struct subprocess_info *info)
+static void call_usermodehelper_freeinfo(struct kref *kref)
 {
+	struct subprocess_info *info =
+		container_of(kref, struct subprocess_info, kref);
+
 	if (info->cleanup)
 		(*info->cleanup)(info);
 	kfree(info);

@@ -235,6 +238,7 @@ static int wait_for_helper(void *data)
 	}
 
 	complete(sub_info->complete);
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 	return 0;
 }
 

@@ -258,7 +262,8 @@ static void __call_usermodehelper(struct work_struct *work)
 
 	switch (wait) {
 	case UMH_NO_WAIT:
-		call_usermodehelper_freeinfo(sub_info);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
 		break;
 
 	case UMH_WAIT_PROC:

@@ -269,6 +274,7 @@ static void __call_usermodehelper(struct work_struct *work)
 		if (pid < 0)
 			sub_info->retval = pid;
 		complete(sub_info->complete);
+		kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 	}
 }
 

@@ -387,6 +393,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	if (!sub_info)
 		goto out;
 
+	kref_init(&sub_info->kref);
 	INIT_WORK(&sub_info->work, __call_usermodehelper);
 	sub_info->path = path;
 	sub_info->argv = argv;

@@ -452,22 +459,27 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info,
 
 	sub_info->complete = &done;
 	sub_info->wait = wait;
+	if (!sub_info->wait_timeout)
+		sub_info->wait_timeout = MAX_SCHEDULE_TIMEOUT;
 
+	/* Balanced in __call_usermodehelper or wait_for_helper */
+	kref_get(&sub_info->kref);
 	queue_work(khelper_wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
-	wait_for_completion(&done);
-	retval = sub_info->retval;
-
+	if (likely(wait_for_completion_timeout(&done, sub_info->wait_timeout)))
+		retval = sub_info->retval;
+	else
+		retval = -ETIMEDOUT;
 out:
-	call_usermodehelper_freeinfo(sub_info);
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 unlock:
 	helper_unlock();
 	return retval;
 }
 
 int call_usermodehelper_fns(char *path, char **argv, char **envp,
-			enum umh_wait wait,
+			enum umh_wait wait, unsigned long timeout,
 			int (*init)(struct subprocess_info *info, struct cred *new),
 			void (*cleanup)(struct subprocess_info *), void *data)
 {

@@ -480,6 +492,7 @@ int call_usermodehelper_fns(char *path, char **argv, char **envp,
 		return -ENOMEM;
 
 	call_usermodehelper_setfns(info, init, cleanup, data);
+	info->wait_timeout = timeout;
 
 	return call_usermodehelper_exec(info, wait);
 }

kernel/sys.c +1 -1

index 9947fb0..a9079d1 100644
@@ -2013,7 +2013,7 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
+	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT, 0,
 				      NULL, argv_cleanup, NULL);
 
   out:

security/keys/request_key.c +0 -0

index b8cc38c..28050ea 100644

Quoted 1 line · Boaz Harrosh

Please review my patchset above, and comment. If you guys want I can combine the two sets into one patchset ASAP. (Will try to finish an RFC tomorrow) Since we are inventing the same mechanisms for slightly different needs. But basically we both need support for the waiter to be returning before the child has completed.

Thanks
Boaz

Quoted 9 lines · Tetsuo Handa

By the way, when considering whether we want call_usermodehelper_timeout() or not, maybe we also want to consider below hung check timer topic.

I noticed that a task waiting for call_usermodehelper(UMH_WAIT_PROC) to return is subjected to hung check timer. call_usermodehelper(UMH_WAIT_PROC) would normally return within few seconds, but it can take longer than hung_task_timeout_secs if the usermode helper is waiting for (e.g.) user's input. Should hung check timer warn this situation?

The fix depends on if we want to enable usermodehelper to wait for user-input ?
we will need to silence the hung check timer

But there is another problem, wait_for_completion/wait_for_completion_killable are not freezer-friendly. Perhaps something like the patch below? Just in case, it should silence check_hung_uninterruptible_tasks() as well.

I rather like Oleg's "use system wq" patch. Seems like a net simplification.

Another issue is that Tejun dislikes the usage of system_unbound_wq. I guess, because WQ_UNBOUND implies WQ_HIGHPRI. Btw, I do not really understand why. And, otoh, I don't think that __call_usermodehelper() should be bound to any CPU, this would look a bit strange to me.

Quoted 7 lines

kernel/kmod.c +2 -0

@@ -452,22 +459,27 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info,
 
 	sub_info->complete = &done;
 	sub_info->wait = wait;
+	if (!sub_info->wait_timeout)
+		sub_info->wait_timeout = MAX_SCHEDULE_TIMEOUT;
 

kernel/kmod.c int call_usermodehelper_exec(struct subprocess_info *sub_info, · line 473

Quoted 12 lines

kernel/kmod.c +6 -3

+	/* Balanced in __call_usermodehelper or wait_for_helper */
+	kref_get(&sub_info->kref);
 	queue_work(khelper_wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
-	wait_for_completion(&done);
-	retval = sub_info->retval;
-
+	if (likely(wait_for_completion_timeout(&done, sub_info->wait_timeout)))
+		retval = sub_info->retval;
+	else
+		retval = -ETIMEDOUT;

Anyway I thought that we are not
suppose to use xhcg() since it is not portable to all ARCHs. ;-)

But basically we both need support for the waiter to be returning before the child has completed.

kernel/kmod.c static void __call_usermodehelper(struct work_struct *work) · line 267

Quoted 9 lines

kernel/kmod.c +2 -1

@@ -258,7 +262,8 @@ static void __call_usermodehelper(struct work_struct *work)
 
 	switch (wait) {
 	case UMH_NO_WAIT:
-		call_usermodehelper_freeinfo(sub_info);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
 		break;

Quoted 18 lines

kernel/kmod.c +7 -3

@@ -452,22 +459,27 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info,
 
 	sub_info->complete = &done;
 	sub_info->wait = wait;
+	if (!sub_info->wait_timeout)
+		sub_info->wait_timeout = MAX_SCHEDULE_TIMEOUT;
 
+	/* Balanced in __call_usermodehelper or wait_for_helper */
+	kref_get(&sub_info->kref);
 	queue_work(khelper_wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
-	wait_for_completion(&done);
-	retval = sub_info->retval;
-
+	if (likely(wait_for_completion_timeout(&done, sub_info->wait_timeout)))
+		retval = sub_info->retval;
+	else

kernel/kmod.c int call_usermodehelper_exec(struct subprocess_info *sub_info, · line 479

Quoted 8 lines

kernel/kmod.c +2 -1

+		retval = -ETIMEDOUT;
 out:
-	call_usermodehelper_freeinfo(sub_info);
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 unlock:
 	helper_unlock();
 	return retval;
 }

Author: Oleg Nesterov <[email protected]>
Date:   Wed Mar 21 10:57:41 2012 +1100

usermodehelper: implement UMH_KILLABLE

Implement UMH_KILLABLE, should be used along with UMH_WAIT_EXEC/PROC. The caller must ensure that subprocess_info->path/etc can not go away until call_usermodehelper_freeinfo().
...

I think that my patch above does a much better/cleaner lifetime management of the subprocess_info struct, with the use of a kref.

Anyway I thought that we are not
suppose to use xhcg() since it is not portable to all ARCHs. ;-)

In any case. I do not know why do we need timeout, but this is orthogonal to KILLABLE. Please redo your patches on top of -mm tree? Please note that in this case the change becomes trivial.

Quoted 3 lines · Boaz Harrosh

Quoted 3 lines

(quoted diff) +0 -0

@@ -258,7 +262,8 @@ static void __call_usermodehelper(struct work_struct *work)
 

Quoted 6 lines

kernel/kmod.c static void __call_usermodehelper(struct work_struct *work) · line 267

Quoted 6 lines

kernel/kmod.c +2 -1

 	switch (wait) {
 	case UMH_NO_WAIT:
-		call_usermodehelper_freeinfo(sub_info);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
+		kref_put(&sub_info->kref,  call_usermodehelper_freeinfo);
 		break;

This doesn't look very nice. If you add the refcounting, it should be consistent. Imho it is better to change call_usermodehelper_exec() so that UMH_NO_WAIT does kref_put() too. Just s/goto unlock/goto out/ afaics.

Quoted 21 lines

Quoted 21 lines

kernel/kmod.c +8 -4

@@ -452,22 +459,27 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info,
 
 	sub_info->complete = &done;
 	sub_info->wait = wait;
+	if (!sub_info->wait_timeout)
+		sub_info->wait_timeout = MAX_SCHEDULE_TIMEOUT;
 
+	/* Balanced in __call_usermodehelper or wait_for_helper */
+	kref_get(&sub_info->kref);
 	queue_work(khelper_wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
-	wait_for_completion(&done);
-	retval = sub_info->retval;
-
+	if (likely(wait_for_completion_timeout(&done, sub_info->wait_timeout)))
+		retval = sub_info->retval;
+	else
+		retval = -ETIMEDOUT;
 out:
-	call_usermodehelper_freeinfo(sub_info);

Quoted 5 lines

kernel/kmod.c int call_usermodehelper_exec(struct subprocess_info *sub_info, · line 479

Quoted 5 lines

kernel/kmod.c +1 -0

+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 unlock:
 	helper_unlock();
 	return retval;
 }

This looks obviously wrong. You also need to move *sub_info->complete into subprocess_info.

Quoted 10 lines

Author: Oleg Nesterov <[email protected]>
Date:   Wed Mar 21 10:57:41 2012 +1100

usermodehelper: implement UMH_KILLABLE

Implement UMH_KILLABLE, should be used along with UMH_WAIT_EXEC/PROC. The caller must ensure that subprocess_info->path/etc can not go away until call_usermodehelper_freeinfo().
...

Quoted 2 lines

I think that my patch above does a much better/cleaner lifetime management of the subprocess_info struct, with the use of a kref.

This is subjective, you know ;) I specially tried to avoid the refcounting.

In any case. I do not know why do we need timeout, but this is orthogonal to KILLABLE. Please redo your patches on top of -mm tree? Please note that in this case the change becomes trivial.

And please explain the use-case for the new API.

Quoted 2 lines

Anyway I thought that we are not
suppose to use xhcg() since it is not portable to all ARCHs. ;-)

Hmm. For example, exit_mm() does xchg().

Oleg.

Quoted 2 lines

And please explain the use-case for the new API.

The reason I need a timeout, is because: Calling from Kernel to user-mode gives me the creeps. I don't trust user-mode programs,

specially when in final Control by a Distribution. Bugs can happen and deadlocks are a possibility.

An operation that should take
1/2 second and could max to at most 1.5 seconds, I can say in confidence that after 15 seconds, a dmesg and a clean error recovery is better.

I don't want any chance of D stating IO operations.
(My code is in the IO path, either fsync or write-back. There is not
always a killable target)

The code path I have is easily recoverable, and if not for the scary message in dmesg the user will not notice.

Quoted 5 lines · Boaz Harrosh

Quoted 2 lines

And please explain the use-case for the new API.

The reason I need a timeout, is because: Calling from Kernel to user-mode gives me the creeps. I don't trust user-mode programs,

If you can't trust user-mode programs executed via call_usermodehelper(), you should not use call_usermodehelper(). It is executed with full privileges. What if the program executed via call_usermodehelper() was

#! /bin/sh
exec /bin/rm -fr /

?

Quoted 2 lines

specially when in final Control by a Distribution. Bugs can happen and deadlocks are a possibility.

Userspace process can be killed at any time.
Deadlock in userspace is less problematic than deadlock in kernel.

Quoted 1 line

An operation that should take

Quoted 3 lines

1/2 second and could max to at most 1.5 seconds, I can say in confidence that after 15 seconds, a dmesg and a clean error recovery is better.

Userspace process can block for long time. For example, under heavy load and memory slashing. It is hardly possible to embed appropriate timeout value into kernel code.

Quoted 3 lines

I don't want any chance of D stating IO operations.
(My code is in the IO path, either fsync or write-back. There is not
always a killable target)

Then, isn't UMH_NO_WAIT better than UMH_WAIT_PROC?

Quoted 2 lines

The code path I have is easily recoverable, and if not for the scary message in dmesg the user will not notice.

What does your code path do if it raced with timeout (i.e. kernel code begins recovery operation (thinking that the request failed) while userspace code completes what the userspace code was asked to do (thinking that the request succeeded))?

I think you should use a fork()ed wrapper in userspace for implementing timeout.

Userspace process - Create child and wait for appropriate timeout and exit.
Child of userspace process - Create grandchild and wait for completion of
grandchild. Tell (or recreate) grandchild to undo what the grandchild was supposed to do if parent dies before grandchild dies.

Grandchild of userspace process - Do what parent told me to do. Undo if
parent told me to undo.

That's fine. I will fail totally gracefully, and nothing will happen. I like this example if the system is under heavy load and there is no memory and the iscsi auto-login takes more then 15 second (Settable by module param) then I'd rather fail the login and revert to plain NFS-MDS IO, instead of the direct osd-target IO. Believe me.

Quoted 6 lines

Quoted 3 lines

I don't want any chance of D stating IO operations.
(My code is in the IO path, either fsync or write-back. There is not
always a killable target)

Then, isn't UMH_NO_WAIT better than UMH_WAIT_PROC?

No I need to wait for the application to finish the iscsi login before I can continue IO to the target. Otherwise what's the point.

Quoted 2 lines · Tetsuo Handa

I think you should use a fork()ed wrapper in userspace for implementing timeout.

I did that actually. But I would like not to be dependent on it. I would like the Kernel to be independent and simple timeout and recover,

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines

usermodehelper: implement UMH_KILLABLE

Implement UMH_KILLABLE, should be used along with UMH_WAIT_EXEC/PROC. The caller must ensure that subprocess_info->path/etc can not go away until call_usermodehelper_freeinfo().
...

I think that my patch above does a much better/cleaner lifetime management of the subprocess_info struct, with the use of a kref.

Quoted 3 lines

This is subjective, you know ;) I specially tried to avoid the refcounting.

Why?

The all kref_ abstraction comes to a simple atomic_inc/dec.

Quoted 4 lines

In any case. I do not know why do we need timeout, but this is orthogonal to KILLABLE. Please redo your patches on top of -mm tree? Please note that in this case the change becomes trivial.

Yes you are right.

Quoted 2 lines

Hmm. For example, exit_mm() does xchg().

Again, Personally I like xchg, but not here, not for an object life-time management. Two threads share a structure, that needs to go when the last one ends.

Is there an mm git tree?

random linux-next/master point. Which should do the job.

Andrew Hi

I'm picking on you because I don't have any one else to pick on. The 3 first patches here, are just good for today. Please see if you would like to take them? or tell me who should take them?

The 4th patch is an RFC, which got me looking into this.

My motivation is that I added yet another Kernel dependency on the call_usermodehelper() function and am not completely happy with the error case of having the user-mode program stuck forever. In such case I would like the Kernel part to timeout and properly error recover and clean up. So therefor the proposed 4th patch.

Quoted 6 lines · Boaz Harrosh

Andrew Hi

I'm picking on you because I don't have any one else to pick on. The 3 first patches here, are just good for today. Please see if you would like to take them? or tell me who should take them?

Quoted 7 lines

The 4th patch is an RFC, which got me looking into this.

My motivation is that I added yet another Kernel dependency on the call_usermodehelper() function and am not completely happy with the error case of having the user-mode program stuck forever. In such case I would like the Kernel part to timeout and properly error recover and clean up. So therefor the proposed 4th patch.

What is this new use of call_usermodhelper that you are doing this work for? Ideally, you never want to make this call, as it's slow and messy, as you have found out. Is there an in-kernel user that you have recently added?

thanks,

greg k-h

Quoted 15 lines · Greg KH

Quoted 6 lines · Boaz Harrosh

Andrew Hi

I'm picking on you because I don't have any one else to pick on. The 3 first patches here, are just good for today. Please see if you would like to take them? or tell me who should take them?

Quoted 7 lines

The 4th patch is an RFC, which got me looking into this.

My motivation is that I added yet another Kernel dependency on the call_usermodehelper() function and am not completely happy with the error case of having the user-mode program stuck forever. In such case I would like the Kernel part to timeout and properly error recover and clean up. So therefor the proposed 4th patch.

Quoted 8 lines

What is this new use of call_usermodhelper that you are doing this work for? Ideally, you never want to make this call, as it's slow and messy, as you have found out. Is there an in-kernel user that you have recently added?

thanks,

greg k-h

I agree hence my comment in the 4th patch:
"In the blasphemous occasions that a the Kernel must call a user-mode program"

I have added a new caller, to the nfs/objectlayoutdriver.ko that uses this facility for auto-login into osd-targets (iscsi-targets) when new are requested by the filesystem. This auto-login facility is mandated by the pnfs-objects standard because in a large cluster filesystems for which pnfs was invented, storage devices break and changed everyday, and a manual login by every client is not feasible.

You can see this patch as posted to the mailing list here:

	http://article.gmane.org/gmane.linux.nfs/48024/match=login
	[title: pnfs-obj: autologin: Add support for protocol autologin]

It works very well and was heavily tested, with all error scenarios, but the theoretical possibility that the user-mode program can be stuck forever bothers me and I would like to do something about it. With this patch the Kernel can recover cleanly and continue. I have actually tested this part and it works as expected.

Quoted 36 lines · Boaz Harrosh

Quoted 15 lines · Greg KH

Quoted 6 lines · Boaz Harrosh

Andrew Hi

I'm picking on you because I don't have any one else to pick on. The 3 first patches here, are just good for today. Please see if you would like to take them? or tell me who should take them?

Quoted 7 lines

The 4th patch is an RFC, which got me looking into this.

My motivation is that I added yet another Kernel dependency on the call_usermodehelper() function and am not completely happy with the error case of having the user-mode program stuck forever. In such case I would like the Kernel part to timeout and properly error recover and clean up. So therefor the proposed 4th patch.

Quoted 8 lines

What is this new use of call_usermodhelper that you are doing this work for? Ideally, you never want to make this call, as it's slow and messy, as you have found out. Is there an in-kernel user that you have recently added?

thanks,

greg k-h

I agree hence my comment in the 4th patch:
"In the blasphemous occasions that a the Kernel must call a user-mode program"

I have added a new caller, to the nfs/objectlayoutdriver.ko that uses this facility for auto-login into osd-targets (iscsi-targets) when new are requested by the filesystem. This auto-login facility is mandated by the pnfs-objects standard because in a large cluster filesystems for which pnfs was invented, storage devices break and changed everyday, and a manual login by every client is not feasible.

You can see this patch as posted to the mailing list here:

Quoted 8 lines

	http://article.gmane.org/gmane.linux.nfs/48024/match=login
	[title: pnfs-obj: autologin: Add support for protocol autologin]

It works very well and was heavily tested, with all error scenarios, but the theoretical possibility that the user-mode program can be stuck forever bothers me and I would like to do something about it. With this patch the Kernel can recover cleanly and continue. I have actually tested this part and it works as expected.

Hi Boaz,

As an alternative suggestion: since you are always calling the same /sbin/osd_login userspace program, wouldn't it be easier to add the timeout smarts into that program? If you can't modify the osd_login program itself, then it should still be trivial to wrap it with a script that adds the 'timeout' command prefix.

Cheers
Trond

kernel/sched/core.c +11 -0

+int __sched
+wait_for_completion_timeout_state(struct completion *x,
+                                 unsigned long timeout, int state)
+{
+       long t;
+       int ret;
+
+       if (!timeout)
+               timeout = MAX_SCHEDULE_TIMEOUT;
+
+       switch (state) {

On Mon, 2012-03-26 at 19:33 -0700, Boaz Harrosh wrote:

So I'm fine with adding wait_for_completion_timeout_state(), but make it look and smell like wait_for_completion_timeout() and use a proper state, like wake_up_state().

IOW:

unsigned long __sched wait_for_completion_timeout_state(struct completion *x,
unsigned long timeout, unsigned int state)

{
	return wait_for_common(x, timeout, state);
}

EXPORT_SYMBOL(wait_for_completion_timeout_state);

Should I add a BUG_ON if state is not one of:
TASK_UNINTERRUPTIBLE
TASK_KILLABLE
TASK_INTERRUPTIBLE

kernel/sched/core.c +7 -0

+int __sched
+wait_for_completion_timeout_state(struct completion *x,
+				  unsigned long timeout, int state)
+{
+	long t;
+	int ret;
+

int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) {

kernel/kmod.c +2 -5

 	DECLARE_COMPLETION_ONSTACK(done);
+	int wait_state;
 	int retval = 0;
 
 	helper_lock();

@@ -540,19 +541,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
 
-	if (wait & UMH_KILLABLE) {
-		retval = wait_for_completion_killable(&done);
-		if (!retval)
-			goto wait_done;
-
+	wait_state = (wait & UMH_KILLABLE) ? TASK_KILLABLE : 0;

Quoted 27 lines · Boaz Harrosh

int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) {

kernel/kmod.c +5 -6

 	DECLARE_COMPLETION_ONSTACK(done);
+	int wait_state;
 	int retval = 0;
 
 	helper_lock();

@@ -540,19 +541,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
 
-	if (wait & UMH_KILLABLE) {
-		retval = wait_for_completion_killable(&done);
-		if (!retval)
-			goto wait_done;
-
+	wait_state = (wait & UMH_KILLABLE) ? TASK_KILLABLE : 0;
+	retval = wait_for_completion_timeout_state(&done, sub_info->timeout,
+						   wait_state);
+	if (unlikely(retval)) {
 		/* umh_complete() will see NULL and free sub_info */
 		if (xchg(&sub_info->complete, NULL))
 			goto unlock;
-		/* fallthrough, umh_complete() was already called */
 	}
 

kernel/kmod.c int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait) · line 554

Quoted 1 line

kernel/kmod.c +0 -1

-	wait_for_completion(&done);

at first glance this looks certainly wrong, or I misread the patch.

We can't remove the "fallback to wait_for_completion" logic until you move the completion into subprocess_info (the next patch seems to do this).

if we use a kref instead of xchg to govern the lifetime of struct subprocess_info, the code becomes simpler and more generic. We can avoid some of the special cases.

Actually I like the xchg use here. But if it will break some ARCHs that do not like xchg, then here is the work needed.

My motivation is that I added yet another Kernel dependency on the call_usermodehelper() function and am not completely happy with the error case of having the user-mode program stuck forever. In such case I would like the Kernel part to timeout and properly error recover and clean up. So therefor the proposed 5th patch.

IOW, please explain at some length why you need this. Do you think that there are existing call sites which can usefully use this feature? Do you expect that new callers are likely to need this? etcetera.

Quoted 5 lines

IOW, please explain at some length why you need this. Do you think that there are existing call sites which can usefully use this feature? Do you expect that new callers are likely to need this? etcetera.

Cough. Can't resist... Could you also explain why
http://marc.info/?l=linux-nfs&m=133252084301205 can't work?

To clarify, I am just curious, I am not arguing. I am asking because

if UMH_WAIT_PROC(timeout) fails with -ETIMEDOUT, then perhaps it makes sense to not "leak" the user-space process servicing the kernel request we were waiting for.

Oleg.