Re: [RFC][PATCH] Add prctl to set sibling thread names (take two)

[Darren Hart <dvhltc@us.ibm.com>]

Hi John,

Just a couple nitpics really, looks pretty good to me - other than the 
need for the wmb()s below.

john stultz wrote:
> This patch exports a task's comm via proc/pid/comm and
> proc/pid/task/tid/comm interfaces, and allows thread siblings to write
> to these values.

And the parent I presume?

> +	/*
> +	 * Threads may access current->comm without holding
> +	 * the task lock, so write the string carefully
> +	 * to avoid non-terminating reads. Readers without a lock
> +	 * with get the oldname, the newname or an empty string.

s/with/will/
s/oldname/old name/ (it isn't a variable right?)
s/newname/new name/ (it isn't a variable right?)

> +	 */
> +	tsk->comm[0] = NULL;
> +	/* XXX - Need an mb() here?*/

I believe you do, yes. Now, which one... hrm... checking... You only 
care about ensuring the the comm[0] store occurs BEFORE the strlcpy. 
But, if no lock is held here, you can be preempted, so this is important 
for both UP and SMP.  I believe what you need here is:

	wmb()

Memory barrier experts, please enlighten us if I am missing something.

> +	strlcpy(tsk->comm+1, buf+1, sizeof(tsk->comm)-1);

And one more here I should think, otherwise that could effectively undo 
the previous one :-)

	wmb()

> +	tsk->comm[0] = buf[0];
>  	task_unlock(tsk);

To be clear, we hold the lock to prevent other threads from changing 
this at the same time as us - any other thread but the target thread 
that is?

> +static ssize_t
> +comm_write(struct file *file, const char __user *buf,
> +	    size_t count, loff_t *offset)
> +{
> +	struct inode *inode = file->f_path.dentry->d_inode;
> +	struct task_struct *p;
> +	char buffer[TASK_COMM_LEN];
> +
> +	memset(buffer, 0, sizeof(buffer));

What purpose does zeroing this entire buffer serve?

> +	if (count > sizeof(buffer) - 1)
> +		count = sizeof(buffer) - 1;
> +	if (copy_from_user(buffer, buf, count))
> +		return -EFAULT;
> +

Extra whitespace

> +
> +	p = get_proc_task(inode);
> +	if (!p)
> +		return -ESRCH;
> +
> +	if (same_thread_group(current, p))
> +		set_task_comm(p, buffer);
> +	else
> +		count = -EINVAL;
> +
> +	put_task_struct(p);
> +
> +	return count;
> +}

Thanks,

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team