[PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[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?

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.

It is still in RFC because it is only lightly tested. I have booted
with this patch and tested my use of it, with the new timeout. I have
also dprinted the new reference counting introduced, and it does not
look like we have any reference leaks.
Though I have not seen in dmesgs one of the three possible cases.

If the General Idea is acceptable and there is a chance this gets
accepted, I will conduct heavy unit testing of all the cases and
will resubmit the 4th patch.

So please review and tell me what you guys think.

[I have CCed all these that scripts/get_maintainers.pl suggested
 so sorry for the noise for some]

The list of patches:
[PATCH 1/4] kmod: Unexport call_usermodehelper_freeinfo()
[PATCH 2/4] kmod: kmod: Convert two call sites to call_usermodehelper_fns()
[PATCH 3/4] kmod: kmod: Move call_usermodehelper_fns() to .c file and unexport it's helpers
	
	These 3 are good cleanups, regardless of any new fixtures

[RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

	This patch is my motivation. Please review and comment on the
	new API, as well as the implementation details.

---
 fs/exec.c                   |    2 +-
 include/linux/kmod.h        |   47 +++++++++-----------------------
 kernel/kmod.c               |   62 +++++++++++++++++++++++++++++++++++-------
 kernel/sys.c                |   11 +------
 security/keys/request_key.c |   13 ++-------
 5 files changed, 71 insertions(+), 64 deletions(-)
---

Thanks
Boaz

[PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[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?

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.

It is still in RFC because it is only lightly tested. I have booted
with this patch and tested my use of it, with the new timeout. I have
also dprinted the new reference counting introduced, and it does not
look like we have any reference leaks.
Though I have not seen in dmesgs one of the three possible cases.

If the General Idea is acceptable and there is a chance this gets
accepted, I will conduct heavy unit testing of all the cases and
will resubmit the 4th patch.

So please review and tell me what you guys think.

[I have CCed all these that scripts/get_maintainers.pl suggested
 so sorry for the noise for some]

The list of patches:
[PATCH 1/4] kmod: Unexport call_usermodehelper_freeinfo()
[PATCH 2/4] kmod: kmod: Convert two call sites to call_usermodehelper_fns()
[PATCH 3/4] kmod: kmod: Move call_usermodehelper_fns() to .c file and unexport it's helpers
	
	These 3 are good cleanups, regardless of any new fixtures

[RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

	This patch is my motivation. Please review and comment on the
	new API, as well as the implementation details.

---
 fs/exec.c                   |    2 +-
 include/linux/kmod.h        |   47 +++++++++-----------------------
 kernel/kmod.c               |   62 +++++++++++++++++++++++++++++++++++-------
 kernel/sys.c                |   11 +------
 security/keys/request_key.c |   13 ++-------
 5 files changed, 71 insertions(+), 64 deletions(-)
---

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] kmod: Un-export call_usermodehelper_freeinfo()

[Boaz Harrosh]

call_usermodehelper_freeinfo() is not used outside of kmod.c. So
unexport it, and make it static to kmod.c

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/kmod.h |    4 ----
 kernel/kmod.c        |    3 +--
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 722f477..936d197 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -80,10 +80,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 /* Actually execute the sub-process */
 int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
 
-/* Free the subprocess_info. This is only needed if you're not going
-   to call call_usermodehelper_exec */
-void call_usermodehelper_freeinfo(struct subprocess_info *info);
-
 static inline int
 call_usermodehelper_fns(char *path, char **argv, char **envp,
 			enum umh_wait wait,
diff --git a/kernel/kmod.c b/kernel/kmod.c
index a0a8854..c268f34 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -191,13 +191,12 @@ fail:
 	do_exit(0);
 }
 
-void call_usermodehelper_freeinfo(struct subprocess_info *info)
+static void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
 		(*info->cleanup)(info);
 	kfree(info);
 }
-EXPORT_SYMBOL(call_usermodehelper_freeinfo);
 
 /* Keventd can't block, but this (a child) can. */
 static int wait_for_helper(void *data)
-- 
1.7.6.2

[PATCH 1/4] kmod: Un-export call_usermodehelper_freeinfo()

[Boaz Harrosh]

call_usermodehelper_freeinfo() is not used outside of kmod.c. So
unexport it, and make it static to kmod.c

Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 include/linux/kmod.h |    4 ----
 kernel/kmod.c        |    3 +--
 2 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 722f477..936d197 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -80,10 +80,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 /* Actually execute the sub-process */
 int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
 
-/* Free the subprocess_info. This is only needed if you're not going
-   to call call_usermodehelper_exec */
-void call_usermodehelper_freeinfo(struct subprocess_info *info);
-
 static inline int
 call_usermodehelper_fns(char *path, char **argv, char **envp,
 			enum umh_wait wait,
diff --git a/kernel/kmod.c b/kernel/kmod.c
index a0a8854..c268f34 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -191,13 +191,12 @@ fail:
 	do_exit(0);
 }
 
-void call_usermodehelper_freeinfo(struct subprocess_info *info)
+static void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
 		(*info->cleanup)(info);
 	kfree(info);
 }
-EXPORT_SYMBOL(call_usermodehelper_freeinfo);
 
 /* Keventd can't block, but this (a child) can. */
 static int wait_for_helper(void *data)
-- 
1.7.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] kmod: Convert two call sites to call_usermodehelper_fns()

[Boaz Harrosh]

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 <bharrosh@panasas.com>
---
 kernel/sys.c                |   11 ++---------
 security/keys/request_key.c |   13 +++----------
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 4070153..9947fb0 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2013,15 +2013,8 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	info = call_usermodehelper_setup(argv[0], argv, envp, GFP_ATOMIC);
-	if (info == NULL) {
-		argv_free(argv);
-		goto out;
-	}
-
-	call_usermodehelper_setfns(info, NULL, argv_cleanup, NULL);
-
-	ret = call_usermodehelper_exec(info, UMH_NO_WAIT);
+	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
+				      NULL, argv_cleanup, NULL);
 
   out:
 	if (ret && force) {
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 8246532..b8cc38c 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,16 +93,9 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 			 struct key *session_keyring, enum umh_wait wait)
 {
-	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
-	struct subprocess_info *info =
-		call_usermodehelper_setup(path, argv, envp, gfp_mask);
-
-	if (!info)
-		return -ENOMEM;
-
-	call_usermodehelper_setfns(info, umh_keys_init, umh_keys_cleanup,
-					key_get(session_keyring));
-	return call_usermodehelper_exec(info, wait);
+	return call_usermodehelper_fns(path, argv, envp, wait,
+				       umh_keys_init, umh_keys_cleanup,
+				       key_get(session_keyring));
 }
 
 /*
-- 
1.7.6.2

[PATCH 2/4] kmod: Convert two call sites to call_usermodehelper_fns()

[Boaz Harrosh]

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 <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 kernel/sys.c                |   11 ++---------
 security/keys/request_key.c |   13 +++----------
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 4070153..9947fb0 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2013,15 +2013,8 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	info = call_usermodehelper_setup(argv[0], argv, envp, GFP_ATOMIC);
-	if (info == NULL) {
-		argv_free(argv);
-		goto out;
-	}
-
-	call_usermodehelper_setfns(info, NULL, argv_cleanup, NULL);
-
-	ret = call_usermodehelper_exec(info, UMH_NO_WAIT);
+	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
+				      NULL, argv_cleanup, NULL);
 
   out:
 	if (ret && force) {
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 8246532..b8cc38c 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,16 +93,9 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 			 struct key *session_keyring, enum umh_wait wait)
 {
-	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
-	struct subprocess_info *info =
-		call_usermodehelper_setup(path, argv, envp, gfp_mask);
-
-	if (!info)
-		return -ENOMEM;
-
-	call_usermodehelper_setfns(info, umh_keys_init, umh_keys_cleanup,
-					key_get(session_keyring));
-	return call_usermodehelper_exec(info, wait);
+	return call_usermodehelper_fns(path, argv, envp, wait,
+				       umh_keys_init, umh_keys_cleanup,
+				       key_get(session_keyring));
 }
 
 /*
-- 
1.7.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] kmod: Move call_usermodehelper_fns() to .c file and unexport it's helpers

[Boaz Harrosh]

If we move call_usermodehelper_fns() to kmod.c file and EXPORT_SYMBOL it
we can avoid exporting all it's helper functions:
	call_usermodehelper_setup
	call_usermodehelper_setfns
	call_usermodehelper_exec
And make all of them static to kmod.c

Since the optimizer will see all these as a single call site it will inline them
inside call_usermodehelper_fns(). So we loose the call to _fns but gain 3
calls to the helpers. (Not that it matters)

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/kmod.h |   30 ++----------------------------
 kernel/kmod.c        |   25 ++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 936d197..5a89c00 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -67,37 +67,11 @@ struct subprocess_info {
 	void *data;
 };
 
-/* Allocate a subprocess_info structure */
-struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask);
-
-/* Set various pieces of state into the subprocess_info structure */
-void call_usermodehelper_setfns(struct subprocess_info *info,
-		    int (*init)(struct subprocess_info *info, struct cred *new),
-		    void (*cleanup)(struct subprocess_info *info),
-		    void *data);
-
-/* Actually execute the sub-process */
-int call_usermodehelper_exec(struct subprocess_info *info, enum umh_wait wait);
-
-static inline int
+extern int
 call_usermodehelper_fns(char *path, char **argv, char **envp,
 			enum umh_wait wait,
 			int (*init)(struct subprocess_info *info, struct cred *new),
-			void (*cleanup)(struct subprocess_info *), void *data)
-{
-	struct subprocess_info *info;
-	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
-
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
-
-	if (info == NULL)
-		return -ENOMEM;
-
-	call_usermodehelper_setfns(info, init, cleanup, data);
-
-	return call_usermodehelper_exec(info, wait);
-}
+			void (*cleanup)(struct subprocess_info *), void *data);
 
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
diff --git a/kernel/kmod.c b/kernel/kmod.c
index c268f34..3cb7457 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -378,6 +378,7 @@ static void helper_unlock(void)
  * structure.  This should be passed to call_usermodehelper_exec to
  * exec the process and free the structure.
  */
+static
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 						  char **envp, gfp_t gfp_mask)
 {
@@ -393,7 +394,6 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
   out:
 	return sub_info;
 }
-EXPORT_SYMBOL(call_usermodehelper_setup);
 
 /**
  * call_usermodehelper_setfns - set a cleanup/init function
@@ -411,6 +411,7 @@ EXPORT_SYMBOL(call_usermodehelper_setup);
  * Function must be runnable in either a process context or the
  * context in which call_usermodehelper_exec is called.
  */
