[RFC PATCH v5 0/7] Another attempt at contained helper execution

[RFC PATCH v5 0/7] Another attempt at contained helper execution

[Ian Kent]

Following Eric's comments and in light of the most recent nfs and
keys patches here is yet another attempt at the basis of contained
usermode helper execution.

Initially I thought that creating threads to be used when executing
a helper wouldn't be feasible because the number of threads would be
far too large but the recent nfs and keys patches make me think that's
probably not the case.

There's more work to do on this, namely identifying already existing
threads for a requested environment, error handling for environments
that have gone away due to summary execution and similar. But I'd
like to get feedback as to whether I'm on the right track and what I
might be missing before spending more time on it.

---

Ian Kent (7):
      kmod - add workqueue service thread store
      kmod - teach usermodehelper to use service workqueues
      nfsd - use service thread if not executing in init namespace
      nfs - cache_lib use service thread if not executing in init namespace
      nfs - objlayout use service thread if not executing in init namespace
      KEYS - use correct memory allocation flag in call_usermodehelper_keys()
      KEYS: exec request key within service thread of key creator


 fs/nfs/cache_lib.c           |    7 +
 fs/nfs/objlayout/objlayout.c |   14 +++
 fs/nfsd/netns.h              |    3 +
 fs/nfsd/nfs4recover.c        |   48 ++++++---
 fs/nfsd/nfsctl.c             |    6 +
 include/linux/key.h          |    3 +
 include/linux/kmod.h         |    8 ++
 include/linux/sunrpc/cache.h |    2 
 kernel/kmod.c                |  217 ++++++++++++++++++++++++++++++++++++++++--
 net/sunrpc/cache.c           |    5 +
 security/keys/gc.c           |    2 
 security/keys/key.c          |    5 +
 security/keys/request_key.c  |   38 ++++++-
 13 files changed, 323 insertions(+), 35 deletions(-)

--
Ian

[RFC PATCH 5 1/7] kmod - add workqueue service thread store

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

Persistent use of process information is needed for contained
execution in a namespace other than the root init namespace.

Use a simple random token as a key to create and store thread
information in a hashed list for use by the usermode helper
thread runner.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 include/linux/kmod.h |    3 +
 kernel/kmod.c        |  179 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 181 insertions(+), 1 deletion(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 0555cc6..fa46722 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -66,6 +66,9 @@ struct subprocess_info {
 	void *data;
 };
 
+extern int umh_wq_get_token(int token, const char *service);
+extern void umh_wq_put_token(int token);
+
 extern int
 call_usermodehelper(char *path, char **argv, char **envp, int wait);
 
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 2777f40..55d20ce 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -40,13 +40,30 @@
 #include <linux/ptrace.h>
 #include <linux/async.h>
 #include <asm/uaccess.h>
+#include <linux/hash.h>
+#include <linux/list.h>
+#include <linux/random.h>
 
 #include <trace/events/module.h>
 
 extern int max_threads;
 
+#define KHELPER		"khelper"
 static struct workqueue_struct *khelper_wq;
 
+#define UMH_WQ_HASH_SHIFT  6
+#define UMH_WQ_HASH_SIZE   1 << UMH_WQ_HASH_SHIFT
+
+struct umh_wq_entry {
+	int token;
+	unsigned int count;
+	struct workqueue_struct *wq;
+	struct hlist_node umh_wq_hlist;
+};
+
+static DEFINE_SPINLOCK(umh_wq_hash_lock);
+static struct hlist_head umh_wq_hash[UMH_WQ_HASH_SIZE];
+
 #define CAP_BSET	(void *)1
 #define CAP_PI		(void *)2
 
@@ -475,6 +492,165 @@ static void helper_unlock(void)
 		wake_up(&running_helpers_waitq);
 }
 
