linux-kernel - Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using get_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 28 Apr 2011 14:35:32 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	John Stultz <john.stultz@...aro.org>
cc:	LKML <linux-kernel@...r.kernel.org>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Dave Hansen <dave@...ux.vnet.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org
Subject: Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using
 get_task_comm()

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/