+static
 void call_usermodehelper_setfns(struct subprocess_info *info,
 		    int (*init)(struct subprocess_info *info, struct cred *new),
 		    void (*cleanup)(struct subprocess_info *info),
@@ -420,7 +421,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 	info->init = init;
 	info->data = data;
 }
-EXPORT_SYMBOL(call_usermodehelper_setfns);
 
 /**
  * call_usermodehelper_exec - start a usermode application
@@ -434,6 +434,7 @@ EXPORT_SYMBOL(call_usermodehelper_setfns);
  * asynchronously if wait is not set, and runs as a child of keventd.
  * (ie. it runs with full root capabilities).
  */
+static
 int call_usermodehelper_exec(struct subprocess_info *sub_info,
 			     enum umh_wait wait)
 {
@@ -464,7 +465,25 @@ unlock:
 	helper_unlock();
 	return retval;
 }
-EXPORT_SYMBOL(call_usermodehelper_exec);
+
+int call_usermodehelper_fns(char *path, char **argv, char **envp,
+			enum umh_wait wait,
+			int (*init)(struct subprocess_info *info, struct cred *new),
+			void (*cleanup)(struct subprocess_info *), void *data)
+{
+	struct subprocess_info *info;
+	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
+
+	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+
+	if (info == NULL)
+		return -ENOMEM;
+
+	call_usermodehelper_setfns(info, init, cleanup, data);
+
+	return call_usermodehelper_exec(info, wait);
+}
+EXPORT_SYMBOL(call_usermodehelper_fns);
 
 static int proc_cap_handler(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
-- 
1.7.6.2

[RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Boaz Harrosh]

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 <Trond.Myklebust@netapp.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 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(-)

diff --git a/fs/exec.c b/fs/exec.c
index 92ce83a..e696081 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -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) {
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 5a89c00..eccc3f5 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -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);
 }
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 3cb7457..a72eefa 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -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);
 }
diff --git a/kernel/sys.c b/kernel/sys.c
index 9947fb0..a9079d1 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -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:
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index b8cc38c..28050ea 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,7 +93,7 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 			 struct key *session_keyring, enum umh_wait wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
+	return call_usermodehelper_fns(path, argv, envp, wait, 0,
 				       umh_keys_init, umh_keys_cleanup,
 				       key_get(session_keyring));
 }
-- 
1.7.6.2

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Greg KH]

On Tue, Mar 20, 2012 at 04:18:49PM -0700, Boaz Harrosh wrote:
> 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.

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

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Boaz Harrosh]

On 03/21/2012 08:35 AM, Greg KH wrote:
> On Tue, Mar 20, 2012 at 04:18:49PM -0700, Boaz Harrosh wrote:
>> 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.
> 
> 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.

Thanks
Boaz

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Myklebust, Trond]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2773 bytes --]

On Wed, 2012-03-21 at 17:18 -0700, Boaz Harrosh wrote:
> On 03/21/2012 08:35 AM, Greg KH wrote:
> > On Tue, Mar 20, 2012 at 04:18:49PM -0700, Boaz Harrosh wrote:
> >> 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.
> > 
> > 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.

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
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Myklebust, Trond]

On Wed, 2012-03-21 at 17:18 -0700, Boaz Harrosh wrote:
> On 03/21/2012 08:35 AM, Greg KH wrote:
> > On Tue, Mar 20, 2012 at 04:18:49PM -0700, Boaz Harrosh wrote:
> >> 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.
> > 
> > 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.

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
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Myklebust, Trond]

T24gV2VkLCAyMDEyLTAzLTIxIGF0IDE3OjE4IC0wNzAwLCBCb2F6IEhhcnJvc2ggd3JvdGU6DQo+
IE9uIDAzLzIxLzIwMTIgMDg6MzUgQU0sIEdyZWcgS0ggd3JvdGU6DQo+ID4gT24gVHVlLCBNYXIg
MjAsIDIwMTIgYXQgMDQ6MTg6NDlQTSAtMDcwMCwgQm9heiBIYXJyb3NoIHdyb3RlOg0KPiA+PiBB
bmRyZXcgSGkNCj4gPj4NCj4gPj4gSSdtIHBpY2tpbmcgb24geW91IGJlY2F1c2UgSSBkb24ndCBo
YXZlIGFueSBvbmUgZWxzZSB0byBwaWNrIG9uLg0KPiA+PiBUaGUgMyBmaXJzdCBwYXRjaGVzIGhl
cmUsIGFyZSBqdXN0IGdvb2QgZm9yIHRvZGF5LiBQbGVhc2Ugc2VlIGlmDQo+ID4+IHlvdSB3b3Vs
ZCBsaWtlIHRvIHRha2UgdGhlbT8gb3IgdGVsbCBtZSB3aG8gc2hvdWxkIHRha2UgdGhlbT8NCj4g
Pj4NCj4gPj4gVGhlIDR0aCBwYXRjaCBpcyBhbiBSRkMsIHdoaWNoIGdvdCBtZSBsb29raW5nIGlu
dG8gdGhpcy4NCj4gPj4NCj4gPj4gTXkgbW90aXZhdGlvbiBpcyB0aGF0IEkgYWRkZWQgeWV0IGFu
b3RoZXIgS2VybmVsIGRlcGVuZGVuY3kgb24gdGhlDQo+ID4+IGNhbGxfdXNlcm1vZGVoZWxwZXIo
KSBmdW5jdGlvbiBhbmQgYW0gbm90IGNvbXBsZXRlbHkgaGFwcHkgd2l0aCB0aGUNCj4gPj4gZXJy
b3IgY2FzZSBvZiBoYXZpbmcgdGhlIHVzZXItbW9kZSBwcm9ncmFtIHN0dWNrIGZvcmV2ZXIuIElu
IHN1Y2gNCj4gPj4gY2FzZSBJIHdvdWxkIGxpa2UgdGhlIEtlcm5lbCBwYXJ0IHRvIHRpbWVvdXQg
YW5kIHByb3Blcmx5IGVycm9yIHJlY292ZXINCj4gPj4gYW5kIGNsZWFuIHVwLiBTbyB0aGVyZWZv
ciB0aGUgcHJvcG9zZWQgNHRoIHBhdGNoLg0KPiA+IA0KPiA+IFdoYXQgaXMgdGhpcyBuZXcgdXNl
IG9mIGNhbGxfdXNlcm1vZGhlbHBlciB0aGF0IHlvdSBhcmUgZG9pbmcgdGhpcyB3b3JrDQo+ID4g
Zm9yPyAgSWRlYWxseSwgeW91IG5ldmVyIHdhbnQgdG8gbWFrZSB0aGlzIGNhbGwsIGFzIGl0J3Mg
c2xvdyBhbmQgbWVzc3ksDQo+ID4gYXMgeW91IGhhdmUgZm91bmQgb3V0LiAgSXMgdGhlcmUgYW4g
aW4ta2VybmVsIHVzZXIgdGhhdCB5b3UgaGF2ZQ0KPiA+IHJlY2VudGx5IGFkZGVkPw0KPiA+IA0K
PiA+IHRoYW5rcywNCj4gPiANCj4gPiBncmVnIGstaA0KPiANCj4gSSBhZ3JlZSBoZW5jZSBteSBj
b21tZW50IGluIHRoZSA0dGggcGF0Y2g6DQo+IAkiSW4gdGhlIGJsYXNwaGVtb3VzIG9jY2FzaW9u
cyB0aGF0IGEgdGhlIEtlcm5lbCBtdXN0IGNhbGwgYSB1c2VyLW1vZGUgcHJvZ3JhbSINCj4gDQo+
IEkgaGF2ZSBhZGRlZCBhIG5ldyBjYWxsZXIsIHRvIHRoZSBuZnMvb2JqZWN0bGF5b3V0ZHJpdmVy
LmtvIHRoYXQgdXNlcyB0aGlzDQo+IGZhY2lsaXR5IGZvciBhdXRvLWxvZ2luIGludG8gb3NkLXRh
cmdldHMgKGlzY3NpLXRhcmdldHMpIHdoZW4gbmV3IGFyZSByZXF1ZXN0ZWQNCj4gYnkgdGhlIGZp
bGVzeXN0ZW0uIFRoaXMgYXV0by1sb2dpbiBmYWNpbGl0eSBpcyBtYW5kYXRlZCBieSB0aGUgcG5m
cy1vYmplY3RzDQo+IHN0YW5kYXJkIGJlY2F1c2UgaW4gYSBsYXJnZSBjbHVzdGVyIGZpbGVzeXN0
ZW1zIGZvciB3aGljaCBwbmZzIHdhcyBpbnZlbnRlZCwgc3RvcmFnZQ0KPiBkZXZpY2VzIGJyZWFr
IGFuZCBjaGFuZ2VkIGV2ZXJ5ZGF5LCBhbmQgYSBtYW51YWwgbG9naW4gYnkgZXZlcnkgY2xpZW50
IGlzIG5vdA0KPiBmZWFzaWJsZS4NCj4gDQo+IFlvdSBjYW4gc2VlIHRoaXMgcGF0Y2ggYXMgcG9z
dGVkIHRvIHRoZSBtYWlsaW5nIGxpc3QgaGVyZToNCj4gCWh0dHA6Ly9hcnRpY2xlLmdtYW5lLm9y
Zy9nbWFuZS5saW51eC5uZnMvNDgwMjQvbWF0Y2g9bG9naW4NCj4gCVt0aXRsZTogcG5mcy1vYmo6
IGF1dG9sb2dpbjogQWRkIHN1cHBvcnQgZm9yIHByb3RvY29sIGF1dG9sb2dpbl0NCj4gDQo+IEl0
IHdvcmtzIHZlcnkgd2VsbCBhbmQgd2FzIGhlYXZpbHkgdGVzdGVkLCB3aXRoIGFsbCBlcnJvciBz
Y2VuYXJpb3MsIGJ1dA0KPiB0aGUgdGhlb3JldGljYWwgcG9zc2liaWxpdHkgdGhhdCB0aGUgdXNl
ci1tb2RlIHByb2dyYW0gY2FuIGJlIHN0dWNrIGZvcmV2ZXINCj4gYm90aGVycyBtZSBhbmQgSSB3
b3VsZCBsaWtlIHRvIGRvIHNvbWV0aGluZyBhYm91dCBpdC4gV2l0aCB0aGlzIHBhdGNoIHRoZQ0K
PiBLZXJuZWwgY2FuIHJlY292ZXIgY2xlYW5seSBhbmQgY29udGludWUuIEkgaGF2ZSBhY3R1YWxs
eSB0ZXN0ZWQgdGhpcyBwYXJ0DQo+IGFuZCBpdCB3b3JrcyBhcyBleHBlY3RlZC4NCg0KSGkgQm9h
eiwNCg0KQXMgYW4gYWx0ZXJuYXRpdmUgc3VnZ2VzdGlvbjogc2luY2UgeW91IGFyZSBhbHdheXMg
Y2FsbGluZyB0aGUNCnNhbWUgL3NiaW4vb3NkX2xvZ2luIHVzZXJzcGFjZSBwcm9ncmFtLCB3b3Vs
ZG4ndCBpdCBiZSBlYXNpZXIgdG8gYWRkIHRoZQ0KdGltZW91dCBzbWFydHMgaW50byB0aGF0IHBy
b2dyYW0/IElmIHlvdSBjYW4ndCBtb2RpZnkgdGhlIG9zZF9sb2dpbg0KcHJvZ3JhbSBpdHNlbGYs
IHRoZW4gaXQgc2hvdWxkIHN0aWxsIGJlIHRyaXZpYWwgdG8gd3JhcCBpdCB3aXRoIGEgc2NyaXB0
DQp0aGF0IGFkZHMgdGhlICd0aW1lb3V0JyBjb21tYW5kIHByZWZpeC4NCg0KQ2hlZXJzDQogIFRy
b25kDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoN
Ck5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Boaz Harrosh]