+static void umh_wq_hash_init(void)
+{
+	int i;
+
+	for (i = 0; i < UMH_WQ_HASH_SIZE; i++)
+		INIT_HLIST_HEAD(&umh_wq_hash[i]);
+}
+
+static struct umh_wq_entry *umh_wq_find_entry(int token)
+{
+	struct umh_wq_entry *this, *entry;
+	struct hlist_head *bucket;
+	unsigned int hash;
+
+	hash = hash_32((unsigned long) token, UMH_WQ_HASH_SHIFT);
+	bucket = &umh_wq_hash[hash];
+
+	entry = ERR_PTR(-ENOENT);
+	if (hlist_empty(bucket))
+		goto out;
+
+	hlist_for_each_entry(this, bucket, umh_wq_hlist) {
+		if (this->token == token) {
+			entry = this;
+			break;
+		}
+	}
+out:
+	return entry;
+}
+
+static struct workqueue_struct *umh_find_wq(int token, unsigned int nowait)
+{
+	struct umh_wq_entry *entry;
+	unsigned long flags;
+
+	if (!token)
+		return khelper_wq;
+
+	if (nowait)
+		spin_lock_irqsave(&umh_wq_hash_lock, flags);
+	else
+		spin_lock(&umh_wq_hash_lock);
+	entry = umh_wq_find_entry(token);
+	if (nowait)
+		spin_unlock_irqrestore(&umh_wq_hash_lock, flags);
+	else
+		spin_unlock(&umh_wq_hash_lock);
+
+	return entry->wq;
+}
+
+/**
+ * umh_wq_get_token - create service thread and return an identifying token
+ * @token: token of an existing service thread or 0 to create a new
+ * 	   service thread.
+ * @name: service name to be appended to "khelper" for identification.
+ *
+ * Returns a token that used with calls to call_usermode_helper_service().
+ * If token corresponds to an existing service thread its reference count
+ * is increased and the token returned. On failure returns a negative errno.
+ */
+int umh_wq_get_token(int token, const char *service)
+{
+	struct workqueue_struct *wq;
+	char *wq_name;
+	int wq_name_len;
+	struct umh_wq_entry *entry;
+	struct hlist_head *bucket;
+	unsigned int hash;
+	unsigned int new_token;
+
+	if (token) {
+		spin_lock(&umh_wq_hash_lock);
+		entry = umh_wq_find_entry(token);
+		if (entry) {
+			entry->count++;
+			spin_unlock(&umh_wq_hash_lock);
+			return token;
+		}
+		spin_unlock(&umh_wq_hash_lock);
+	}
+
+	if (!service)
+		return -EINVAL;
+
+	wq_name_len = sizeof(KHELPER) + strlen(service) + 1;
+	wq_name = kmalloc(wq_name_len, GFP_KERNEL);
+	if (!wq_name)
+		return -ENOMEM;
+	strcpy(wq_name, "KHELPER-");
+	strcat(wq_name, service);
+
+	entry = kzalloc(sizeof(struct umh_wq_entry), GFP_KERNEL);
+	if (!entry) {
+		kfree(wq_name);
+		return -ENOMEM;
+	}
+
+	wq = create_singlethread_workqueue(wq_name);
+	if (IS_ERR(wq)) {
+		kfree(wq_name);
+		kfree(entry);
+		return PTR_ERR(wq);
+	}
+	kfree(wq_name);
+	entry->wq = wq;
+
+	do {
+		new_token = get_random_int();
+		if (!new_token)
+			continue;
+		spin_lock(&umh_wq_hash_lock);
+		entry = umh_wq_find_entry(new_token);
+		if (likely(IS_ERR(entry))) {
+			hash = hash_32(new_token, UMH_WQ_HASH_SHIFT);
+			bucket = &umh_wq_hash[hash];
+			hlist_add_head(&entry->umh_wq_hlist, bucket);
+			entry->token = (long) new_token;
+			spin_unlock(&umh_wq_hash_lock);
+			break;
+		}
+		spin_unlock(&umh_wq_hash_lock);
+	} while (1);
+
+	return (int) new_token;
+}
+EXPORT_SYMBOL(umh_wq_get_token);
+
+/**
+ * umh_ns_put_token - stop a service thread if it's not longer in use
+ * 		      and remove it from the serive thread list
+ * @token: token of a service thread.
+ */
+void umh_wq_put_token(int token)
+{
+	struct umh_wq_entry *entry;
+
+	if (!token)
+		return;
+
+	spin_lock(&umh_wq_hash_lock);
+	entry = umh_wq_find_entry(token);
+	if (unlikely(IS_ERR(entry)))
+		spin_unlock(&umh_wq_hash_lock);
+	else {
+		if (--entry->count)
+			spin_unlock(&umh_wq_hash_lock);
+		else {
+			hlist_del(&entry->umh_wq_hlist);
+			spin_unlock(&umh_wq_hash_lock);
+			destroy_workqueue(entry->wq);
+			kfree(entry);
+		}
+	}
+	return;
+}
+EXPORT_SYMBOL(umh_wq_put_token);
+
 /**
  * call_usermodehelper_setup - prepare to call a usermode helper
  * @path: path to usermode executable
@@ -689,6 +865,7 @@ struct ctl_table usermodehelper_table[] = {
 
 void __init usermodehelper_init(void)
 {
-	khelper_wq = create_singlethread_workqueue("khelper");
+	umh_wq_hash_init();
+	khelper_wq = create_singlethread_workqueue(KHELPER);
 	BUG_ON(!khelper_wq);
 }

Re: [RFC PATCH 5 1/7] kmod - add workqueue service thread store

[Jeff Layton]

On Tue, 31 Mar 2015 11:14:42 +0800
Ian Kent <raven@themaw.net> wrote:

> From: Ian Kent <ikent@redhat.com>
> 
> Persistent use of process information is needed for contained
> execution in a namespace other than the root init namespace.
> 
> Use a simple random token as a key to create and store thread
> information in a hashed list for use by the usermode helper
> thread runner.
> 
> Signed-off-by: Ian Kent <ikent@redhat.com>
> Cc: Benjamin Coddington <bcodding@redhat.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Trond Myklebust <trond.myklebust@primarydata.com>
> Cc: Oleg Nesterov <onestero@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Jeff Layton <jeff.layton@primarydata.com>
> ---
>  include/linux/kmod.h |    3 +
>  kernel/kmod.c        |  179 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index 0555cc6..fa46722 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -66,6 +66,9 @@ struct subprocess_info {
>  	void *data;
>  };
>  
> +extern int umh_wq_get_token(int token, const char *service);
> +extern void umh_wq_put_token(int token);
> +
>  extern int
>  call_usermodehelper(char *path, char **argv, char **envp, int wait);
>  
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index 2777f40..55d20ce 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -40,13 +40,30 @@
>  #include <linux/ptrace.h>
>  #include <linux/async.h>
>  #include <asm/uaccess.h>
> +#include <linux/hash.h>
> +#include <linux/list.h>
> +#include <linux/random.h>
>  
>  #include <trace/events/module.h>
>  
>  extern int max_threads;
>  
> +#define KHELPER		"khelper"
>  static struct workqueue_struct *khelper_wq;
>  
> +#define UMH_WQ_HASH_SHIFT  6
> +#define UMH_WQ_HASH_SIZE   1 << UMH_WQ_HASH_SHIFT
> +
> +struct umh_wq_entry {
> +	int token;
> +	unsigned int count;
> +	struct workqueue_struct *wq;
> +	struct hlist_node umh_wq_hlist;
> +};
> +
> +static DEFINE_SPINLOCK(umh_wq_hash_lock);
> +static struct hlist_head umh_wq_hash[UMH_WQ_HASH_SIZE];
> +
>  #define CAP_BSET	(void *)1
>  #define CAP_PI		(void *)2
>  
> @@ -475,6 +492,165 @@ static void helper_unlock(void)
>  		wake_up(&running_helpers_waitq);
>  }
>  
> +static void umh_wq_hash_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < UMH_WQ_HASH_SIZE; i++)
> +		INIT_HLIST_HEAD(&umh_wq_hash[i]);
> +}
> +
> +static struct umh_wq_entry *umh_wq_find_entry(int token)
> +{
> +	struct umh_wq_entry *this, *entry;
> +	struct hlist_head *bucket;
> +	unsigned int hash;
> +
> +	hash = hash_32((unsigned long) token, UMH_WQ_HASH_SHIFT);
> +	bucket = &umh_wq_hash[hash];
> +
> +	entry = ERR_PTR(-ENOENT);
> +	if (hlist_empty(bucket))
> +		goto out;
> +
> +	hlist_for_each_entry(this, bucket, umh_wq_hlist) {
> +		if (this->token == token) {
> +			entry = this;
> +			break;
> +		}
> +	}
> +out:
> +	return entry;
> +}
> +
> +static struct workqueue_struct *umh_find_wq(int token, unsigned int nowait)

nit: there's no caller of this in this patch, but one is added in patch #2.

> +{
> +	struct umh_wq_entry *entry;
> +	unsigned long flags;
> +
> +	if (!token)
> +		return khelper_wq;
> +
> +	if (nowait)
> +		spin_lock_irqsave(&umh_wq_hash_lock, flags);
> +	else
> +		spin_lock(&umh_wq_hash_lock);
> +	entry = umh_wq_find_entry(token);
> +	if (nowait)
> +		spin_unlock_irqrestore(&umh_wq_hash_lock, flags);
> +	else
> +		spin_unlock(&umh_wq_hash_lock);
> +
> +	return entry->wq;
> +}
> +
> +/**
> + * umh_wq_get_token - create service thread and return an identifying token
> + * @token: token of an existing service thread or 0 to create a new
> + * 	   service thread.
> + * @name: service name to be appended to "khelper" for identification.
> + *
> + * Returns a token that used with calls to call_usermode_helper_service().
> + * If token corresponds to an existing service thread its reference count
> + * is increased and the token returned. On failure returns a negative errno.
> + */
> +int umh_wq_get_token(int token, const char *service)
> +{
> +	struct workqueue_struct *wq;
> +	char *wq_name;
> +	int wq_name_len;
> +	struct umh_wq_entry *entry;
> +	struct hlist_head *bucket;
> +	unsigned int hash;
> +	unsigned int new_token;
> +
> +	if (token) {
> +		spin_lock(&umh_wq_hash_lock);
> +		entry = umh_wq_find_entry(token);
> +		if (entry) {
> +			entry->count++;
> +			spin_unlock(&umh_wq_hash_lock);
> +			return token;
> +		}
> +		spin_unlock(&umh_wq_hash_lock);
> +	}
> +
> +	if (!service)
> +		return -EINVAL;
> +
> +	wq_name_len = sizeof(KHELPER) + strlen(service) + 1;
> +	wq_name = kmalloc(wq_name_len, GFP_KERNEL);
> +	if (!wq_name)
> +		return -ENOMEM;
> +	strcpy(wq_name, "KHELPER-");
> +	strcat(wq_name, service);
> +
> +	entry = kzalloc(sizeof(struct umh_wq_entry), GFP_KERNEL);
> +	if (!entry) {
> +		kfree(wq_name);
> +		return -ENOMEM;
> +	}
> +
> +	wq = create_singlethread_workqueue(wq_name);
> +	if (IS_ERR(wq)) {
> +		kfree(wq_name);
> +		kfree(entry);
> +		return PTR_ERR(wq);
> +	}
> +	kfree(wq_name);
> +	entry->wq = wq;
> +
> +	do {
> +		new_token = get_random_int();
> +		if (!new_token)
> +			continue;

Why a random value here? Is there some attack that can be done by
guessing the token?

ISTM that that could end up with you reusing a token soon after it has
been removed from the hash if you were really unlucky. If so, then that
could potentially be dangerous.

If there isn't a potential attack vector that involves guessing this
value, then a simple counter might mean less chance that the token
would be reused (particularly if the token were 64 bits).

> +		spin_lock(&umh_wq_hash_lock);
> +		entry = umh_wq_find_entry(new_token);
> +		if (likely(IS_ERR(entry))) {
> +			hash = hash_32(new_token, UMH_WQ_HASH_SHIFT);
> +			bucket = &umh_wq_hash[hash];
> +			hlist_add_head(&entry->umh_wq_hlist, bucket);
> +			entry->token = (long) new_token;

Why cast to long here? Shouldn't that be (int)?

> +			spin_unlock(&umh_wq_hash_lock);
> +			break;
> +		}
> +		spin_unlock(&umh_wq_hash_lock);
> +	} while (1);
> +
> +	return (int) new_token;
> +}
> +EXPORT_SYMBOL(umh_wq_get_token);
> +
> +/**
> + * umh_ns_put_token - stop a service thread if it's not longer in use
> + * 		      and remove it from the serive thread list
> + * @token: token of a service thread.
> + */
> +void umh_wq_put_token(int token)
> +{
> +	struct umh_wq_entry *entry;
> +
> +	if (!token)
> +		return;
> +
> +	spin_lock(&umh_wq_hash_lock);
> +	entry = umh_wq_find_entry(token);
> +	if (unlikely(IS_ERR(entry)))
> +		spin_unlock(&umh_wq_hash_lock);
> +	else {
> +		if (--entry->count)
> +			spin_unlock(&umh_wq_hash_lock);
> +		else {
> +			hlist_del(&entry->umh_wq_hlist);
> +			spin_unlock(&umh_wq_hash_lock);
> +			destroy_workqueue(entry->wq);
> +			kfree(entry);
> +		}
> +	}
> +	return;
> +}
> +EXPORT_SYMBOL(umh_wq_put_token);
> +
>  /**
>   * call_usermodehelper_setup - prepare to call a usermode helper
>   * @path: path to usermode executable
> @@ -689,6 +865,7 @@ struct ctl_table usermodehelper_table[] = {
>  
>  void __init usermodehelper_init(void)
>  {
> -	khelper_wq = create_singlethread_workqueue("khelper");
> +	umh_wq_hash_init();
> +	khelper_wq = create_singlethread_workqueue(KHELPER);
>  	BUG_ON(!khelper_wq);
>  }
> 


-- 
Jeff Layton <jeff.layton@primarydata.com>

Re: [RFC PATCH 5 1/7] kmod - add workqueue service thread store

[Ian Kent]

On Tue, 2015-03-31 at 07:21 -0400, Jeff Layton wrote:
> On Tue, 31 Mar 2015 11:14:42 +0800
> Ian Kent <raven@themaw.net> wrote:
> 
> > From: Ian Kent <ikent@redhat.com>
> > 
> > Persistent use of process information is needed for contained
> > execution in a namespace other than the root init namespace.
> > 
> > Use a simple random token as a key to create and store thread
> > information in a hashed list for use by the usermode helper
> > thread runner.
> > 
> > Signed-off-by: Ian Kent <ikent@redhat.com>
> > Cc: Benjamin Coddington <bcodding@redhat.com>
> > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > Cc: J. Bruce Fields <bfields@fieldses.org>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Trond Myklebust <trond.myklebust@primarydata.com>
> > Cc: Oleg Nesterov <onestero@redhat.com>
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> >  include/linux/kmod.h |    3 +
> >  kernel/kmod.c        |  179 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 181 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> > index 0555cc6..fa46722 100644
> > --- a/include/linux/kmod.h
> > +++ b/include/linux/kmod.h
> > @@ -66,6 +66,9 @@ struct subprocess_info {
> >  	void *data;
> >  };
> >  
> > +extern int umh_wq_get_token(int token, const char *service);
> > +extern void umh_wq_put_token(int token);
> > +
> >  extern int
> >  call_usermodehelper(char *path, char **argv, char **envp, int wait);
> >  
> > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > index 2777f40..55d20ce 100644
> > --- a/kernel/kmod.c
> > +++ b/kernel/kmod.c
> > @@ -40,13 +40,30 @@
> >  #include <linux/ptrace.h>
> >  #include <linux/async.h>
> >  #include <asm/uaccess.h>
> > +#include <linux/hash.h>
> > +#include <linux/list.h>
> > +#include <linux/random.h>
> >  
> >  #include <trace/events/module.h>
> >  
> >  extern int max_threads;
> >  
> > +#define KHELPER		"khelper"
> >  static struct workqueue_struct *khelper_wq;
> >  
> > +#define UMH_WQ_HASH_SHIFT  6
> > +#define UMH_WQ_HASH_SIZE   1 << UMH_WQ_HASH_SHIFT
> > +
> > +struct umh_wq_entry {
> > +	int token;
> > +	unsigned int count;
> > +	struct workqueue_struct *wq;
> > +	struct hlist_node umh_wq_hlist;
> > +};
> > +
> > +static DEFINE_SPINLOCK(umh_wq_hash_lock);
> > +static struct hlist_head umh_wq_hash[UMH_WQ_HASH_SIZE];
> > +
> >  #define CAP_BSET	(void *)1
> >  #define CAP_PI		(void *)2
> >  
> > @@ -475,6 +492,165 @@ static void helper_unlock(void)
> >  		wake_up(&running_helpers_waitq);
> >  }
> >  
> > +static void umh_wq_hash_init(void)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < UMH_WQ_HASH_SIZE; i++)
> > +		INIT_HLIST_HEAD(&umh_wq_hash[i]);
> > +}
> > +
> > +static struct umh_wq_entry *umh_wq_find_entry(int token)
> > +{
> > +	struct umh_wq_entry *this, *entry;
> > +	struct hlist_head *bucket;
> > +	unsigned int hash;
> > +
> > +	hash = hash_32((unsigned long) token, UMH_WQ_HASH_SHIFT);
> > +	bucket = &umh_wq_hash[hash];
> > +
> > +	entry = ERR_PTR(-ENOENT);
> > +	if (hlist_empty(bucket))
> > +		goto out;
> > +
> > +	hlist_for_each_entry(this, bucket, umh_wq_hlist) {
> > +		if (this->token == token) {
> > +			entry = this;
> > +			break;
> > +		}
> > +	}
> > +out:
> > +	return entry;
> > +}
> > +
> > +static struct workqueue_struct *umh_find_wq(int token, unsigned int nowait)
> 
> nit: there's no caller of this in this patch, but one is added in patch #2.

I could change that.
The patch is meant to setup the infrastructure for the subsequent patch.
I can re-organize them if this ends up being worth continuing with.

> 
> > +{
> > +	struct umh_wq_entry *entry;
> > +	unsigned long flags;
> > +
> > +	if (!token)
> > +		return khelper_wq;
> > +
> > +	if (nowait)
> > +		spin_lock_irqsave(&umh_wq_hash_lock, flags);
> > +	else
> > +		spin_lock(&umh_wq_hash_lock);
> > +	entry = umh_wq_find_entry(token);
> > +	if (nowait)
> > +		spin_unlock_irqrestore(&umh_wq_hash_lock, flags);
> > +	else
> > +		spin_unlock(&umh_wq_hash_lock);
> > +
> > +	return entry->wq;
> > +}
> > +
> > +/**
> > + * umh_wq_get_token - create service thread and return an identifying token
> > + * @token: token of an existing service thread or 0 to create a new
> > + * 	   service thread.
> > + * @name: service name to be appended to "khelper" for identification.
> > + *
> > + * Returns a token that used with calls to call_usermode_helper_service().
> > + * If token corresponds to an existing service thread its reference count
> > + * is increased and the token returned. On failure returns a negative errno.
> > + */
> > +int umh_wq_get_token(int token, const char *service)
> > +{
> > +	struct workqueue_struct *wq;
> > +	char *wq_name;
> > +	int wq_name_len;
> > +	struct umh_wq_entry *entry;
> > +	struct hlist_head *bucket;
> > +	unsigned int hash;
> > +	unsigned int new_token;
> > +
> > +	if (token) {
> > +		spin_lock(&umh_wq_hash_lock);
> > +		entry = umh_wq_find_entry(token);
> > +		if (entry) {
> > +			entry->count++;
> > +			spin_unlock(&umh_wq_hash_lock);
> > +			return token;
> > +		}
> > +		spin_unlock(&umh_wq_hash_lock);
> > +	}
> > +
> > +	if (!service)
> > +		return -EINVAL;
> > +
> > +	wq_name_len = sizeof(KHELPER) + strlen(service) + 1;
> > +	wq_name = kmalloc(wq_name_len, GFP_KERNEL);
> > +	if (!wq_name)
> > +		return -ENOMEM;
> > +	strcpy(wq_name, "KHELPER-");
> > +	strcat(wq_name, service);
> > +
> > +	entry = kzalloc(sizeof(struct umh_wq_entry), GFP_KERNEL);
> > +	if (!entry) {
> > +		kfree(wq_name);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	wq = create_singlethread_workqueue(wq_name);
> > +	if (IS_ERR(wq)) {
> > +		kfree(wq_name);
> > +		kfree(entry);
> > +		return PTR_ERR(wq);
> > +	}
> > +	kfree(wq_name);
> > +	entry->wq = wq;
> > +
> > +	do {
> > +		new_token = get_random_int();
> > +		if (!new_token)
> > +			continue;
> 
> Why a random value here? Is there some attack that can be done by
> guessing the token?

Good question for which I have no good answer.
I was originally concerned about reuse after overflow from continued
issue and thought using a random token would be better but ....

> 
> ISTM that that could end up with you reusing a token soon after it has
> been removed from the hash if you were really unlucky. If so, then that
> could potentially be dangerous.

I cover that by not using a token for which an entry already exists.
But a counter is probably equally as good.

> 
> If there isn't a potential attack vector that involves guessing this
> value, then a simple counter might mean less chance that the token
> would be reused (particularly if the token were 64 bits).
> 
> > +		spin_lock(&umh_wq_hash_lock);
> > +		entry = umh_wq_find_entry(new_token);
> > +		if (likely(IS_ERR(entry))) {
> > +			hash = hash_32(new_token, UMH_WQ_HASH_SHIFT);
> > +			bucket = &umh_wq_hash[hash];
> > +			hlist_add_head(&entry->umh_wq_hlist, bucket);
> > +			entry->token = (long) new_token;
> 
> Why cast to long here? Shouldn't that be (int)?

Yes, I missed that when re-doing the patch, ;)

> 
> > +			spin_unlock(&umh_wq_hash_lock);
> > +			break;
> > +		}
> > +		spin_unlock(&umh_wq_hash_lock);
> > +	} while (1);
> > +
> > +	return (int) new_token;
> > +}
> > +EXPORT_SYMBOL(umh_wq_get_token);
> > +
> > +/**
> > + * umh_ns_put_token - stop a service thread if it's not longer in use
> > + * 		      and remove it from the serive thread list
> > + * @token: token of a service thread.
> > + */
> > +void umh_wq_put_token(int token)
> > +{
> > +	struct umh_wq_entry *entry;
> > +
> > +	if (!token)
> > +		return;
> > +
> > +	spin_lock(&umh_wq_hash_lock);
> > +	entry = umh_wq_find_entry(token);
> > +	if (unlikely(IS_ERR(entry)))
> > +		spin_unlock(&umh_wq_hash_lock);
> > +	else {
> > +		if (--entry->count)
> > +			spin_unlock(&umh_wq_hash_lock);
> > +		else {
> > +			hlist_del(&entry->umh_wq_hlist);
> > +			spin_unlock(&umh_wq_hash_lock);
> > +			destroy_workqueue(entry->wq);
> > +			kfree(entry);
> > +		}
> > +	}
> > +	return;
> > +}
> > +EXPORT_SYMBOL(umh_wq_put_token);
> > +
> >  /**
> >   * call_usermodehelper_setup - prepare to call a usermode helper
> >   * @path: path to usermode executable
> > @@ -689,6 +865,7 @@ struct ctl_table usermodehelper_table[] = {
> >  
> >  void __init usermodehelper_init(void)
> >  {
> > -	khelper_wq = create_singlethread_workqueue("khelper");
> > +	umh_wq_hash_init();
> > +	khelper_wq = create_singlethread_workqueue(KHELPER);
> >  	BUG_ON(!khelper_wq);
> >  }
> > 
> 
> 

[RFC PATCH 5 2/7] kmod - teach usermodehelper to use service workqueues

[Ian Kent]

The call_usermodehelper() function executes all binaries in the
global "init" root context. This doesn't allow a binary to be run
within a namespace environment such as a container. To do this
the namespace environment of the contaner must be available to
provide the required execution environment.

This can be done by creating a service thread, identified by
issuing a token identifier, used when executing the helper
with a function that takes the token as a parameter.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 include/linux/kmod.h |    5 +++++
 kernel/kmod.c        |   38 +++++++++++++++++++++++++++++++-------
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index fa46722..9a9fcb3 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -56,6 +56,7 @@ struct file;
 struct subprocess_info {
 	struct work_struct work;
 	struct completion *complete;
+	int wq_token;
 	char *path;
 	char **argv;
 	char **envp;
@@ -72,6 +73,10 @@ extern void umh_wq_put_token(int token);
 extern int
 call_usermodehelper(char *path, char **argv, char **envp, int wait);
 
+extern int
+call_usermodehelper_service(char *path, char **argv,
+			    char **envp, int token, int wait);
+
 extern struct subprocess_info *
 call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
 			  int (*init)(struct subprocess_info *info, struct cred *new),
diff --git a/kernel/kmod.c b/kernel/kmod.c
index 55d20ce..47c5ff5 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -712,6 +712,7 @@ EXPORT_SYMBOL(call_usermodehelper_setup);
  */
 int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 {
+	static struct workqueue_struct *wq;
 	DECLARE_COMPLETION_ONSTACK(done);
 	int retval = 0;
 
@@ -720,7 +721,8 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 		return -EINVAL;
 	}
 	helper_lock();
