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@xxxxxxx>
> CC: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> CC: David Rientjes <rientjes@xxxxxxxxxx>
> CC: Dave Hansen <dave@xxxxxxxxxxxxxxxxxx>
> CC: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> CC: linux-mm@xxxxxxxxx
> Acked-by: David Rientjes <rientjes@xxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>