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

[Ian Kent <raven@themaw.net>]

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