-	if (!khelper_wq || usermodehelper_disabled) {
+	wq = umh_find_wq(sub_info->wq_token, wait);
+	if (!wq || usermodehelper_disabled) {
 		retval = -EBUSY;
 		goto out;
 	}
@@ -732,7 +734,7 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
 	sub_info->complete = (wait == UMH_NO_WAIT) ? NULL : &done;
 	sub_info->wait = wait;
 
-	queue_work(khelper_wq, &sub_info->work);
+	queue_work(wq, &sub_info->work);
 	if (wait == UMH_NO_WAIT)	/* task has freed sub_info */
 		goto unlock;
 
@@ -759,19 +761,21 @@ unlock:
 EXPORT_SYMBOL(call_usermodehelper_exec);
 
 /**
- * call_usermodehelper() - prepare and start a usermode application
+ * call_usermodehelper_service() - start a usermode application within
+ * 				a service environment.
  * @path: path to usermode executable
  * @argv: arg vector for process
  * @envp: environment for process
+ * @token: token corresponding to a service environment obtained by a
+ * 	   call to umh_wq_get_token().
  * @wait: wait for the application to finish and return status.
  *        when UMH_NO_WAIT don't wait at all, but you get no useful error back
  *        when the program couldn't be exec'ed. This makes it safe to call
  *        from interrupt context.
- *
- * This function is the equivalent to use call_usermodehelper_setup() and
- * call_usermodehelper_exec().
  */
-int call_usermodehelper(char *path, char **argv, char **envp, int wait)
+int call_usermodehelper_service(char *path,
+				char **argv, char **envp,
+				int token, int wait)
 {
 	struct subprocess_info *info;
 	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
@@ -780,9 +784,29 @@ int call_usermodehelper(char *path, char **argv, char **envp, int wait)
 					 NULL, NULL, NULL);
 	if (info == NULL)
 		return -ENOMEM;
+	info->wq_token = token;
 
 	return call_usermodehelper_exec(info, wait);
 }