On 03/21/2012 05:31 PM, Myklebust, Trond wrote:
> On Wed, 2012-03-21 at 17:18 -0700, Boaz Harrosh wrote:
>> On 03/21/2012 08:35 AM, Greg KH wrote:
>>> On Tue, Mar 20, 2012 at 04:18:49PM -0700, Boaz Harrosh wrote:
>>>> 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.
>>>
>>> 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.
> 
> 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.
> 

That's a good idea. The osd_login is a script supplied by us. And should
be submitted to nfs-utils by Steve. See the 4th patch sent as part of the
login patchset. I will change it to fork and timeout properly.

Regardless, for the long run I would like to pursue the Kernel-side timeout
as well, since that would be the more robust thing to do. Also in the light
that we provide root with the means to register a new osd_login program.

> Cheers
>   Trond

Thanks
Boaz

Re: [PATCHSET 0/4] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Boaz Harrosh]

On 03/21/2012 05:31 PM, Myklebust, Trond wrote:
> On Wed, 2012-03-21 at 17:18 -0700, Boaz Harrosh wrote:
>> On 03/21/2012 08:35 AM, Greg KH wrote:
>>> On Tue, Mar 20, 2012 at 04:18:49PM -0700, Boaz Harrosh wrote:
>>>> 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.
>>>
>>> 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.
> 
> 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.
> 

That's a good idea. The osd_login is a script supplied by us. And should
be submitted to nfs-utils by Steve. See the 4th patch sent as part of the
login patchset. I will change it to fork and timeout properly.

Regardless, for the long run I would like to pursue the Kernel-side timeout
as well, since that would be the more robust thing to do. Also in the light
that we provide root with the means to register a new osd_login program.

> Cheers
>   Trond

Thanks
Boaz

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Boaz Harrosh]

On 03/20/2012 04:32 PM, Boaz Harrosh wrote:
> 
> 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 <Trond.Myklebust@netapp.com>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  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(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 92ce83a..e696081 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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) {
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 5a89c00..eccc3f5 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -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);
>  }
>  
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 3cb7457..a72eefa 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -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);
>  }
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 9947fb0..a9079d1 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -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:
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index b8cc38c..28050ea 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -93,7 +93,7 @@ static void umh_keys_cleanup(struct subprocess_info *info)
>  static int call_usermodehelper_keys(char *path, char **argv, char **envp,
>  			 struct key *session_keyring, enum umh_wait wait)
>  {
> -	return call_usermodehelper_fns(path, argv, envp, wait,
> +	return call_usermodehelper_fns(path, argv, envp, wait, 0,
>  				       umh_keys_init, umh_keys_cleanup,
>  				       key_get(session_keyring));
>  }

Hi Oleg, Tetsuo, and Andrew

Tetsuo Handa has brought to my attention the existence of this patch:
	http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=ccc7cdcf6ecf7d92300dc83315dde0efb2907f39

Author: Oleg Nesterov <oleg@redhat.com>
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().
    
    call_usermodehelper_exec(UMH_KILLABLE) does wait_for_completion_killable.
    If it fails, it uses xchg(&sub_info->complete, NULL) to serialize with
    umh_complete() which does the same xhcg() to access sub_info->complete.
    
    If call_usermodehelper_exec wins, it can safely return.  umh_complete()
    should get NULL and call call_usermodehelper_freeinfo().
    
    Otherwise we know that umh_complete() was already called, in this case
    call_usermodehelper_exec() falls back to wait_for_completion() which
    should succeed "very soon".
    
    Note: UMH_NO_WAIT == -1 but it obviously should not be used with
    UMH_KILLABLE.  We delay the neccessary cleanup to simplify the back
    porting.
    
    Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Cc: Rusty Russell <rusty@rustcorp.com.au>
    Cc: Tejun Heo <tj@kernel.org>
    Cc: David Rientjes <rientjes@google.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

And it's surrounding patchset.

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. ;-)

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

Thanks
Boaz

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Boaz Harrosh]

On 03/20/2012 04:32 PM, Boaz Harrosh wrote:
> 
> 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 <Trond.Myklebust@netapp.com>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  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(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 92ce83a..e696081 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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) {
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 5a89c00..eccc3f5 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -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);
>  }
>  
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 3cb7457..a72eefa 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -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);
>  }
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 9947fb0..a9079d1 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -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:
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index b8cc38c..28050ea 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -93,7 +93,7 @@ static void umh_keys_cleanup(struct subprocess_info *info)
>  static int call_usermodehelper_keys(char *path, char **argv, char **envp,
>  			 struct key *session_keyring, enum umh_wait wait)
>  {
> -	return call_usermodehelper_fns(path, argv, envp, wait,
> +	return call_usermodehelper_fns(path, argv, envp, wait, 0,
>  				       umh_keys_init, umh_keys_cleanup,
>  				       key_get(session_keyring));
>  }

Hi Oleg, Tetsuo, and Andrew

Tetsuo Handa has brought to my attention the existence of this patch:
	http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=ccc7cdcf6ecf7d92300dc83315dde0efb2907f39

Author: Oleg Nesterov <oleg@redhat.com>
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().
    
    call_usermodehelper_exec(UMH_KILLABLE) does wait_for_completion_killable.
    If it fails, it uses xchg(&sub_info->complete, NULL) to serialize with
    umh_complete() which does the same xhcg() to access sub_info->complete.
    
    If call_usermodehelper_exec wins, it can safely return.  umh_complete()
    should get NULL and call call_usermodehelper_freeinfo().
    
    Otherwise we know that umh_complete() was already called, in this case
    call_usermodehelper_exec() falls back to wait_for_completion() which
    should succeed "very soon".
    
    Note: UMH_NO_WAIT == -1 but it obviously should not be used with
    UMH_KILLABLE.  We delay the neccessary cleanup to simplify the back
    porting.
    
    Signed-off-by: Oleg Nesterov <oleg@redhat.com>
    Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Cc: Rusty Russell <rusty@rustcorp.com.au>
    Cc: Tejun Heo <tj@kernel.org>
    Cc: David Rientjes <rientjes@google.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

And it's surrounding patchset.

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. ;-)

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

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Boaz Harrosh]

On 03/21/2012 07:48 PM, Boaz Harrosh wrote:
> On 03/20/2012 04:32 PM, Boaz Harrosh wrote:
>>
<snip>
> 
> 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.
> 

I forgot to ask, what is the official git tree for these patches?

Currently I picked them up from the linux-next/master tree.
Is there a more stable source for these? (Though I can just carry them
as a patchset, I'll need to rebase them anyway)

> Thanks
> Boaz

Re: [PATCH 2/4] kmod: Convert two call sites to call_usermodehelper_fns()

[James Morris]

On Tue, 20 Mar 2012, Boaz Harrosh wrote:

> 
> 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 <bharrosh@panasas.com>

Reviewed-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/4] kmod: Convert two call sites to call_usermodehelper_fns()

[James Morris]

On Tue, 20 Mar 2012, Boaz Harrosh wrote:

> 
> 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 <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>


-- 
James Morris
<jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout()API

[Tetsuo Handa]

Boaz Harrosh wrote:
> > @@ -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;

This patch is incomplete because sub_info->complete refers on-stack variable.
Returning without waiting for completion will overwrite stack memory later.

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

Don't worry. xchg() is used in many places. ;-)
http://tomoyo.sourceforge.jp/cgi-bin/lxr/ident?i=xchg

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

I think basically we should not give up unless fatal events (e.g. SIGKILL or
TIF_MEMDIE) occur. Thus, I feel UMH_KILLABLE is sufficient.

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

On 03/21, Boaz Harrosh wrote:
>
> > @@ -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;

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.

> > @@ -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;
> >  }

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

> Author: Oleg Nesterov <oleg@redhat.com>
> 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.

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.

> 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.

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

On 03/21, Boaz Harrosh wrote:
>
> > @@ -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;

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.

> > @@ -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;
> >  }

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

> Author: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 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.

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.

> 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

BTW,

On 03/22, Oleg Nesterov wrote:
>
> 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.

I have found 1-3 on marc.info, they look fine.

But 2/4 is not exactly correct, it forgets to do argv_free() if
call_usermodehelper_fns() returns -ENOMEM. And I guess you forgot
to remove the unused "struct subprocess_info *info".

Oleg.

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

BTW,

On 03/22, Oleg Nesterov wrote:
>
> 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.

I have found 1-3 on marc.info, they look fine.

But 2/4 is not exactly correct, it forgets to do argv_free() if
call_usermodehelper_fns() returns -ENOMEM. And I guess you forgot
to remove the unused "struct subprocess_info *info".

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Boaz Harrosh]

On 03/22/2012 07:27 AM, Oleg Nesterov wrote:
> On 03/21, Boaz Harrosh wrote:
>>
>>> @@ -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;
> 
> 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.
> 

Yes I've seen this. after I sent the patch. Hence the RFC tag

>>> @@ -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;
>>>  }
> 
> This looks obviously wrong. You also need to move *sub_info->complete
> into subprocess_info.
> 

Yes I caught that with farther testing. A stupid mistake. Again RFC

>> Author: Oleg Nesterov <oleg@redhat.com>
>> 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.
> 
> This is subjective, you know ;) I specially tried to avoid the
> refcounting.
> 

Why? 

The all kref_ abstraction comes to a simple atomic_inc/dec.
Which is in theory a more lite wait operation then xchg, no memory
bus locking, and in practice is the same. (Except on massively
parallel machines which it is)

The last time I submitted a patch with xchg I got clobbered on the head
so strong that I ran away from it as-fast-as-I-could.

