Re: [PATCH 3/3] comm: ext4: Protect task->comm access by usingget_task_comm()

[David Rientjes]

On Wed, 27 Apr 2011, John Stultz wrote:

> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 7b80d54..d37414e 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		static unsigned long unaligned_warn_time;
>  
>  		/* Warn about this once per day */
> -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> +			char comm[TASK_COMM_LEN];
> +
> +			get_task_comm(comm, current);
>  			ext4_msg(inode->i_sb, KERN_WARNING,
>  				 "Unaligned AIO/DIO on inode %ld by %s; "
>  				 "performance will be poor.",
> -				 inode->i_ino, current->comm);
> +				 inode->i_ino, comm);
> +		}
>  		mutex_lock(ext4_aio_mutex(inode));
>  		ext4_aiodio_wait(inode);
>  	}

Thanks very much for looking into concurrent readers of current->comm, 
John!

This patch in the series demonstrates one of the problems with using 
get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
and that could become risky if we don't know its current depth.  We may be 
particularly deep in the stack and then cause an overflow because of the 
16 bytes.

I'm wondering if it would be better for ->comm to be protected by a 
spinlock (or rwlock) other than ->alloc_lock and then just require readers 
to take the lock prior to dereferencing it?  That's what is done in the 
oom killer with task_lock().  Perhaps you could introduce new 
task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
in over 300 locations within the kernel?
--
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/