+EXPORT_SYMBOL(call_usermodehelper_service);
+
+/**
+ * call_usermodehelper() - prepare and start a usermode application
+ * @path: path to usermode executable
+ * @argv: arg vector for process
+ * @envp: environment for process
+ * @wait: wait for the application to finish and return status.
+ *        when UMH_NO_WAIT don't wait at all, but you get no useful error back
+ *        when the program couldn't be exec'ed. This makes it safe to call
+ *        from interrupt context.
+ *
+ * This function is the equivalent to using call_usermodehelper_setup() and
+ * call_usermodehelper_exec().
+ */
+int call_usermodehelper(char *path, char **argv, char **envp, int wait)
+{
+	return call_usermodehelper_service(path, argv, envp, 0, wait);
+}
 EXPORT_SYMBOL(call_usermodehelper);
 
 static int proc_cap_handler(struct ctl_table *table, int write,

[RFC PATCH 5 3/7] nfsd - use service thread if not executing in init namespace

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

If nfsd is running within a container the client tracking operations
should run within their originating container also. To do that get a
token to a service thread created within the container environment
for usermode helper calls.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfsd/netns.h       |    3 +++
 fs/nfsd/nfs4recover.c |   48 +++++++++++++++++++++++++++++++-----------------
 fs/nfsd/nfsctl.c      |    6 ++++++
 3 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ea6749a..099a3c5 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -112,6 +112,9 @@ struct nfsd_net {
 	u32 clientid_counter;
 
 	struct svc_serv *nfsd_serv;
+
+	/* Namespace token */
+	int umh_token;
 };
 
 /* Simple check to find out if a given net was properly initialized */
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 1c307f0..2547edb 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -1184,7 +1184,8 @@ nfsd4_cltrack_grace_start(time_t grace_start)
 }
 
 static int
-nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
+nfsd4_umh_cltrack_upcall(char *cmd, char *arg,
+			 char *env0, char *env1, int token)
 {
 	char *envp[3];
 	char *argv[4];
@@ -1209,7 +1210,11 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
 	argv[2] = arg;
 	argv[3] = NULL;
 
-	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+	if (token > 0)
+		ret = call_usermodehelper_service(argv[0], argv, envp,
+						  token, UMH_WAIT_PROC);
+	else
+		ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
 	/*
 	 * Disable the upcall mechanism if we're getting an ENOENT or EACCES
 	 * error. The admin can re-enable it on the fly by using sysfs
@@ -1252,14 +1257,8 @@ nfsd4_umh_cltrack_init(struct net *net)
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
 
-	/* XXX: The usermode helper s not working in container yet. */
-	if (net != &init_net) {
-		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
-			"tracking in a container!\n");
-		return -EINVAL;
-	}
-
-	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
+	ret = nfsd4_umh_cltrack_upcall("init", NULL,
+					grace_start, NULL, nn->umh_token);
 	kfree(grace_start);
 	return ret;
 }
