Re: [PATCH 1/3] comm: Introduce comm_lock seqlock to protect task->comm access

[Jiri Slaby <jirislaby@gmail.com>]

On 05/16/2011 11:19 PM, John Stultz wrote:
> The implicit rules for current->comm access being safe without locking
> are no longer true. Accessing current->comm without holding the task
> lock may result in null or incomplete strings (however, access won't
> run off the end of the string).
> 
> In order to properly fix this, I've introduced a comm_lock spinlock
> which will protect comm access and modified get_task_comm() and
> set_task_comm() to use it.
> 
> Since there are a number of cases where comm access is open-coded
> safely grabbing the task_lock(), we preserve the task locking in
> set_task_comm, so those users are also safe.
> 
> With this patch, users that access current->comm without a lock
> are still prone to null/incomplete comm strings, but it should
> be no worse then it is now.
> 
> The next step is to go through and convert all comm accesses to
> use get_task_comm(). This is substantial, but can be done bit by
> bit, reducing the race windows with each patch.
> 
> CC: Ted Ts'o <tytso@mit.edu>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: David Rientjes <rientjes@google.com>
> CC: Dave Hansen <dave@linux.vnet.ibm.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: linux-mm@kvack.org
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  fs/exec.c                 |   19 ++++++++++++++++---
>  include/linux/init_task.h |    1 +
>  include/linux/sched.h     |    5 ++---
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 5e62d26..34fa611 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -998,17 +998,28 @@ static void flush_old_files(struct files_struct * files)
>  
>  char *get_task_comm(char *buf, struct task_struct *tsk)
>  {
> -	/* buf must be at least sizeof(tsk->comm) in size */
> -	task_lock(tsk);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsk->comm_lock, flags);
>  	strncpy(buf, tsk->comm, sizeof(tsk->comm));
> -	task_unlock(tsk);
> +	spin_unlock_irqrestore(&tsk->comm_lock, flags);
>  	return buf;
>  }
>  
>  void set_task_comm(struct task_struct *tsk, char *buf)
>  {
> +	unsigned long flags;
> +
> +	/*
> +	 * XXX - Even though comm is protected by comm_lock,
> +	 * we take the task_lock here to serialize against
> +	 * current users that directly access comm.
> +	 * Once those users are removed, we can drop the
> +	 * task locking & memsetting.
> +	 */
>  	task_lock(tsk);
>  
> +	spin_lock_irqsave(&tsk->comm_lock, flags);
>  	/*
>  	 * Threads may access current->comm without holding
>  	 * the task lock, so write the string carefully.
> @@ -1018,6 +1029,8 @@ void set_task_comm(struct task_struct *tsk, char *buf)
>  	memset(tsk->comm, 0, TASK_COMM_LEN);
>  	wmb();
>  	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> +	spin_unlock_irqrestore(&tsk->comm_lock, flags);
> +
>  	task_unlock(tsk);
>  	perf_event_comm(tsk);
>  }
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index caa151f..b69d94b 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -161,6 +161,7 @@ extern struct cred init_cred;
>  	.group_leader	= &tsk,						\
>  	RCU_INIT_POINTER(.real_cred, &init_cred),			\
>  	RCU_INIT_POINTER(.cred, &init_cred),				\
> +	.comm_lock	= __SPIN_LOCK_UNLOCKED(tsk.comm_lock),		\

Hmm, you should also init the spinlock somewhere in copy_process.
Otherwise when a process is forked in the middle of [gs]et_task_comm
called on it on another cpu, you have two locked locks and only the
parent's will be unlocked, right?

>  	.comm		= "swapper",					\
>  	.thread		= INIT_THREAD,					\
>  	.fs		= &init_fs,					\
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 18d63ce..f8a7cdf 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1333,10 +1333,9 @@ struct task_struct {
>  	const struct cred __rcu *cred;	/* effective (overridable) subjective task
>  					 * credentials (COW) */
>  	struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
> -
> +	spinlock_t comm_lock;		/* protect's comm */
>  	char comm[TASK_COMM_LEN]; /* executable name excluding path
> -				     - access with [gs]et_task_comm (which lock
> -				       it with task_lock())
> +				     - access with [gs]et_task_comm
>  				     - initialized normally by setup_new_exec */
>  /* file system info */
>  	int link_count, total_link_count;

thanks,
-- 
js