For objects life cycle the kref_get/put pattern is a much simpler
more common and understood style in the Kernel, if just for that sake.

I don't see why it needs to be "avoided".

> 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. 

> 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.

So in short it is so I can sleep at night.

>> 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().
> 

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. That's a kref_ abstraction. Kref,
inside, could be implemented with xchg(), But that's not for me to
decide, I should use good abstractions when they exist and do the
job (well). No?

> Oleg.
> 

Thanks Oleg, yes I'll rebase, Is there an mm git tree? I could not
find it on git://git.kernel.org/pub/scm/ . mean while I'll use a
random linux-next/master point. Which should do the job.

Thanks
Boaz

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout()API

[Tetsuo Handa]

Boaz Harrosh wrote:
> > 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 /

?

> 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.

> 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.

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.

> 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?

> 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.

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout()API

[Boaz Harrosh]

On 03/22/2012 03:16 PM, Tetsuo Handa wrote:

> Boaz Harrosh wrote:
>>> 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 /
> 
> ?
> 


You missed my point. I meant unintentional bugs, heavy load, mis-configuration
Administrator mistake. If the admin *wants*  /bin/rm -fr / that's
fine he does not need me.

>> 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.
> 

>> 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.
> 
> 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.
> 


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.

>> 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.

>> 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))?
> 


As I said that's completely fine. Please give me a bit of Credit.
The IO in question would revert to NFS-MDS IO. Since the login succeeded
eventually, the next time the device is looked for it will be found and future
IO will be fast direct to storage. Perfectly fine.

They can race as much as they want. 

> 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, as you said
even the very first execv can timeout in an overloaded system. In that case
I'd like to fail as well and revert to slower IO.

>   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.


We did that here:
	http://thread.gmane.org/gmane.linux.nfs/47921/focus=48182

But again I'd prefer Kernel independence in those matters

Cheers
Boaz

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout()API

[Tetsuo Handa]

Boaz Harrosh wrote:
> 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.

So, race with timeout causes no inconsistency to the kernel.
Then, it will be no problem to have timeout-able version.

> >> 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.

Your code which launches usermodehelper is in the I/O path, isn't it?
Then, don't you need to use GFP_NOFS rather than GFP_KERNEL?
http://lists.infradead.org/pipermail/linux-mtd-cvs/2007-October/005937.html

I'm not familiar with GFP_* flags usage. Can somebody clarify when we need to
use GFP_NOFS rather than GFP_KERNEL by enumerating the name of function (e.g.
xxxfs_writepage()) and/or the name of locks (e.g. xxx_mutex)? I've asked this
in the past http://www.spinics.net/lists/linux-fsdevel/msg30248.html but
explanation with actual function/variable names is easier to understand.

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout()API

[Tetsuo Handa]

Boaz Harrosh wrote:
> 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.

So, race with timeout causes no inconsistency to the kernel.
Then, it will be no problem to have timeout-able version.

> >> 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.

Your code which launches usermodehelper is in the I/O path, isn't it?
Then, don't you need to use GFP_NOFS rather than GFP_KERNEL?
http://lists.infradead.org/pipermail/linux-mtd-cvs/2007-October/005937.html

I'm not familiar with GFP_* flags usage. Can somebody clarify when we need to
use GFP_NOFS rather than GFP_KERNEL by enumerating the name of function (e.g.
xxxfs_writepage()) and/or the name of locks (e.g. xxx_mutex)? I've asked this
in the past http://www.spinics.net/lists/linux-fsdevel/msg30248.html but
explanation with actual function/variable names is easier to understand.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

On 03/22, Boaz Harrosh wrote:
>
> On 03/22/2012 07:27 AM, Oleg Nesterov wrote:
> >>
> >>     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.
> >
> > This is subjective, you know ;) I specially tried to avoid the
> > refcounting.
> >
>
> Why?
>
> The all kref_ abstraction comes to a simple atomic_inc/dec.

Again, this is subjective, but kref_ looks like the unnecessary
complication to me. But I won't insist, see below.

> > 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.

OK, good.

> > 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.

And xchg(info->complete) implements the simplest counter,
xchg() == NULL is equivalent to atomic_dec_and_test() == T.

But again, again, I won't argue if you send the patch which uses
kref_ instead. I do not maintain this code and I do not really mind.
And I never pretended my taste is good ;)

My point is, this is completely orthogonal to "add the timeout",
and if you want to change the refcounting I'd suggest a separate
patch.

What we need is wait_for_completion_state_timeout() to avoid
the horror like

	if (UMH_KILLABLE && !timeout)
		wait_for_completion_killable(...);
	else if (UMH_KILLABLE && timeout)
		wait_for_completion_killable_timeout(...);
	else if (!UMH_KILLABLE && !timeout)
		...

IOW, I think we need to export wait_for_common() first.

This is the only complication afaics. After that "add the timeout"
becomes almost one-liner, with or without "switch to kref_".


> Is there an mm git tree?

No, afaik

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

Yes, I think this should work.

Oleg.

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

On 03/22, Boaz Harrosh wrote:
>
> On 03/22/2012 07:27 AM, Oleg Nesterov wrote:
> >>
> >>     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.
> >
> > This is subjective, you know ;) I specially tried to avoid the
> > refcounting.
> >
>
> Why?
>
> The all kref_ abstraction comes to a simple atomic_inc/dec.

Again, this is subjective, but kref_ looks like the unnecessary
complication to me. But I won't insist, see below.

> > 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.

OK, good.

> > 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.

And xchg(info->complete) implements the simplest counter,
xchg() == NULL is equivalent to atomic_dec_and_test() == T.

But again, again, I won't argue if you send the patch which uses
kref_ instead. I do not maintain this code and I do not really mind.
And I never pretended my taste is good ;)

My point is, this is completely orthogonal to "add the timeout",
and if you want to change the refcounting I'd suggest a separate
patch.

What we need is wait_for_completion_state_timeout() to avoid
the horror like

	if (UMH_KILLABLE && !timeout)
		wait_for_completion_killable(...);
	else if (UMH_KILLABLE && timeout)
		wait_for_completion_killable_timeout(...);
	else if (!UMH_KILLABLE && !timeout)
		...

IOW, I think we need to export wait_for_common() first.

This is the only complication afaics. After that "add the timeout"
becomes almost one-liner, with or without "switch to kref_".


> Is there an mm git tree?

No, afaik

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

Yes, I think this should work.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 4/4] {RFC} kmod.c: Add new call_usermodehelper_timeout()API

[Oleg Nesterov]

On 03/22, Boaz Harrosh wrote:
>
> On 03/22/2012 03:16 PM, Tetsuo Handa wrote:
>
> > 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,

Tetsuo, Boaz, since I do not understand the problem space, I am not
going to discuss the "do we need the timeout" thing.

But, just in case, perhaps there is no need to change kmod.c ? I do
not know if it works for you, but you can simply do something like


	struct kill_work {
		struct delayed_work work;
		struct pid *pid;
	};

	void kill_work_func(struct work_struct *_work)
	{
		struct kill_work *work = container_of(_work, struct kill_work, work);
		kill_pid(work->pid, SIGKILL);
		put_pid(work->pid);
		kfree(work);
	}

	int unh_setup_timeout(struct subprocess_info *info, struct cred *new)
	{
		struct kill_work *work = kmalloc(sizeof(struct kill_work));
		if (!work)
			return -ENOMEM;

		INIT_WORK(&work->work, kill_work_func);
		work->pid = get_pid(task_pid(current));
		schedule_delayed_work(&work->work, (long)info->data);

		return 0;
	}

Now you can do

	call_usermodehelper_fns(init => unh_setup_timeout,
				data => (void*)timeout);

Oleg.

[PATCHSET 0/6 version 2] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Boaz Harrosh]

I'm re-sending the timeout optional waiting in call_usermodehelper()

Diff from version 1:
- Rebased on Olegs patches, based on Linus tree as of [e22057c]
- Fix bugs as noted by Oleg Nesterov, Thanks Oleg for looking into this.
- As noted by Oleg it would be cleaner to export from sched/core a
  new generic wait_for_complition_state() API that encapsulate all the
  different waits in one API. This is the option-A below.
  If the maintainers of kernel/sched/ do not like this API then an option-B
  is presented as a static implementation in kmod.c.
- As requested By Oleg the convert to kref is optional at the very end and
  is independent of the timeout changes.
- Added kernel/sched maintainers to the list (as advised by get_maintainers.pl)

Version 1:
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.

Please review and tell me what you guys think.

List of patches:
[PATCH 1/6] kmod: Unexport call_usermodehelper_freeinfo()

	This one has minor conflicts with Tetsuo's patch which ever is applied first.

[PATCH 2/6] kmod: kmod: Convert two call sites to call_usermodehelper_fns()

	Fix mem leak in the case of -ENOMEM. Tetsuo what was the optimization you
	had for this one?

[PATCH 3/6] kmod: kmod: Move call_usermodehelper_fns() to .c file and unexport all it's helpers
	Good common sense

[PATCH 4/6 OPTION-A] completion: Add new wait_for_completion_timeout_state
[PATCH 4/6 OPTION-B] kmod: {OPTION B} Add new wait_for_completion_timeout_state

	These two are interchangeable. I prefer the first one. I don't think
	kmod will be the only user for long. And it is much cleaner when wait_for_common()
	is used.
	Ingo, Peter, kernel/sched/ people please ack/nack on this approach so we can
	decide how to advance?

[PATCH 5/6] kmod: Add new call_usermodehelper_timeout() API

	And finally my motivation for all this.

[PATCH 6/6] kmod: optional: Convert the use of xchg to a kref

	This is optional. I'm including it here since I already had that code
	when I was not aware of Oleg's xchng patch. whoever is the maintainer of
	this code should decide. I'm signed on the patch but I don't have strong
	feelings one way or the other.

Thanks
Boaz

[PATCH 1/6] kmod: Unexport call_usermodehelper_freeinfo()

[Boaz Harrosh]

call_usermodehelper_freeinfo() is not used outside of kmod.c. So
unexport it, and make it static to kmod.c

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/kmod.h |    4 ----
 kernel/kmod.c        |    2 +-
 2 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 9efeae6..ba89dc2 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -79,10 +79,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 /* Actually execute the sub-process */
 int call_usermodehelper_exec(struct subprocess_info *info, int wait);
 
-/* Free the subprocess_info. This is only needed if you're not going
-   to call call_usermodehelper_exec */
-void call_usermodehelper_freeinfo(struct subprocess_info *info);
-
 static inline int
 call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
 			int (*init)(struct subprocess_info *info, struct cred *new),
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 957a7aa..c5af66a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -221,13 +221,13 @@ static int ____call_usermodehelper(void *data)
 	return 0;
 }
 