@@ -1285,6 +1284,7 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
 {
 	char *hexid, *has_session, *grace_start;
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+	int ret;
 
 	/*
 	 * With v4.0 clients, there's little difference in outcome between a
@@ -1312,7 +1312,10 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
 	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
 
 	nfsd4_cltrack_upcall_lock(clp);
-	if (!nfsd4_umh_cltrack_upcall("create", hexid, has_session, grace_start))
+	ret = nfsd4_umh_cltrack_upcall("create",
+				       hexid, has_session, grace_start,
+				       nn->umh_token);
+	if (!ret)
 		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
 	nfsd4_cltrack_upcall_unlock(clp);
 
@@ -1324,7 +1327,9 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
 static void
 nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
 {
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 	char *hexid;
+	int ret;
 
 	if (!test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return;
@@ -1336,9 +1341,13 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
 	}
 
 	nfsd4_cltrack_upcall_lock(clp);
-	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags) &&
-	    nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL) == 0)
-		clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) {
+		ret = nfsd4_umh_cltrack_upcall("remove",
+					       hexid, NULL, NULL,
+					       nn->umh_token);
+		if (ret == 0)
+			clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+	}
 	nfsd4_cltrack_upcall_unlock(clp);
 
 	kfree(hexid);
@@ -1347,8 +1356,9 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
 static int
 nfsd4_umh_cltrack_check(struct nfs4_client *clp)
 {
-	int ret;
+	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 	char *hexid, *has_session, *legacy;
+	int ret;
 
 	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return 0;
@@ -1366,7 +1376,9 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
 	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) {
 		ret = 0;
 	} else {
-		ret = nfsd4_umh_cltrack_upcall("check", hexid, has_session, legacy);
+		ret = nfsd4_umh_cltrack_upcall("check", hexid,
+					       has_session, legacy,
+					       nn->umh_token);
 		if (ret == 0)
 			set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
 	}
@@ -1386,7 +1398,9 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
 
 	sprintf(timestr, "%ld", nn->boot_time);
 	legacy = nfsd4_cltrack_legacy_topdir();
-	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
+	nfsd4_umh_cltrack_upcall("gracedone",
+				 timestr, legacy, NULL,
+				 nn->umh_token);
 	kfree(legacy);
 }
 
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index aa47d75..f5ae092 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1224,6 +1224,8 @@ static __net_init int nfsd_init_net(struct net *net)
 		goto out_idmap_error;
 	nn->nfsd4_lease = 90;	/* default lease time */
 	nn->nfsd4_grace = 90;
+	if (net != &init_net)
+		nn->umh_token = umh_wq_get_token(nn->umh_token, "knfsd");
 	return 0;
 
 out_idmap_error:
@@ -1234,6 +1236,10 @@ out_export_error:
 
 static __net_exit void nfsd_exit_net(struct net *net)
 {
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+
+	umh_wq_put_token(nn->umh_token);
+	nn->umh_token = 0;
 	nfsd_idmap_shutdown(net);
 	nfsd_export_shutdown(net);
 }

Re: [RFC PATCH 5 3/7] nfsd - use service thread if not executing in init namespace

[J. Bruce Fields]

On Tue, Mar 31, 2015 at 11:14:58AM +0800, Ian Kent wrote:
> From: Ian Kent <ikent@redhat.com>
> 
> If nfsd is running within a container the client tracking operations
> should run within their originating container also. To do that get a
> token to a service thread created within the container environment
> for usermode helper calls.
> 
> Signed-off-by: Ian Kent <ikent@redhat.com>
> Cc: Benjamin Coddington <bcodding@redhat.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: J. Bruce Fields <bfields@fieldses.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Trond Myklebust <trond.myklebust@primarydata.com>
> Cc: Oleg Nesterov <onestero@redhat.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Jeff Layton <jeff.layton@primarydata.com>
> ---
>  fs/nfsd/netns.h       |    3 +++
>  fs/nfsd/nfs4recover.c |   48 +++++++++++++++++++++++++++++++-----------------
>  fs/nfsd/nfsctl.c      |    6 ++++++
>  3 files changed, 40 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index ea6749a..099a3c5 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -112,6 +112,9 @@ struct nfsd_net {
>  	u32 clientid_counter;
>  
>  	struct svc_serv *nfsd_serv;
> +
> +	/* Namespace token */
> +	int umh_token;
>  };
>  
>  /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 1c307f0..2547edb 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -1184,7 +1184,8 @@ nfsd4_cltrack_grace_start(time_t grace_start)
>  }
>  
>  static int
> -nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> +nfsd4_umh_cltrack_upcall(char *cmd, char *arg,
> +			 char *env0, char *env1, int token)
>  {
>  	char *envp[3];
>  	char *argv[4];
> @@ -1209,7 +1210,11 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
>  	argv[2] = arg;
>  	argv[3] = NULL;
>  
> -	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> +	if (token > 0)
> +		ret = call_usermodehelper_service(argv[0], argv, envp,
> +						  token, UMH_WAIT_PROC);
> +	else
> +		ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);

Do we really need to handle the init_net case specially, or could we
just let it create a workqueue in that case as well?

--b.

