Re: [PATCH][BUGFIX] cgroups: fix pid namespace bug

[Serge E. Hallyn]

Quoting Li Zefan (lizf@xxxxxxxxxxxxxx):
> The bug was introduced by commit cc31edceee04a7b87f2be48f9489ebb72d264844
> ("cgroups: convert tasks file to use a seq_file with shared pid array").
> 
> We cache a pid array for all threads that are opening the same "tasks"
> file, but the pids in the array are always from the namespace of the
> last process that opened the file, so all other threads will read pids
> from that namespace instead of their own namespaces.
> 
> To fix it, we maintain a list of pid arrays, which is keyed by pid_ns.
> The list will be of length 1 at most time.
> 
> Reported-by: Paul Menage <menage@xxxxxxxxxx>
> Idea-by: Paul Menage <menage@xxxxxxxxxx>
> Signed-off-by: Li Zefan <lizf@xxxxxxxxxxxxxx>

Reviewed-by: Serge Hallyn <serue@xxxxxxxxxx>

> ---
>  include/linux/cgroup.h |   11 ++----
>  kernel/cgroup.c        |   94 +++++++++++++++++++++++++++++++++++------------
>  2 files changed, 74 insertions(+), 31 deletions(-)
> 
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index 665fa70..20411d2 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -179,14 +179,11 @@ struct cgroup {
>  	 */
>  	struct list_head release_list;
> 
> -	/* pids_mutex protects the fields below */
> +	/* pids_mutex protects pids_list and cached pid arrays. */
>  	struct rw_semaphore pids_mutex;
> -	/* Array of process ids in the cgroup */
> -	pid_t *tasks_pids;
> -	/* How many files are using the current tasks_pids array */
> -	int pids_use_count;
> -	/* Length of the current tasks_pids array */
> -	int pids_length;
> +
> +	/* Linked list of struct cgroup_pids */
> +	struct list_head pids_list;
> 
>  	/* For RCU-protected deletion */
>  	struct rcu_head rcu_head;
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 3737a68..13dddb4 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -47,6 +47,7 @@
>  #include <linux/hash.h>
>  #include <linux/namei.h>
>  #include <linux/smp_lock.h>
> +#include <linux/pid_namespace.h>
> 
>  #include <asm/atomic.h>
> 
> @@ -960,6 +961,7 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
>  	INIT_LIST_HEAD(&cgrp->children);
>  	INIT_LIST_HEAD(&cgrp->css_sets);
>  	INIT_LIST_HEAD(&cgrp->release_list);
> +	INIT_LIST_HEAD(&cgrp->pids_list);
>  	init_rwsem(&cgrp->pids_mutex);
>  }
>  static void init_cgroup_root(struct cgroupfs_root *root)
> @@ -2201,12 +2203,30 @@ err:
>  	return ret;
>  }
> 
> +/*
> + * Cache pids for all threads in the same pid namespace that are
> + * opening the same "tasks" file.
> + */
> +struct cgroup_pids {
> +	/* The node in cgrp->pids_list */
> +	struct list_head list;
> +	/* The cgroup those pids belong to */
> +	struct cgroup *cgrp;
> +	/* The namepsace those pids belong to */
> +	struct pid_namespace *pid_ns;
> +	/* Array of process ids in the cgroup */
> +	pid_t *tasks_pids;
> +	/* How many files are using the this tasks_pids array */
> +	int pids_use_count;
> +	/* Length of the current tasks_pids array */
> +	int pids_length;
> +};
> +
>  static int cmppid(const void *a, const void *b)
>  {
>  	return *(pid_t *)a - *(pid_t *)b;
>  }
> 
> -
>  /*
>   * seq_file methods for the "tasks" file. The seq_file position is the
>   * next pid to display; the seq_file iterator is a pointer to the pid
> @@ -2221,45 +2241,47 @@ static void *cgroup_tasks_start(struct seq_file *s, loff_t *pos)
>  	 * after a seek to the start). Use a binary-search to find the
>  	 * next pid to display, if any
>  	 */
> -	struct cgroup *cgrp = s->private;
> +	struct cgroup_pids *cp = s->private;
> +	struct cgroup *cgrp = cp->cgrp;
>  	int index = 0, pid = *pos;
>  	int *iter;
> 
>  	down_read(&cgrp->pids_mutex);
>  	if (pid) {
> -		int end = cgrp->pids_length;
> +		int end = cp->pids_length;
> 
>  		while (index < end) {
>  			int mid = (index + end) / 2;
> -			if (cgrp->tasks_pids[mid] == pid) {
> +			if (cp->tasks_pids[mid] == pid) {
>  				index = mid;
>  				break;
> -			} else if (cgrp->tasks_pids[mid] <= pid)
> +			} else if (cp->tasks_pids[mid] <= pid)
>  				index = mid + 1;
>  			else
>  				end = mid;
>  		}
>  	}
>  	/* If we're off the end of the array, we're done */
> -	if (index >= cgrp->pids_length)
> +	if (index >= cp->pids_length)
>  		return NULL;
>  	/* Update the abstract position to be the actual pid that we found */
> -	iter = cgrp->tasks_pids + index;
> +	iter = cp->tasks_pids + index;
>  	*pos = *iter;
>  	return iter;
>  }
> 
>  static void cgroup_tasks_stop(struct seq_file *s, void *v)
>  {
> -	struct cgroup *cgrp = s->private;
> +	struct cgroup_pids *cp = s->private;
> +	struct cgroup *cgrp = cp->cgrp;
>  	up_read(&cgrp->pids_mutex);
>  }
> 
>  static void *cgroup_tasks_next(struct seq_file *s, void *v, loff_t *pos)
>  {
> -	struct cgroup *cgrp = s->private;
> +	struct cgroup_pids *cp = s->private;
>  	int *p = v;
> -	int *end = cgrp->tasks_pids + cgrp->pids_length;
> +	int *end = cp->tasks_pids + cp->pids_length;
> 
>  	/*
>  	 * Advance to the next pid in the array. If this goes off the
> @@ -2286,26 +2308,32 @@ static struct seq_operations cgroup_tasks_seq_operations = {
>  	.show = cgroup_tasks_show,
>  };
> 
> -static void release_cgroup_pid_array(struct cgroup *cgrp)
> +static void release_cgroup_pid_array(struct cgroup_pids *cp)
>  {
> +	struct cgroup *cgrp = cp->cgrp;
> +
>  	down_write(&cgrp->pids_mutex);
> -	BUG_ON(!cgrp->pids_use_count);
> -	if (!--cgrp->pids_use_count) {
> -		kfree(cgrp->tasks_pids);
> -		cgrp->tasks_pids = NULL;
> -		cgrp->pids_length = 0;
> +	BUG_ON(!cp->pids_use_count);
> +	if (!--cp->pids_use_count) {
> +		list_del(&cp->list);
> +		kfree(cp->tasks_pids);
> +		kfree(cp);
>  	}
>  	up_write(&cgrp->pids_mutex);
>  }
> 
>  static int cgroup_tasks_release(struct inode *inode, struct file *file)
>  {
> -	struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent);
> +	struct seq_file *seq;
> +	struct cgroup_pids *cp;
> 
>  	if (!(file->f_mode & FMODE_READ))
>  		return 0;
> 
> -	release_cgroup_pid_array(cgrp);
> +	seq = file->private_data;
> +	cp = seq->private;
> +
> +	release_cgroup_pid_array(cp);
>  	return seq_release(inode, file);
>  }
> 
> @@ -2324,6 +2352,8 @@ static struct file_operations cgroup_tasks_operations = {
>  static int cgroup_tasks_open(struct inode *unused, struct file *file)
>  {
>  	struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent);
> +	struct pid_namespace *pid_ns = task_active_pid_ns(current);
> +	struct cgroup_pids *cp;
>  	pid_t *pidarray;
>  	int npids;
>  	int retval;
> @@ -2350,20 +2380,36 @@ static int cgroup_tasks_open(struct inode *unused, struct file *file)
>  	 * array if necessary
>  	 */
>  	down_write(&cgrp->pids_mutex);
> -	kfree(cgrp->tasks_pids);
> -	cgrp->tasks_pids = pidarray;
> -	cgrp->pids_length = npids;
> -	cgrp->pids_use_count++;
> +
> +	list_for_each_entry(cp, &cgrp->pids_list, list) {
> +		if (pid_ns == cp->pid_ns)
> +			goto found;
> +	}
> +
> +	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> +	if (!cp) {
> +		up_write(&cgrp->pids_mutex);
> +		kfree(pidarray);
> +		return -ENOMEM;
> +	}
> +	cp->cgrp = cgrp;
> +	cp->pid_ns = pid_ns;
> +	list_add(&cp->list, &cgrp->pids_list);
> +found:
> +	kfree(cp->tasks_pids);
> +	cp->tasks_pids = pidarray;
> +	cp->pids_length = npids;
> +	cp->pids_use_count++;
>  	up_write(&cgrp->pids_mutex);
> 
>  	file->f_op = &cgroup_tasks_operations;
> 
>  	retval = seq_open(file, &cgroup_tasks_seq_operations);
>  	if (retval) {
> -		release_cgroup_pid_array(cgrp);
> +		release_cgroup_pid_array(cp);
>  		return retval;
>  	}
> -	((struct seq_file *)file->private_data)->private = cgrp;
> +	((struct seq_file *)file->private_data)->private = cp;
>  	return 0;
>  }
> 
> -- 
> 1.5.4.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/