+static
 void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
 		(*info->cleanup)(info);
 	kfree(info);
 }
-EXPORT_SYMBOL(call_usermodehelper_freeinfo);
 
 static void umh_complete(struct subprocess_info *sub_info)
 {
-- 
1.7.6.5

[PATCH 1/6] kmod: Unexport call_usermodehelper_freeinfo()

[Boaz Harrosh]

call_usermodehelper_freeinfo() is not used outside of kmod.c. So
unexport it, and make it static to kmod.c

Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 include/linux/kmod.h |    4 ----
 kernel/kmod.c        |    2 +-
 2 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 9efeae6..ba89dc2 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -79,10 +79,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 /* Actually execute the sub-process */
 int call_usermodehelper_exec(struct subprocess_info *info, int wait);
 
-/* Free the subprocess_info. This is only needed if you're not going
-   to call call_usermodehelper_exec */
-void call_usermodehelper_freeinfo(struct subprocess_info *info);
-
 static inline int
 call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
 			int (*init)(struct subprocess_info *info, struct cred *new),
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 957a7aa..c5af66a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -221,13 +221,13 @@ static int ____call_usermodehelper(void *data)
 	return 0;
 }
 
+static
 void call_usermodehelper_freeinfo(struct subprocess_info *info)
 {
 	if (info->cleanup)
 		(*info->cleanup)(info);
 	kfree(info);
 }
-EXPORT_SYMBOL(call_usermodehelper_freeinfo);
 
 static void umh_complete(struct subprocess_info *sub_info)
 {
-- 
1.7.6.5


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] kmod: Convert two call sites to call_usermodehelper_fns()

[Boaz Harrosh]

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 <bharrosh@panasas.com>
---
 kernel/sys.c                |   19 ++++++++-----------
 security/keys/request_key.c |   13 +++----------
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 9eb7fca..37b1971 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2013,7 +2013,6 @@ int orderly_poweroff(bool force)
 		NULL
 	};
 	int ret = -ENOMEM;
-	struct subprocess_info *info;
 
 	if (argv == NULL) {
 		printk(KERN_WARNING "%s failed to allocate memory for \"%s\"\n",
@@ -2021,18 +2020,16 @@ int orderly_poweroff(bool force)
 		goto out;
 	}
 
-	info = call_usermodehelper_setup(argv[0], argv, envp, GFP_ATOMIC);
-	if (info == NULL) {
-		argv_free(argv);
-		goto out;
-	}
-
-	call_usermodehelper_setfns(info, NULL, argv_cleanup, NULL);
+	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
+				      NULL, argv_cleanup, NULL);
+out:
+	if (likely(!ret))
+		return 0;
 
-	ret = call_usermodehelper_exec(info, UMH_NO_WAIT);
+	if (ret == -ENOMEM)
+		argv_free(argv);
 
-  out:
-	if (ret && force) {
+	if (force) {
 		printk(KERN_WARNING "Failed to start orderly shutdown: "
 		       "forcing the issue\n");
 
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index cc37903..000e750 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,16 +93,9 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 					struct key *session_keyring, int wait)
 {
-	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
-	struct subprocess_info *info =
-		call_usermodehelper_setup(path, argv, envp, gfp_mask);
-
-	if (!info)
-		return -ENOMEM;
-
-	call_usermodehelper_setfns(info, umh_keys_init, umh_keys_cleanup,
-					key_get(session_keyring));
-	return call_usermodehelper_exec(info, wait);
+	return call_usermodehelper_fns(path, argv, envp, wait,
+				       umh_keys_init, umh_keys_cleanup,
+				       key_get(session_keyring));
 }
 
 /*
-- 
1.7.6.5

[PATCH 3/6] kmod: Move call_usermodehelper_fns() to .c file and unexport all it's helpers

[Boaz Harrosh]

If we move call_usermodehelper_fns() to kmod.c file and EXPORT_SYMBOL it
we can avoid exporting all it's helper functions:
	call_usermodehelper_setup
	call_usermodehelper_setfns
	call_usermodehelper_exec
And make all of them static to kmod.c

Since the optimizer will see all these as a single call site it will inline them
inside call_usermodehelper_fns(). So we loose the call to _fns but gain 3
calls to the helpers. (Not that it matters)

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/kmod.h |   30 ++----------------------------
 kernel/kmod.c        |   25 ++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index ba89dc2..ae3f9623 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -66,36 +66,10 @@ struct subprocess_info {
 	void *data;
 };
 
-/* Allocate a subprocess_info structure */
-struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
-						  char **envp, gfp_t gfp_mask);
-
-/* Set various pieces of state into the subprocess_info structure */
-void call_usermodehelper_setfns(struct subprocess_info *info,
-		    int (*init)(struct subprocess_info *info, struct cred *new),
-		    void (*cleanup)(struct subprocess_info *info),
-		    void *data);
-
-/* Actually execute the sub-process */
-int call_usermodehelper_exec(struct subprocess_info *info, int wait);
-
-static inline int
+extern int
 call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
 			int (*init)(struct subprocess_info *info, struct cred *new),
-			void (*cleanup)(struct subprocess_info *), void *data)
-{
-	struct subprocess_info *info;
-	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
-
-	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
-
-	if (info == NULL)
-		return -ENOMEM;
-
-	call_usermodehelper_setfns(info, init, cleanup, data);
-
-	return call_usermodehelper_exec(info, wait);
-}
+			void (*cleanup)(struct subprocess_info *), void *data);
 
 static inline int
 call_usermodehelper(char *path, char **argv, char **envp, int wait)
diff --git a/kernel/kmod.c b/kernel/kmod.c
index c5af66a..309299a 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -422,6 +422,7 @@ static void helper_unlock(void)
  * structure.  This should be passed to call_usermodehelper_exec to
  * exec the process and free the structure.
  */
+static
 struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 						  char **envp, gfp_t gfp_mask)
 {
@@ -437,7 +438,6 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
   out:
 	return sub_info;
 }
-EXPORT_SYMBOL(call_usermodehelper_setup);
 
 /**
  * call_usermodehelper_setfns - set a cleanup/init function
@@ -455,6 +455,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
  * Function must be runnable in either a process context or the
  * context in which call_usermodehelper_exec is called.
  */
+static
 void call_usermodehelper_setfns(struct subprocess_info *info,
 		    int (*init)(struct subprocess_info *info, struct cred *new),
 		    void (*cleanup)(struct subprocess_info *info),
@@ -464,7 +465,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 	info->init = init;
 	info->data = data;
 }
-EXPORT_SYMBOL(call_usermodehelper_setfns);
 
 /**
  * call_usermodehelper_exec - start a usermode application
@@ -478,6 +478,7 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
  * asynchronously if wait is not set, and runs as a child of keventd.
  * (ie. it runs with full root capabilities).
  */
+static
 int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 {
 	DECLARE_COMPLETION_ONSTACK(done);
@@ -519,7 +520,25 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	helper_unlock();
 	return retval;
 }
-EXPORT_SYMBOL(call_usermodehelper_exec);
+
+int call_usermodehelper_fns(
+	char *path, char **argv, char **envp, int wait,
+	int (*init)(struct subprocess_info *info, struct cred *new),
+	void (*cleanup)(struct subprocess_info *), void *data)
+{
+	struct subprocess_info *info;
+	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
+
+	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
+
+	if (info == NULL)
+		return -ENOMEM;
+
+	call_usermodehelper_setfns(info, init, cleanup, data);
+
+	return call_usermodehelper_exec(info, wait);
+}
+EXPORT_SYMBOL(call_usermodehelper_fns);
 
 static int proc_cap_handler(struct ctl_table *table, int write,
 			 void __user *buffer, size_t *lenp, loff_t *ppos)
-- 
1.7.6.5

[PATCH 4/6 OPTION-A] completion: Add new wait_for_completion_timeout_state

[Boaz Harrosh]

There are sometime places where an API hides a completion
inside it's implementation, and needs to expose the kind of
wait it does to it's upper users. Instead of such cases that
now need an ugly switch case to call the different
wait_for_completion_xxx function define a generic one
which can do all.

A new user will be added to this API in linux/kmod.h.

The return value from this member is a more Linux Kernel natural.
It will return:
	0  - If the wait was actually completed by a complete() signal.
	-ERESTARTSYS - If interrupted
	-ETIMEDOUT - If timeout expired

CC: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/completion.h |    2 +
 kernel/sched/core.c        |   56 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 51494e6..c41b836 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -85,6 +85,8 @@ extern long wait_for_completion_interruptible_timeout(
 	struct completion *x, unsigned long timeout);
 extern long wait_for_completion_killable_timeout(
 	struct completion *x, unsigned long timeout);