>  	/*
>  	 * Disable the upcall mechanism if we're getting an ENOENT or EACCES
>  	 * error. The admin can re-enable it on the fly by using sysfs
> @@ -1252,14 +1257,8 @@ nfsd4_umh_cltrack_init(struct net *net)
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
>  
> -	/* XXX: The usermode helper s not working in container yet. */
> -	if (net != &init_net) {
> -		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
> -			"tracking in a container!\n");
> -		return -EINVAL;
> -	}
> -
> -	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
> +	ret = nfsd4_umh_cltrack_upcall("init", NULL,
> +					grace_start, NULL, nn->umh_token);
>  	kfree(grace_start);
>  	return ret;
>  }
> @@ -1285,6 +1284,7 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
>  {
>  	char *hexid, *has_session, *grace_start;
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> +	int ret;
>  
>  	/*
>  	 * With v4.0 clients, there's little difference in outcome between a
> @@ -1312,7 +1312,10 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
>  	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
>  
>  	nfsd4_cltrack_upcall_lock(clp);
> -	if (!nfsd4_umh_cltrack_upcall("create", hexid, has_session, grace_start))
> +	ret = nfsd4_umh_cltrack_upcall("create",
> +				       hexid, has_session, grace_start,
> +				       nn->umh_token);
> +	if (!ret)
>  		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
>  	nfsd4_cltrack_upcall_unlock(clp);
>  
> @@ -1324,7 +1327,9 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
>  static void
>  nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
>  {
> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	char *hexid;
> +	int ret;
>  
>  	if (!test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return;
> @@ -1336,9 +1341,13 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
>  	}
>  
>  	nfsd4_cltrack_upcall_lock(clp);
> -	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags) &&
> -	    nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL) == 0)
> -		clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> +	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) {
> +		ret = nfsd4_umh_cltrack_upcall("remove",
> +					       hexid, NULL, NULL,
> +					       nn->umh_token);
> +		if (ret == 0)
> +			clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> +	}
>  	nfsd4_cltrack_upcall_unlock(clp);
>  
>  	kfree(hexid);
> @@ -1347,8 +1356,9 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
>  static int
>  nfsd4_umh_cltrack_check(struct nfs4_client *clp)
>  {
> -	int ret;
> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	char *hexid, *has_session, *legacy;
> +	int ret;
>  
>  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return 0;
> @@ -1366,7 +1376,9 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
>  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) {
>  		ret = 0;
>  	} else {
> -		ret = nfsd4_umh_cltrack_upcall("check", hexid, has_session, legacy);
> +		ret = nfsd4_umh_cltrack_upcall("check", hexid,
> +					       has_session, legacy,
> +					       nn->umh_token);
>  		if (ret == 0)
>  			set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
>  	}
> @@ -1386,7 +1398,9 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
>  
>  	sprintf(timestr, "%ld", nn->boot_time);
>  	legacy = nfsd4_cltrack_legacy_topdir();
> -	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
> +	nfsd4_umh_cltrack_upcall("gracedone",
> +				 timestr, legacy, NULL,
> +				 nn->umh_token);
>  	kfree(legacy);
>  }
>  
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index aa47d75..f5ae092 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1224,6 +1224,8 @@ static __net_init int nfsd_init_net(struct net *net)
>  		goto out_idmap_error;
>  	nn->nfsd4_lease = 90;	/* default lease time */
>  	nn->nfsd4_grace = 90;
> +	if (net != &init_net)
> +		nn->umh_token = umh_wq_get_token(nn->umh_token, "knfsd");
>  	return 0;
>  
>  out_idmap_error:
> @@ -1234,6 +1236,10 @@ out_export_error:
>  
>  static __net_exit void nfsd_exit_net(struct net *net)
>  {
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	umh_wq_put_token(nn->umh_token);
> +	nn->umh_token = 0;
>  	nfsd_idmap_shutdown(net);
>  	nfsd_export_shutdown(net);
>  }

Re: [RFC PATCH 5 3/7] nfsd - use service thread if not executing in init namespace

[Ian Kent]

On Tue, 2015-03-31 at 09:14 -0400, J. Bruce Fields wrote:
> On Tue, Mar 31, 2015 at 11:14:58AM +0800, Ian Kent wrote:
> > From: Ian Kent <ikent@redhat.com>
> > 
> > If nfsd is running within a container the client tracking operations
> > should run within their originating container also. To do that get a
> > token to a service thread created within the container environment
> > for usermode helper calls.
> > 
> > Signed-off-by: Ian Kent <ikent@redhat.com>
> > Cc: Benjamin Coddington <bcodding@redhat.com>
> > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > Cc: J. Bruce Fields <bfields@fieldses.org>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Trond Myklebust <trond.myklebust@primarydata.com>
> > Cc: Oleg Nesterov <onestero@redhat.com>
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> >  fs/nfsd/netns.h       |    3 +++
> >  fs/nfsd/nfs4recover.c |   48 +++++++++++++++++++++++++++++++-----------------
> >  fs/nfsd/nfsctl.c      |    6 ++++++
> >  3 files changed, 40 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > index ea6749a..099a3c5 100644
> > --- a/fs/nfsd/netns.h
> > +++ b/fs/nfsd/netns.h
> > @@ -112,6 +112,9 @@ struct nfsd_net {
> >  	u32 clientid_counter;
> >  
> >  	struct svc_serv *nfsd_serv;
> > +
> > +	/* Namespace token */
> > +	int umh_token;
> >  };
> >  
> >  /* Simple check to find out if a given net was properly initialized */
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index 1c307f0..2547edb 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -1184,7 +1184,8 @@ nfsd4_cltrack_grace_start(time_t grace_start)
> >  }
> >  
> >  static int
> > -nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> > +nfsd4_umh_cltrack_upcall(char *cmd, char *arg,
> > +			 char *env0, char *env1, int token)
> >  {
> >  	char *envp[3];
> >  	char *argv[4];
> > @@ -1209,7 +1210,11 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> >  	argv[2] = arg;
> >  	argv[3] = NULL;
> >  
> > -	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> > +	if (token > 0)
> > +		ret = call_usermodehelper_service(argv[0], argv, envp,
> > +						  token, UMH_WAIT_PROC);
> > +	else
> > +		ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> 
> Do we really need to handle the init_net case specially, or could we
> just let it create a workqueue in that case as well?

That's pretty much up to you but there's still the need to get a token
to create the work queue (and put it to terminate it) or just pass 0 to
call_usermodehelper_service().

> 
> --b.
> 
> >  	/*
> >  	 * Disable the upcall mechanism if we're getting an ENOENT or EACCES
> >  	 * error. The admin can re-enable it on the fly by using sysfs
> > @@ -1252,14 +1257,8 @@ nfsd4_umh_cltrack_init(struct net *net)
> >  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> >  	char *grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> >  
> > -	/* XXX: The usermode helper s not working in container yet. */
> > -	if (net != &init_net) {
> > -		WARN(1, KERN_ERR "NFSD: attempt to initialize umh client "
> > -			"tracking in a container!\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	ret = nfsd4_umh_cltrack_upcall("init", NULL, grace_start, NULL);
> > +	ret = nfsd4_umh_cltrack_upcall("init", NULL,
> > +					grace_start, NULL, nn->umh_token);
> >  	kfree(grace_start);
> >  	return ret;
> >  }
> > @@ -1285,6 +1284,7 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
> >  {
> >  	char *hexid, *has_session, *grace_start;
> >  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> > +	int ret;
> >  
> >  	/*
> >  	 * With v4.0 clients, there's little difference in outcome between a
> > @@ -1312,7 +1312,10 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
> >  	grace_start = nfsd4_cltrack_grace_start(nn->boot_time);
> >  
> >  	nfsd4_cltrack_upcall_lock(clp);
> > -	if (!nfsd4_umh_cltrack_upcall("create", hexid, has_session, grace_start))
> > +	ret = nfsd4_umh_cltrack_upcall("create",
> > +				       hexid, has_session, grace_start,
> > +				       nn->umh_token);
> > +	if (!ret)
> >  		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> >  	nfsd4_cltrack_upcall_unlock(clp);
> >  
> > @@ -1324,7 +1327,9 @@ nfsd4_umh_cltrack_create(struct nfs4_client *clp)
> >  static void
> >  nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
> >  {
> > +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> >  	char *hexid;
> > +	int ret;
> >  
> >  	if (!test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> >  		return;
> > @@ -1336,9 +1341,13 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
> >  	}
> >  
> >  	nfsd4_cltrack_upcall_lock(clp);
> > -	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags) &&
> > -	    nfsd4_umh_cltrack_upcall("remove", hexid, NULL, NULL) == 0)
> > -		clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> > +	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) {
> > +		ret = nfsd4_umh_cltrack_upcall("remove",
> > +					       hexid, NULL, NULL,
> > +					       nn->umh_token);
> > +		if (ret == 0)
> > +			clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> > +	}
> >  	nfsd4_cltrack_upcall_unlock(clp);
> >  
> >  	kfree(hexid);
> > @@ -1347,8 +1356,9 @@ nfsd4_umh_cltrack_remove(struct nfs4_client *clp)
> >  static int
> >  nfsd4_umh_cltrack_check(struct nfs4_client *clp)
> >  {
> > -	int ret;
> > +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> >  	char *hexid, *has_session, *legacy;
> > +	int ret;
> >  
> >  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> >  		return 0;
> > @@ -1366,7 +1376,9 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
> >  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags)) {
> >  		ret = 0;
> >  	} else {
> > -		ret = nfsd4_umh_cltrack_upcall("check", hexid, has_session, legacy);
> > +		ret = nfsd4_umh_cltrack_upcall("check", hexid,
> > +					       has_session, legacy,
> > +					       nn->umh_token);
> >  		if (ret == 0)
> >  			set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> >  	}
> > @@ -1386,7 +1398,9 @@ nfsd4_umh_cltrack_grace_done(struct nfsd_net *nn)
> >  
> >  	sprintf(timestr, "%ld", nn->boot_time);
> >  	legacy = nfsd4_cltrack_legacy_topdir();
> > -	nfsd4_umh_cltrack_upcall("gracedone", timestr, legacy, NULL);
> > +	nfsd4_umh_cltrack_upcall("gracedone",
> > +				 timestr, legacy, NULL,
> > +				 nn->umh_token);
> >  	kfree(legacy);
> >  }
> >  
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index aa47d75..f5ae092 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1224,6 +1224,8 @@ static __net_init int nfsd_init_net(struct net *net)
> >  		goto out_idmap_error;
> >  	nn->nfsd4_lease = 90;	/* default lease time */
> >  	nn->nfsd4_grace = 90;
> > +	if (net != &init_net)
> > +		nn->umh_token = umh_wq_get_token(nn->umh_token, "knfsd");
> >  	return 0;
> >  
> >  out_idmap_error:
> > @@ -1234,6 +1236,10 @@ out_export_error:
> >  
> >  static __net_exit void nfsd_exit_net(struct net *net)
> >  {
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +
> > +	umh_wq_put_token(nn->umh_token);
> > +	nn->umh_token = 0;
> >  	nfsd_idmap_shutdown(net);
> >  	nfsd_export_shutdown(net);
> >  }

Re: [RFC PATCH 5 3/7] nfsd - use service thread if not executing in init namespace

[J. Bruce Fields]

On Wed, Apr 01, 2015 at 08:22:58AM +0800, Ian Kent wrote:
> On Tue, 2015-03-31 at 09:14 -0400, J. Bruce Fields wrote:
> > On Tue, Mar 31, 2015 at 11:14:58AM +0800, Ian Kent wrote:
> > > From: Ian Kent <ikent@redhat.com>
> > > 
> > > If nfsd is running within a container the client tracking operations
> > > should run within their originating container also. To do that get a
> > > token to a service thread created within the container environment
> > > for usermode helper calls.
> > > 
> > > Signed-off-by: Ian Kent <ikent@redhat.com>
> > > Cc: Benjamin Coddington <bcodding@redhat.com>
> > > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > > Cc: J. Bruce Fields <bfields@fieldses.org>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: Trond Myklebust <trond.myklebust@primarydata.com>
> > > Cc: Oleg Nesterov <onestero@redhat.com>
> > > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > > Cc: Jeff Layton <jeff.layton@primarydata.com>
> > > ---
> > >  fs/nfsd/netns.h       |    3 +++
> > >  fs/nfsd/nfs4recover.c |   48 +++++++++++++++++++++++++++++++-----------------
> > >  fs/nfsd/nfsctl.c      |    6 ++++++
> > >  3 files changed, 40 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> > > index ea6749a..099a3c5 100644
> > > --- a/fs/nfsd/netns.h
> > > +++ b/fs/nfsd/netns.h
> > > @@ -112,6 +112,9 @@ struct nfsd_net {
> > >  	u32 clientid_counter;
> > >  
> > >  	struct svc_serv *nfsd_serv;
> > > +
> > > +	/* Namespace token */
> > > +	int umh_token;
> > >  };
> > >  
> > >  /* Simple check to find out if a given net was properly initialized */
> > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > > index 1c307f0..2547edb 100644
> > > --- a/fs/nfsd/nfs4recover.c
> > > +++ b/fs/nfsd/nfs4recover.c
> > > @@ -1184,7 +1184,8 @@ nfsd4_cltrack_grace_start(time_t grace_start)
> > >  }
> > >  
> > >  static int
> > > -nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> > > +nfsd4_umh_cltrack_upcall(char *cmd, char *arg,
> > > +			 char *env0, char *env1, int token)
> > >  {
> > >  	char *envp[3];
> > >  	char *argv[4];
> > > @@ -1209,7 +1210,11 @@ nfsd4_umh_cltrack_upcall(char *cmd, char *arg, char *env0, char *env1)
> > >  	argv[2] = arg;
> > >  	argv[3] = NULL;
> > >  
> > > -	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> > > +	if (token > 0)
> > > +		ret = call_usermodehelper_service(argv[0], argv, envp,
> > > +						  token, UMH_WAIT_PROC);
> > > +	else
> > > +		ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
> > 
> > Do we really need to handle the init_net case specially, or could we
> > just let it create a workqueue in that case as well?
> 
> That's pretty much up to you but there's still the need to get a token
> to create the work queue (and put it to terminate it) or just pass 0 to
> call_usermodehelper_service().

Creating a new workqueue in the init_net case would mean inheriting
stuff from the rpc.nfsd environment that we didn't used to.  (But what
stuff exactly?)  So for backwards compatibility reasons perhaps it's
safer to do what you've done even if the result seems a little
inconsistent.

--b.

[RFC PATCH 5 4/7] nfs - cache_lib use service thread if not executing in init namespace

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

If pipefs is registered within a container pipefs requests should be run
within their originating container also. To do that get a token to a
service thread created within the container environment for usermode
helper calls.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfs/cache_lib.c           |    7 ++++++-
 include/linux/sunrpc/cache.h |    2 ++
 net/sunrpc/cache.c           |    5 +++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/cache_lib.c b/fs/nfs/cache_lib.c
index 5f7b053..78f9b6e 100644
--- a/fs/nfs/cache_lib.c
+++ b/fs/nfs/cache_lib.c
@@ -48,7 +48,12 @@ int nfs_cache_upcall(struct cache_detail *cd, char *entry_name)
 
 	if (nfs_cache_getent_prog[0] == '\0')
 		goto out;
-	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+	if (cd->u.pipefs.umh_token) {
+		int token = cd->u.pipefs.umh_token;
+		ret = call_usermodehelper_service(argv[0], argv, envp,
+						  token, UMH_WAIT_EXEC);
+	} else
+		ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
 	/*
 	 * Disable the upcall mechanism if we're getting an ENOENT or
 	 * EACCES error. The admin can re-enable it on the fly by using
diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
index 437ddb6..bb57c7e 100644
--- a/include/linux/sunrpc/cache.h
+++ b/include/linux/sunrpc/cache.h
@@ -68,6 +68,8 @@ struct cache_detail_procfs {
 
 struct cache_detail_pipefs {
 	struct dentry *dir;
+	/* Namespace token */
+	int umh_token;
 };
 
 struct cache_detail {
diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 5199bb1..eabc0d0 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -1811,6 +1811,9 @@ int sunrpc_cache_register_pipefs(struct dentry *parent,
 	if (IS_ERR(dir))
 		return PTR_ERR(dir);
 	cd->u.pipefs.dir = dir;
+	if (cd->net != &init_net)
+		cd->u.pipefs.umh_token =
+			 umh_wq_get_token(cd->u.pipefs.umh_token, "pipefs");
 	return 0;
 }
 EXPORT_SYMBOL_GPL(sunrpc_cache_register_pipefs);
@@ -1819,6 +1822,8 @@ void sunrpc_cache_unregister_pipefs(struct cache_detail *cd)
 {
 	rpc_remove_cache_dir(cd->u.pipefs.dir);
 	cd->u.pipefs.dir = NULL;
+	umh_wq_put_token(cd->u.pipefs.umh_token);
+	cd->u.pipefs.umh_token = 0;
 }
 EXPORT_SYMBOL_GPL(sunrpc_cache_unregister_pipefs);
 

[RFC PATCH 5 5/7] nfs - objlayout use service thread if not executing in init namespace

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

If the caller is running within a container then execute the usermode
helper callback within the container also.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfs/objlayout/objlayout.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/objlayout/objlayout.c b/fs/nfs/objlayout/objlayout.c
index 919efd4..1d4fd11 100644
--- a/fs/nfs/objlayout/objlayout.c
+++ b/fs/nfs/objlayout/objlayout.c
@@ -600,8 +600,14 @@ static int __objlayout_upcall(struct __auto_login *login)
 		NULL
 	};
 	char *argv[8];
+	int umh_token;
 	int ret;
 
+	/* If running within a container use the container namespace */
+	umh_token = 0;
+	if (current->nsproxy->net_ns != &init_net)
+		umh_token = umh_wq_get_token(0, "objlayout");
+
 	if (unlikely(!osd_login_prog[0])) {
 		dprintk("%s: osd_login_prog is disabled\n", __func__);
 		return -EACCES;
@@ -620,7 +626,13 @@ static int __objlayout_upcall(struct __auto_login *login)
 	argv[6] = login->systemid_hex;
 	argv[7] = NULL;
 
-	ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+	if (!umh_token)
+		ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+	else {
+		ret = call_usermodehelper_service(argv[0], argv, envp,
+						  umh_token, UMH_WAIT_PROC);
+		umh_wq_put_token(umh_token);
+	}
 	/*
 	 * Disable the upcall mechanism if we're getting an ENOENT or
 	 * EACCES error. The admin can re-enable it on the fly by using

[RFC PATCH 5 6/7] KEYS - use correct memory allocation flag in call_usermodehelper_keys()

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

When call_usermodehelper_keys() is called it assumes it won't be called
with the flag UMH_NO_WAIT. Currently that's always the case.

Change this to check the flag and use the correct kernel memory allocation
flag to guard against future changes.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 security/keys/request_key.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index 486ef6f..e865f9f 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -76,8 +76,10 @@ static int call_usermodehelper_keys(char *path, char **argv, char **envp,
 					struct key *session_keyring, int wait)
 {
 	struct subprocess_info *info;
+	unsigned int gfp_mask = (wait & UMH_NO_WAIT) ?
+					GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp, GFP_KERNEL,
+	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
 					  umh_keys_init, umh_keys_cleanup,
 					  session_keyring);
 	if (!info)

[RFC PATCH 5 7/7] KEYS: exec request key within service thread of key creator

[Ian Kent]

From: Ian Kent <ikent@redhat.com>

Containerized request key helper callbacks need the ability to execute
a binary in a container's context. To do that get a token to a service
thread created within the container environment for usermode helper calls.

Signed-off-by: Ian Kent <ikent@redhat.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Cc: Oleg Nesterov <onestero@redhat.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Jeff Layton <jeff.layton@primarydata.com>
---
 include/linux/key.h         |    3 +++
 security/keys/gc.c          |    2 ++
 security/keys/key.c         |    5 +++++
 security/keys/request_key.c |   34 +++++++++++++++++++++++++++-------
 4 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index e1d4715..144727d 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -209,6 +209,9 @@ struct key {
 		} payload;
 		struct assoc_array keys;
 	};
+
+	/* Namespace token */
+	int umh_token;
 };
 
 extern struct key *key_alloc(struct key_type *type,
diff --git a/security/keys/gc.c b/security/keys/gc.c
index c795237..c689ffd 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -156,6 +156,8 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 
 		kfree(key->description);
 
+		umh_wq_put_token(key->umh_token);
+
 #ifdef KEY_DEBUGGING
 		key->magic = KEY_DEBUG_MAGIC_X;
 #endif
diff --git a/security/keys/key.c b/security/keys/key.c
index aee2ec5..3ca0825 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -18,6 +18,8 @@
 #include <linux/workqueue.h>
 #include <linux/random.h>
 #include <linux/err.h>
+#include <net/net_namespace.h>
+#include <linux/nsproxy.h>
 #include "internal.h"
 
 struct kmem_cache *key_jar;
@@ -309,6 +311,9 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 	/* publish the key by giving it a serial number */
 	atomic_inc(&user->nkeys);
 	key_alloc_serial(key);
+	/* If running within a container use the container namespace */
+	if (current->nsproxy->net_ns != &init_net)
+		key->umh_token = umh_wq_get_token(0, "keys");
 
 error:
 	return key;
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index e865f9f..233e837 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -70,26 +70,40 @@ static void umh_keys_cleanup(struct subprocess_info *info)
 }
 
 /*
- * Call a usermode helper with a specific session keyring.
+ * Call a usermode helper with a specific session keyring and execute
+ * within a namespace.
  */
-static int call_usermodehelper_keys(char *path, char **argv, char **envp,
-					struct key *session_keyring, int wait)
+static int call_usermodehelper_keys_service(char *path,
+					    char **argv, char **envp,
+					    struct key *session_keyring,
+					    int token, unsigned int wait)
 {
 	struct subprocess_info *info;
 	unsigned int gfp_mask = (wait & UMH_NO_WAIT) ?
 					GFP_ATOMIC : GFP_KERNEL;
 
 	info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
-					  umh_keys_init, umh_keys_cleanup,
-					  session_keyring);
+					 umh_keys_init, umh_keys_cleanup,
+					 session_keyring);
 	if (!info)
 		return -ENOMEM;
+	info->wq_token = token;
 
 	key_get(session_keyring);
 	return call_usermodehelper_exec(info, wait);
 }
 
 /*
+ * Call a usermode helper with a specific session keyring.
+ */
+static int call_usermodehelper_keys(char *path, char **argv, char **envp,
+					struct key *session_keyring, int wait)
+{
+	return call_usermodehelper_keys_service(path, argv, envp,
+						session_keyring, 0, wait);
+}
+
+/*
  * Request userspace finish the construction of a key
  * - execute "/sbin/request-key <op> <key> <uid> <gid> <keyring> <keyring> <keyring>"
  */
@@ -174,8 +188,14 @@ static int call_sbin_request_key(struct key_construction *cons,
 	argv[i] = NULL;
 
 	/* do it */
-	ret = call_usermodehelper_keys(argv[0], argv, envp, keyring,
-				       UMH_WAIT_PROC);
+	/* If running within a container use the container namespace */
+	if (key->umh_token)
+		ret = call_usermodehelper_keys_service(argv[0], argv, envp,
+						       keyring, key->umh_token,
+						       UMH_WAIT_PROC);
+	else
+		ret = call_usermodehelper_keys(argv[0], argv, envp,
+					       keyring, UMH_WAIT_PROC);
 	kdebug("usermode -> 0x%x", ret);
 	if (ret >= 0) {
 		/* ret is the exit/wait code */

Re: [RFC PATCH 5 1/7] kmod - add workqueue service thread store

[David Howells]

Ian Kent <raven@themaw.net> wrote:

> +static struct umh_wq_entry *umh_wq_find_entry(int token)
> +{
> +	struct umh_wq_entry *this, *entry;
> +	struct hlist_head *bucket;
> +	unsigned int hash;
> +
> +	hash = hash_32((unsigned long) token, UMH_WQ_HASH_SHIFT);
> +	bucket = &umh_wq_hash[hash];
> +
> +	entry = ERR_PTR(-ENOENT);
> +	if (hlist_empty(bucket))
> +		goto out;
> +
> +	hlist_for_each_entry(this, bucket, umh_wq_hlist) {
> +		if (this->token == token) {
> +			entry = this;
> +			break;
> +		}
> +	}
> +out:
> +	return entry;
> +}

Can "struct umh_wq_entry *" be used as the token?

David

Re: [RFC PATCH 5 1/7] kmod - add workqueue service thread store

[Ian Kent]

On Thu, 2015-04-02 at 13:43 +0100, David Howells wrote:
> Ian Kent <raven@themaw.net> wrote:
> 
> > +static struct umh_wq_entry *umh_wq_find_entry(int token)
> > +{
> > +	struct umh_wq_entry *this, *entry;
> > +	struct hlist_head *bucket;
> > +	unsigned int hash;
> > +
> > +	hash = hash_32((unsigned long) token, UMH_WQ_HASH_SHIFT);
> > +	bucket = &umh_wq_hash[hash];
> > +
> > +	entry = ERR_PTR(-ENOENT);
> > +	if (hlist_empty(bucket))
> > +		goto out;
> > +
> > +	hlist_for_each_entry(this, bucket, umh_wq_hlist) {
> > +		if (this->token == token) {
> > +			entry = this;
> > +			break;
> > +		}
> > +	}
> > +out:
> > +	return entry;
> > +}
> 
> Can "struct umh_wq_entry *" be used as the token?

Probably not, for example.

Couldn't a user set a different workqueue_struct and have it used for
execution. Not sure what that would get the user but it sounds like the
original reason we couldn't allow execution directly within the caller
environment.

> 
> David

Re: [RFC PATCH 5 7/7] KEYS: exec request key within service thread of key creator

[David Howells]

Ian Kent <raven@themaw.net> wrote:

> +
> +	/* Namespace token */
> +	int umh_token;

If you could put it after data_len so that all the smaller-than-wordsize
fields are together for better packing.

> +		umh_wq_put_token(key->umh_token);

Does gc.c need an extra #include for this?

> +	/* If running within a container use the container namespace */
> +	if (current->nsproxy->net_ns != &init_net)
> +		key->umh_token = umh_wq_get_token(0, "keys");

So keys live in the networking namespace?

> -	ret = call_usermodehelper_keys(argv[0], argv, envp, keyring,
> -				       UMH_WAIT_PROC);
> +	/* If running within a container use the container namespace */
> +	if (key->umh_token)
> +		ret = call_usermodehelper_keys_service(argv[0], argv, envp,
> +						       keyring, key->umh_token,
> +						       UMH_WAIT_PROC);
> +	else
> +		ret = call_usermodehelper_keys(argv[0], argv, envp,
> +					       keyring, UMH_WAIT_PROC);

call_usermodehelper_keys_service() would appear to be superfluous.  If
key->umh_token is 0, you call call_usermodehelper_keys() which then calls
call_usermodehelper_keys_service() with a 0 token...

David

Re: [RFC PATCH 5 7/7] KEYS: exec request key within service thread of key creator

[Ian Kent]

On Thu, 2015-04-02 at 13:58 +0100, David Howells wrote:
> Ian Kent <raven@themaw.net> wrote:
> 
> > +
> > +	/* Namespace token */
> > +	int umh_token;
> 
> If you could put it after data_len so that all the smaller-than-wordsize
> fields are together for better packing.

OK.

> 
> > +		umh_wq_put_token(key->umh_token);
> 
> Does gc.c need an extra #include for this?

Umm ... you'd think so, wonder how it compiled without kmod.h ....

> 
> > +	/* If running within a container use the container namespace */
> > +	if (current->nsproxy->net_ns != &init_net)
> > +		key->umh_token = umh_wq_get_token(0, "keys");
> 
> So keys live in the networking namespace?

Perhaps checking the pid namespace would make more sense?

> 
> > -	ret = call_usermodehelper_keys(argv[0], argv, envp, keyring,
> > -				       UMH_WAIT_PROC);
> > +	/* If running within a container use the container namespace */
> > +	if (key->umh_token)
> > +		ret = call_usermodehelper_keys_service(argv[0], argv, envp,
> > +						       keyring, key->umh_token,
> > +						       UMH_WAIT_PROC);
> > +	else
> > +		ret = call_usermodehelper_keys(argv[0], argv, envp,
> > +					       keyring, UMH_WAIT_PROC);
> 
> call_usermodehelper_keys_service() would appear to be superfluous.  If
> key->umh_token is 0, you call call_usermodehelper_keys() which then calls
> call_usermodehelper_keys_service() with a 0 token...

Yeah, not really worth the additional function. IIRC there are no other
callers of call_usermodehelper_keys().

> 
> David

Re: [RFC PATCH 5 6/7] KEYS - use correct memory allocation flag in call_usermodehelper_keys()

[David Howells]

Ian Kent <raven@themaw.net> wrote:

> When call_usermodehelper_keys() is called it assumes it won't be called
> with the flag UMH_NO_WAIT. Currently that's always the case.
> 
> Change this to check the flag and use the correct kernel memory allocation
> flag to guard against future changes.
> 
> Signed-off-by: Ian Kent <ikent@redhat.com>

I think this is okay.

Reviewed-by: David Howells <dhowells@redhat.com>