+extern int wait_for_completion_timeout_state(
+	struct completion *x, unsigned long timeout, int state);
 extern bool try_wait_for_completion(struct completion *x);
 extern bool completion_done(struct completion *x);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 503d642..023ba27 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3664,6 +3664,62 @@ int __sched wait_for_completion_killable(struct completion *x)
 EXPORT_SYMBOL(wait_for_completion_killable_timeout);
 
 /**
+ * wait_for_completion_timeout_state: - waits for completion of a task all modes
+ * @x:  holds the state of this particular completion
+ * @timeout:  timeout value in jiffies
+ * @state: Can be either 0 or TASK_KILLABLE or TASK_INTERRUPTIBLE
+ *
+ * This waits for either a completion of a specific task to be
+ * signaled or for a specified timeout to expire.
+ * If @state == TASK_KILLABLE It can be interrupted by a kill signal only.
+ * If @state == TASK_INTERRUPTIBLE It can be interrupted by any signal.
+ *
+ * This is a Swiss-army-knife of all the above, for API's that needs to hide
+ * a completion, and needs to expose it's control to users.
+ *
+ * The return value is:
+ *	0 means completion was signaled with a complete call.
+ *	-ERESTARTSYS if interrupted
+ *	-ETIMEDOUT if timeout expired
+ *  Be careful the return value is Boolean opposite to the above _timeout
+ *  members.
+ */
+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) {
+	default:
+		WARN_ON_ONCE(1);
+		/* fall through */
+	case 0:
+		state = TASK_UNINTERRUPTIBLE;
+		break;
+	case TASK_KILLABLE:
+	case TASK_INTERRUPTIBLE:
+		break;
+	}
+
+	t = wait_for_common(x, timeout, state);
+	if (likely(t > 0)) {
+		ret = 0;
+	} else  {
+		if (t < 0)
+			ret = t;
+		else
+			ret = -ETIMEDOUT;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(wait_for_completion_timeout_state);
+
+/**
  *	try_wait_for_completion - try to decrement a completion without blocking
  *	@x:	completion structure
  *
-- 
1.7.6.5

[PATCH 4/6 OPTION-A] completion: Add new wait_for_completion_timeout_state

[Boaz Harrosh]

There are sometime places where an API hides a completion
inside it's implementation, and needs to expose the kind of
wait it does to it's upper users. Instead of such cases that
now need an ugly switch case to call the different
wait_for_completion_xxx function define a generic one
which can do all.

A new user will be added to this API in linux/kmod.h.

The return value from this member is a more Linux Kernel natural.
It will return:
	0  - If the wait was actually completed by a complete() signal.
	-ERESTARTSYS - If interrupted
	-ETIMEDOUT - If timeout expired

CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 include/linux/completion.h |    2 +
 kernel/sched/core.c        |   56 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 51494e6..c41b836 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -85,6 +85,8 @@ extern long wait_for_completion_interruptible_timeout(
 	struct completion *x, unsigned long timeout);
 extern long wait_for_completion_killable_timeout(
 	struct completion *x, unsigned long timeout);
+extern int wait_for_completion_timeout_state(
+	struct completion *x, unsigned long timeout, int state);
 extern bool try_wait_for_completion(struct completion *x);
 extern bool completion_done(struct completion *x);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 503d642..023ba27 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3664,6 +3664,62 @@ int __sched wait_for_completion_killable(struct completion *x)
 EXPORT_SYMBOL(wait_for_completion_killable_timeout);
 
 /**
+ * wait_for_completion_timeout_state: - waits for completion of a task all modes
+ * @x:  holds the state of this particular completion
+ * @timeout:  timeout value in jiffies
+ * @state: Can be either 0 or TASK_KILLABLE or TASK_INTERRUPTIBLE
+ *
+ * This waits for either a completion of a specific task to be
+ * signaled or for a specified timeout to expire.
+ * If @state == TASK_KILLABLE It can be interrupted by a kill signal only.
+ * If @state == TASK_INTERRUPTIBLE It can be interrupted by any signal.
+ *
+ * This is a Swiss-army-knife of all the above, for API's that needs to hide
+ * a completion, and needs to expose it's control to users.
+ *
+ * The return value is:
+ *	0 means completion was signaled with a complete call.
+ *	-ERESTARTSYS if interrupted
+ *	-ETIMEDOUT if timeout expired
+ *  Be careful the return value is Boolean opposite to the above _timeout
+ *  members.
+ */
+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) {
+	default:
+		WARN_ON_ONCE(1);
+		/* fall through */
+	case 0:
+		state = TASK_UNINTERRUPTIBLE;
+		break;
+	case TASK_KILLABLE:
+	case TASK_INTERRUPTIBLE:
+		break;
+	}
+
+	t = wait_for_common(x, timeout, state);
+	if (likely(t > 0)) {
+		ret = 0;
+	} else  {
+		if (t < 0)
+			ret = t;
+		else
+			ret = -ETIMEDOUT;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(wait_for_completion_timeout_state);
+
+/**
  *	try_wait_for_completion - try to decrement a completion without blocking
  *	@x:	completion structure
  *
-- 
1.7.6.5


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6 option-B] kmod: add new wait_for_completion_timeout_state() helper

[Boaz Harrosh]

This patch is if the more desirable patch is not accepted:

    completion: Add new wait_for_completion_timeout_state

    There are sometime places where an API hides a completion
    inside it's implementation, and needs to expose the kind of
    wait it does to it's upper users. Instead of such cases that
    now need an ugly switch case to call the different
    wait_for_completion_xxx function define a generic one
    which can do all.

    A new user will be added to this API in linux/kmod.h.

    The return value from this member is a more Linux Kernel natural.
    It will return:
    	0  - If the wait was actually completed by a complete() signal.
    	-ERESTARTSYS - If interrupted
    	-ETIMEDOUT - If timeout expired

It can be seen that done here is more complicated/ugly then if
done at kernel/sched/core.c

Note that at this stage there is a warning:
kernel/kmod.c:473:1: warning: 'wait_for_completion_timeout_state' defined but not used [-Wunused-function]

Until the next patch. I keep it separate for the different options
to be taken

CC: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 kernel/kmod.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 309299a..b404f99 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -466,6 +466,46 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 	info->data = data;
 }
 
+/* Used by call_usermodehelper_exec below should be considered for
+ * kernel/sched/core.c
+ */
+static int
+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) {
+	default:
+		WARN_ON_ONCE(1);
+		/* fall through */
+	case 0:
+		wait_for_completion(x);
+		t = 1;
+		break;
+	case TASK_KILLABLE:
+		t = wait_for_completion_killable_timeout(x, timeout);
+		break;
+	case TASK_INTERRUPTIBLE:
+		t = wait_for_completion_interruptible_timeout(x, timeout);
+		break;
+	}
+
+	if (likely(t > 0)) {
+		ret = 0;
+	} else  {
+		if (t < 0)
+			ret = t;
+		else
+			ret = -ETIMEDOUT;
+	}
+	return ret;
+}
+
 /**
  * call_usermodehelper_exec - start a usermode application
  * @sub_info: information about the subprocessa
-- 
1.7.6.5

[PATCH 5/6] kmod: Add new call_usermodehelper_timeout() API

[Boaz Harrosh]

In the blasphemous occasions that 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.

Also an gfp_t parameter is added, for finer allocation control

The code in call_usermodehelper_exec is simplified by the use of
the all-mighty wait_for_completion_timeout_state() which is able
to wait in all the different modes. See:
	completion: Add wait_for_completion_timeout_state()

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

CC: Trond Myklebust <Trond.Myklebust@netapp.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exec.c                   |    4 ++--
 include/linux/kmod.h        |   19 ++++++++++++++-----
 kernel/kmod.c               |   24 ++++++++++++------------
 kernel/sys.c                |    2 +-
 security/keys/request_key.c |    2 +-
 5 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 23559c2..8789ba8 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -2185,8 +2185,8 @@ 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, &cprm);
+					NULL, UMH_WAIT_EXEC, 0, 0,
+					umh_pipe_setup, NULL, &cprm);
 		argv_free(helper_argv);
 		if (retval) {
  			printk(KERN_INFO "Core dump to %s pipe failed\n",
diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index ae3f9623..4d9f202 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -60,21 +60,30 @@ struct subprocess_info {
 	char **argv;
 	char **envp;
 	int wait;
+	unsigned long timeout;
 	int retval;
 	int (*init)(struct subprocess_info *info, struct cred *new);
 	void (*cleanup)(struct subprocess_info *info);
 	void *data;
 };
 
-extern int
-call_usermodehelper_fns(char *path, char **argv, char **envp, int wait,
-			int (*init)(struct subprocess_info *info, struct cred *new),
-			void (*cleanup)(struct subprocess_info *), void *data);
+extern int call_usermodehelper_fns(char *path, char **argv, char **envp,
+	int wait, unsigned long timeout, gfp_t gfp,
+	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, int wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
+	return call_usermodehelper_fns(path, argv, envp, wait, 0, 0,
+				       NULL, NULL, NULL);
+}
+
+static inline int
+call_usermodehelper_timeout(char *path, char **argv, char **envp,
+			    int wait, unsigned long timeout, gfp_t gfp)
+{
+	return call_usermodehelper_fns(path, argv, envp, wait, timeout, gfp,
 				       NULL, NULL, NULL);
 }
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index b404f99..588cb6f 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -90,7 +90,7 @@ static int call_modprobe(char *module_name, int wait)
 	argv[4] = NULL;
 
 	return call_usermodehelper_fns(modprobe_path, argv, envp,
-		wait | UMH_KILLABLE, NULL, free_modprobe_argv, NULL);
+		wait | UMH_KILLABLE, 0, 0, NULL, free_modprobe_argv, NULL);
 free_argv:
 	kfree(argv);
 out:
@@ -522,6 +522,7 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 {
 	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 */
 	}
 
-	wait_for_completion(&done);
-wait_done:
 	retval = sub_info->retval;
 out:
 	call_usermodehelper_freeinfo(sub_info);
@@ -561,13 +558,15 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	return retval;
 }
 
-int call_usermodehelper_fns(
-	char *path, char **argv, char **envp, int wait,
+int call_usermodehelper_fns(char *path, char **argv, char **envp,
+	int wait, unsigned long timeout, gfp_t gfp_mask,
 	int (*init)(struct subprocess_info *info, struct cred *new),
 	void (*cleanup)(struct subprocess_info *), void *data)
 {
 	struct subprocess_info *info;
-	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
+
+	if (!gfp_mask)
+		gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
 	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
 
@@ -575,6 +574,7 @@ int call_usermodehelper_fns(
 		return -ENOMEM;
 
 	call_usermodehelper_setfns(info, init, cleanup, data);
+	info->timeout = timeout;
 
 	return call_usermodehelper_exec(info, wait);
 }
diff --git a/kernel/sys.c b/kernel/sys.c
index 37b1971..16d14d1 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2020,7 +2020,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, 0,
 				      NULL, argv_cleanup, NULL);
 out:
 	if (likely(!ret))
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 000e750..aa94dbb 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -93,7 +93,7 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 					struct key *session_keyring, int wait)
 {
-	return call_usermodehelper_fns(path, argv, envp, wait,
+	return call_usermodehelper_fns(path, argv, envp, wait, 0, 0,
 				       umh_keys_init, umh_keys_cleanup,
 				       key_get(session_keyring));
 }
-- 
1.7.6.5

[PATCH 6/6] kmod: optional: Convert the use of xchg to a kref

[Boaz Harrosh]

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.

No functional change

CC: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/kmod.h |    4 +++-
 kernel/kmod.c        |   43 +++++++++++++++++--------------------------
 2 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 4d9f202..bd0c3c9 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -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
 
@@ -54,8 +55,9 @@ extern __printf(2, 3)
 #define UMH_KILLABLE	4	/* wait for EXEC/PROC killable */
 
 struct subprocess_info {
+	struct kref kref;
 	struct work_struct work;
-	struct completion *complete;
+	struct completion complete;
 	char *path;
 	char **argv;
 	char **envp;
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 588cb6f..948a674 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -221,9 +221,11 @@ static int ____call_usermodehelper(void *data)
 	return 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);
@@ -231,15 +233,8 @@ void call_usermodehelper_freeinfo(struct subprocess_info *info)
 
 static void umh_complete(struct subprocess_info *sub_info)
 {
-	struct completion *comp = xchg(&sub_info->complete, NULL);
-	/*
-	 * See call_usermodehelper_exec(). If xchg() returns NULL
-	 * we own sub_info, the UMH_KILLABLE caller has gone away.
-	 */
-	if (comp)
-		complete(comp);
-	else
-		call_usermodehelper_freeinfo(sub_info);
+	complete(&sub_info->complete);
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 }
 
 /* Keventd can't block, but this (a child) can. */
@@ -302,7 +297,7 @@ 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);
 		break;
 
 	case UMH_WAIT_PROC:
@@ -431,6 +426,8 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	if (!sub_info)
 		goto out;
 
+	kref_init(&sub_info->kref);
+	init_completion(&sub_info->complete);
 	INIT_WORK(&sub_info->work, __call_usermodehelper);
 	sub_info->path = path;
 	sub_info->argv = argv;
@@ -521,7 +518,6 @@ void call_usermodehelper_setfns(struct subprocess_info *info,
 static
 int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 {
-	DECLARE_COMPLETION_ONSTACK(done);
 	int wait_state;
 	int retval = 0;
 
@@ -534,26 +530,21 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 		goto out;
 	}
 
-	sub_info->complete = &done;
 	sub_info->wait = wait;
 
+	/* 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;
+		goto out;
 
 	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;
-	}
-
-	retval = sub_info->retval;
+	retval = wait_for_completion_timeout_state(&sub_info->complete,
+						sub_info->timeout, wait_state);
+	if (likely(!retval))
+		retval = sub_info->retval;
 out:
-	call_usermodehelper_freeinfo(sub_info);
-unlock:
+	kref_put(&sub_info->kref, call_usermodehelper_freeinfo);
 	helper_unlock();
 	return retval;
 }
-- 
1.7.6.5

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Boaz Harrosh]

Sorry had a checkpatch problem near the switch statement (missing space)

Boaz
---
From: Boaz Harrosh <bharrosh@panasas.com>
Subject: [PATCH] completion: Add new wait_for_completion_timeout_state

There are sometime places where an API hides a completion
inside it's implementation, and needs to expose the kind of
wait it does to it's upper users. Instead of such cases that
now need an ugly switch case to call the different
wait_for_completion_xxx function define a generic one
which can do all.

A new user will be added to this API in linux/kmod.h.

The return value from this member is a more Linux Kernel natural.
It will return:
	0  - If the wait was actually completed by a complete() signal.
	-ERESTARTSYS - If interrupted
	-ETIMEDOUT - If timeout expired

CC: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/linux/completion.h |    2 +
 kernel/sched/core.c        |   56 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 51494e6..c41b836 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -85,6 +85,8 @@ extern long wait_for_completion_interruptible_timeout(
 	struct completion *x, unsigned long timeout);
 extern long wait_for_completion_killable_timeout(
 	struct completion *x, unsigned long timeout);
+extern int wait_for_completion_timeout_state(
+	struct completion *x, unsigned long timeout, int state);
 extern bool try_wait_for_completion(struct completion *x);
 extern bool completion_done(struct completion *x);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 503d642..7aef53e 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3664,6 +3664,62 @@ int __sched wait_for_completion_killable(struct completion *x)
 EXPORT_SYMBOL(wait_for_completion_killable_timeout);
 
 /**
+ * wait_for_completion_timeout_state: - waits for completion of a task all modes
+ * @x:  holds the state of this particular completion
+ * @timeout:  timeout value in jiffies
+ * @state: Can be either 0 or TASK_KILLABLE or TASK_INTERRUPTIBLE
+ *
+ * This waits for either a completion of a specific task to be
+ * signaled or for a specified timeout to expire.
+ * If @state == TASK_KILLABLE It can be interrupted by a kill signal only.
+ * If @state == TASK_INTERRUPTIBLE It can be interrupted by any signal.
+ *
+ * This is a Swiss-army-knife of all the above, for API's that needs to hide
+ * a completion, and needs to expose it's control to users.
+ *
+ * The return value is:
+ *	0 means completion was signaled with a complete call.
+ *	-ERESTARTSYS if interrupted
+ *	-ETIMEDOUT if timeout expired
+ *  Be careful the return value is Boolean opposite to the above _timeout
+ *  members.
+ */
+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) {
+	default:
+		WARN_ON_ONCE(1);
+		/* fall through */
+	case 0:
+		state = TASK_UNINTERRUPTIBLE;
+		break;
+	case TASK_KILLABLE:
+	case TASK_INTERRUPTIBLE:
+		break;
+	}
+
+	t = wait_for_common(x, timeout, state);
+	if (likely(t > 0)) {
+		ret = 0;
+	} else  {
+		if (t < 0)
+			ret = t;
+		else
+			ret = -ETIMEDOUT;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(wait_for_completion_timeout_state);
+
+/**
  *	try_wait_for_completion - try to decrement a completion without blocking
  *	@x:	completion structure
  *
-- 
1.7.6.5

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Peter Zijlstra]

On Mon, 2012-03-26 at 19:33 -0700, Boaz Harrosh wrote:
> +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) {
> +       default:
> +               WARN_ON_ONCE(1);
> +               /* fall through */
> +       case 0:
> +               state = TASK_UNINTERRUPTIBLE;
> +               break;
> +       case TASK_KILLABLE:
> +       case TASK_INTERRUPTIBLE:
> +               break;
> +       }

Don't do this, just pass in TASK_UNINTERRUPTIBLE (or TASK_NORMAL)
whatever you like/need.

Don't fudge it by over-loading TASK_RUNNING (aka. 0).

> +       t = wait_for_common(x, timeout, state);
> +       if (likely(t > 0)) {
> +               ret = 0;
> +       } else  {
> +               if (t < 0)
> +                       ret = t;
> +               else
> +                       ret = -ETIMEDOUT;
> +       }
> +       return ret;

Again, why wreck an entirely reasonable return code and break with the
rest of the API.

> +}
> +EXPORT_SYMBOL(wait_for_completion_timeout_state); 


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);

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Peter Zijlstra]

On Mon, 2012-03-26 at 19:33 -0700, Boaz Harrosh wrote:
> +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) {
> +       default:
> +               WARN_ON_ONCE(1);
> +               /* fall through */
> +       case 0:
> +               state = TASK_UNINTERRUPTIBLE;
> +               break;
> +       case TASK_KILLABLE:
> +       case TASK_INTERRUPTIBLE:
> +               break;
> +       }

Don't do this, just pass in TASK_UNINTERRUPTIBLE (or TASK_NORMAL)
whatever you like/need.

Don't fudge it by over-loading TASK_RUNNING (aka. 0).

> +       t = wait_for_common(x, timeout, state);
> +       if (likely(t > 0)) {
> +               ret = 0;
> +       } else  {
> +               if (t < 0)
> +                       ret = t;
> +               else
> +                       ret = -ETIMEDOUT;
> +       }
> +       return ret;

Again, why wreck an entirely reasonable return code and break with the
rest of the API.

> +}
> +EXPORT_SYMBOL(wait_for_completion_timeout_state); 


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);


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] kmod: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

Hi Boaz,

I'll read this series tomorrow, but

On 03/26, Boaz Harrosh wrote:
>
>  int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  {
>  	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 */
>  	}
>
> -	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).

xchg() can race with umh_complete(). If it returns NULL, umh_complete()
was already called and got ->complete != NULL, we must not return until
umh_complete() finishes complete().

Oleg.

Re: [PATCH 5/6] kmod: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

Hi Boaz,

I'll read this series tomorrow, but

On 03/26, Boaz Harrosh wrote:
>
>  int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  {
>  	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 */
>  	}
>
> -	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).

xchg() can race with umh_complete(). If it returns NULL, umh_complete()
was already called and got ->complete != NULL, we must not return until
umh_complete() finishes complete().

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHSET 0/6 version 2] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Andrew Morton]

On Mon, 26 Mar 2012 18:57:54 -0700
Boaz Harrosh <bharrosh@panasas.com> wrote:

> 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.

Before adding a new feature we should hear more about why you think
it's needed.  afaik we haven't needed this capability in the past ten
years or so and may not need it for another ten.  Hence we probably
shouldn't add it!

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.


Patches 1-3 are general cleanups and look good, so I grabbed those.

Re: [PATCHSET 0/6 version 2] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Andrew Morton]

On Mon, 26 Mar 2012 18:57:54 -0700
Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> wrote:

> 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.

Before adding a new feature we should hear more about why you think
it's needed.  afaik we haven't needed this capability in the past ten
years or so and may not need it for another ten.  Hence we probably
shouldn't add it!

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.


Patches 1-3 are general cleanups and look good, so I grabbed those.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] kmod: optional: Convert the use of xchg to a kref

[Oleg Nesterov]

On 03/26, Boaz Harrosh wrote:
>
> 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.

Looks correct, and perhaps this makes the code more understandable.
If we use kref, we have to move the completion into subprocess_info,
but this allows to remove the "fallback to wait_for_completion()"
subtleness. Up to maintainer.

However, I agree with Peter, wait_for_completion_timeout_state()
helper from 4/6 needs more discussion.

And the previous looks patch is wrong, see my reply. The bug goes
away after this patch, but this is not good anyway. IOW, I think
you should redo 5 and 6 even if the resulting code looks correct.

Just one nit below.

> 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.

Well, xchg() has other arch-neutral users. But again, I am not
arguing with this change.

> @@ -302,7 +297,7 @@ 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);
>  		break;

This is minor and subjective, but UMH_NO_WAIT could use umh_complete()
too, the unnecessary complete() doesn't hurt. In this case we could do


	switch (wait) {
	case UMH_WAIT_PROC:
		if (pid > 0)
			break;
		/* FALLTHROUGH */
	case UMH_WAIT_EXEC:
		if (pid < 0)
			sub_info->retval = pid;
		/* FALLTHROUGH */
	case UMH_NO_WAIT:
		umh_complete(sub_info);
	}

Oleg.

Re: [PATCH 5/6] kmod: Add new call_usermodehelper_timeout() API

[Oleg Nesterov]

On 03/27, Oleg Nesterov wrote:
>
> On 03/26, Boaz Harrosh wrote:
> >
> >  int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
> >  {
> >  	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 */
> >  	}
> >
> > -	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).

Yes, I think this is true after I re-checked the code.



One more nit. With this patch call_usermodehelper_fns() does

	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);

	call_usermodehelper_setfns(info, init, cleanup, data);

	info->timeout = timeout;

We have 2 (static!) helpers to initialize info, yet we initialize
->timeout by hand. Of course I do not blame this patch, but imho
this looks a bit messy and deserves another minor cleanup.

Oleg.

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Oleg Nesterov]

On 03/26, Boaz Harrosh wrote:
>
> +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) {
> +	default:
> +		WARN_ON_ONCE(1);
> +		/* fall through */
> +	case 0:
> +		state = TASK_UNINTERRUPTIBLE;
> +		break;

Well, this looks strange, imho. If the caller wants TASK_UNINTERRUPTIBLE,
it should simply pass TASK_UNINTERRUPTIBLE.
wait_for_completion_timeout_state(state => 0) looks confusing, and this
is not symmetrical wrt other states.

> +	t = wait_for_common(x, timeout, state);
> +	if (likely(t > 0)) {
> +		ret = 0;
> +	} else  {
> +		if (t < 0)
> +			ret = t;
> +		else
> +			ret = -ETIMEDOUT;
> +	}
> +	return ret;

I tend to agree with Peter. This is the common helper, probably it
will have more users. We shouldn't throw out the positive return
value, it can be useful.

call_usermodehelper_exec() can simply do

	retval = wait_for_common(...);

	if (retval > 0)
		retval = sub_info->retval;
	else if (!retval)
		retval = -ETIMEDOUT;

Oleg.

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Boaz Harrosh]

On 03/27/2012 01:11 AM, Peter Zijlstra wrote:

> 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);
> 



Looks good thanks It's what I had at first, and went
overboard with it. KISS right?

I'll resend and use it as is? Should I add a BUG_ON
if state is not one of:
	TASK_UNINTERRUPTIBLE
	TASK_KILLABLE
	TASK_INTERRUPTIBLE

Thanks Peter, this will help me a lot
Boaz

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Boaz Harrosh]

On 03/27/2012 01:11 AM, Peter Zijlstra wrote:

> 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);
> 



Looks good thanks It's what I had at first, and went
overboard with it. KISS right?

I'll resend and use it as is? Should I add a BUG_ON
if state is not one of:
	TASK_UNINTERRUPTIBLE
	TASK_KILLABLE
	TASK_INTERRUPTIBLE

Thanks Peter, this will help me a lot
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Peter Zijlstra]

On Wed, 2012-03-28 at 11:19 -0700, Boaz Harrosh wrote:
> Should I add a BUG_ON if state is not one of:
>         TASK_UNINTERRUPTIBLE
>         TASK_KILLABLE
>         TASK_INTERRUPTIBLE 

Nah, it'd be the only such site doing so, seems pointless.

Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state

[Peter Zijlstra]

On Wed, 2012-03-28 at 11:19 -0700, Boaz Harrosh wrote:
> Should I add a BUG_ON if state is not one of:
>         TASK_UNINTERRUPTIBLE
>         TASK_KILLABLE
>         TASK_INTERRUPTIBLE 

Nah, it'd be the only such site doing so, seems pointless.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHSET 0/6 version 2] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Oleg Nesterov]

On 03/27, Andrew Morton wrote:
>
> 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.

Re: [PATCHSET 0/6 version 2] kmod: Optional timeout on the wait in call_usermodehelper_exec

[Boaz Harrosh]

> On 03/27, Andrew Morton wrote:

>>
>> 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.
> 


Andrew Hi.

Yes all these explanations were on the other thread which started
this one. sorry.

On my next attempt I will also add a user for this API as a last
Patch. So you can see exactly how it makes sense, and why I do
need it. When I started this the user was not in the Kernel
yet, but now it's in for the 3.4-rc1, and it can make things clearer.

But in (very) short. The call is made in some special cases at
the read/write path of NFS. I have a very clean way out if
the call fails for any reason, not only timeout, it can fail
and properly handled in many other cases as well.

So early failure does not scare me, what does scare me a lot is
if it will get stuck (deadlocked) forever. This will eventually do
very bad things to the NFS client.

So the very easy thing to do is just add the proper timeout parameter
to existing wait_for_completion calls. If it means some cleanups and code
reorganization met on the way I don't mind.

And yes, I expect that a lot of users that now use UMH_WAIT_PROC
which need to sync with the app, will enjoy the timeout, just as I do.
And all users should be revisited and perhaps enhanced with the extra
robustness this gives.

BTW: Currently the script ran - "osd_login", as submitted
to the nfs-utils package maintainer, has a user-mode "watchdog"
sub-process that will kill the parent after a timeout. But I would not
like to rely on user-mode for this. There are plenty of other things
that can go wrong. I'd like the Kernel to be independent. Also in light
that the script may slightly change from distro to distro and is not
totally in my control.

On 03/28/2012 01:19 PM, Oleg Nesterov wrote:
> Cough. Can't resist... Could you also explain why
> http://marc.info/?l=linux-nfs&m=133252084301205 can't work?
> 


Because it's another thread and another wait object and all bunch of
other allocations, where I already have 3 forks now. I don't do that
because the Politics to get new code in the Kernel is hard.

I'm utilizing all the current resources and am just exposing currently
hard coded constants to upper API's but in effect I'm not degrading
the hot path *at all*. I'll never do what you suggest it's a total waist
when the right way is just doing what is done today, only cleaned and
exposed.

> 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.
> 


Hopefully it is not leaked, right? It will eventually return and de-allocate.
Do you mean that I should kill it? that's an additional mess in kmod.c.

But it's a good point I'll make it killable, and an admin can kill it if it's
really deadlocked forever.

> Oleg.
> 


Thanks
Boaz

call_usermodehelper && check_hung_uninterruptible_tasks

[Oleg Nesterov]

(change subject, add lkml)

On 05/19, Boaz Harrosh wrote:
>
> On 05/19/2012 05:21 AM, Tetsuo Handa wrote:
>
> > 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

tomoyo_load_policy() can use call_usermodehelper(UMH_WAIT_PROC | UMH_KILLABLE),
this should also silence the hung check.


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.

Oleg.

--- x/kernel/kmod.c
+++ x/kernel/kmod.c
@@ -557,7 +557,9 @@ int call_usermodehelper_exec(struct subp
 		goto unlock;
 
 	if (wait & UMH_KILLABLE) {
+		freezer_do_not_count();
 		retval = wait_for_completion_killable(&done);
+		freezer_count();
 		if (!retval)
 			goto wait_done;
 
@@ -567,7 +569,9 @@ int call_usermodehelper_exec(struct subp
 		/* fallthrough, umh_complete() was already called */
 	}
 
+	freezer_do_not_count();
 	wait_for_completion(&done);
+	freezer_count();
 wait_done:
 	retval = sub_info->retval;
 out:

UMH_WAIT_EXEC->UMH_WAIT_PROC deadlock

[Oleg Nesterov]

On 05/21, Rusty Russell wrote:
>
> I rather like Oleg's "use system wq" patch.  Seems like a net
> simplification.

OK. Lets discuss this patch (attached below).

Obviously, I like it too ;) And yes, it looks like a cleanup to me.

But. This change can obviously increase the number of
__call_usermodehelper()'s running in parallel, and in particular increase
the number of workqueue threads.

Is it OK?

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.

So, Tejun, what do you think about this patch? Which system_ wq it
should use if you think it makes sense?

Oleg.


------------------------------------------------------------------------
[PATCH] kmod: kill khelper_wq, fix UMH_WAIT_EXEC->UMH_WAIT_PROC deadlock

A UMH_WAIT_EXEC request can trigger the reqursive UMH_WAIT_PROC
if kernel_execve(sub_info->path) needs request_module() to load
the binfmt module.

This leads to deadlock. The worker thread sleeps waiting for
CLONE_VFORK completion, its child queues another sub_info->work
and waits for it, but since khelper_wq->max_active == 1 no other
work can run.

Quiting Tetsuo:

	The easiest example to observe this deadlock is to use
	a corrupted /sbin/hotplug binary (like shown below).

	# : > /tmp/dummy
	# chmod 755 /tmp/dummy
	# echo /tmp/dummy > /proc/sys/kernel/hotplug
	# modprobe whatever

Kill khelper_wq and use the system wq instead. Workqueues were
greatly improved, I do not think kmod needs the dedicated wq.

In the scenario above, UMH_WAIT_EXEC succeeds with this patch
assuming that the number of UMH_WAIT_EXEC requests in flight
doesn't exceed max_active.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/kmod.h |    2 --
 init/main.c          |    1 -
 kernel/kmod.c        |   14 +++-----------
 3 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 9efeae6..eced4e3 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -110,8 +110,6 @@ call_usermodehelper(char *path, char **argv, char **envp, int wait)
 
 extern struct ctl_table usermodehelper_table[];
 
-extern void usermodehelper_init(void);
-
 extern int usermodehelper_disable(void);
 extern void usermodehelper_enable(void);
 extern bool usermodehelper_is_disabled(void);
diff --git a/init/main.c b/init/main.c
index ff49a6d..f538aa5 100644
--- a/init/main.c
+++ b/init/main.c
@@ -722,7 +722,6 @@ static void __init do_initcalls(void)
 static void __init do_basic_setup(void)
 {
 	cpuset_init_smp();
-	usermodehelper_init();
 	shmem_init();
 	driver_init();
 	init_irq_proc();
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 3a69031..d1712c0 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -43,8 +43,6 @@
 
 extern int max_threads;
 
-static struct workqueue_struct *khelper_wq;
-
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2
 
@@ -285,7 +283,7 @@ static int wait_for_helper(void *data)
 	return 0;
 }
 
-/* This is run by khelper thread  */
+/* This is run by workqueue thread  */
 static void __call_usermodehelper(struct work_struct *work)
 {
 	struct subprocess_info *sub_info =
@@ -494,7 +492,7 @@ pr_crit("UMH: call %16s:%-4d inf=%p w=%d %s\n",
 	if (sub_info->path[0] == '\0')
 		goto out;
 
-	if (!khelper_wq || usermodehelper_disabled) {
+	if (!system_unbound_wq || usermodehelper_disabled) {
 		retval = -EBUSY;
 		goto out;
 	}
@@ -502,7 +500,7 @@ pr_crit("UMH: call %16s:%-4d inf=%p w=%d %s\n",
 	sub_info->complete = &done;
 	sub_info->wait = wait;
 
-	queue_work(khelper_wq, &sub_info->work);
+	queue_work(system_unbound_wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
 
@@ -605,9 +603,3 @@ struct ctl_table usermodehelper_table[] = {
 	},
 	{ }
 };
-
-void __init usermodehelper_init(void)
-{
-	khelper_wq = create_singlethread_workqueue("khelper");
-	BUG_ON(!khelper_wq);
-}
-- 
1.5.5.1

Re: UMH_WAIT_EXEC->UMH_WAIT_PROC deadlock

[Oleg Nesterov]

forgot to mention...

On 05/21, Oleg Nesterov wrote:
>
> 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.

and please note that currently khelper_wq is WQ_UNBOUND anyway.

Oleg.

Re: call_usermodehelper && check_hung_uninterruptible_tasks

[Oleg Nesterov]

On 05/21, Oleg Nesterov wrote:
>
> 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.

Ah, please ignore. I forgot about freeze_processes()->usermodehelper_disable().

Oleg.