linux-kernel - Re: [PATCH 2/2] aio, mem-hotplug: Add memory barrier to aio ring page migration.

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, 27 Feb 2014 09:57:30 -0500
From:	Benjamin LaHaise <bcrl@...ck.org>
To:	Tang Chen <tangchen@...fujitsu.com>
Cc:	viro@...iv.linux.org.uk, jmoyer@...hat.com,
	kosaki.motohiro@...il.com, kosaki.motohiro@...fujitsu.com,
	isimatu.yasuaki@...fujitsu.com, guz.fnst@...fujitsu.com,
	linux-fsdevel@...r.kernel.org, linux-aio@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] aio, mem-hotplug: Add memory barrier to aio ring page migration.

On Thu, Feb 27, 2014 at 06:40:16PM +0800, Tang Chen wrote:
> When doing aio ring page migration, we migrated the page, and update
> ctx->ring_pages[]. Like the following:
> 
> aio_migratepage()
>  |-> migrate_page_copy(new, old)
>  |   ......				/* Need barrier here */
>  |-> ctx->ring_pages[idx] = new
> 
> Actually, we need a memory barrier between these two operations.
> Otherwise, if ctx->ring_pages[] is updated before memory copy due to
> the compiler optimization, other processes may have an opportunity
> to access to the not fully initialized new ring page.
> 
> So add a wmb to synchronize them.

The smp_wmb() is not needed after you added the taking of ctx->completion_lock 
lock since all accesses to ring_pages is then protected by the spinlock.  
Why are you adding this then?  Or have you missed adding the lock somewhere?  
Also, if you've changed the patch, it is customary to add a "v2" somewhere in 
the patch title so that I have some idea what version of the patch should be 
applied.

		-ben

> Reported-by: Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@...fujitsu.com>
> ---
>  fs/aio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 50c089c..f0ed838 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -327,6 +327,14 @@ static int aio_migratepage(struct address_space *mapping, struct page *new,
>  		pgoff_t idx;
>  		spin_lock_irqsave(&ctx->completion_lock, flags);
>  		migrate_page_copy(new, old);
> +
> +		/*
> +		 * Ensure memory copy is finished before updating
> +		 * ctx->ring_pages[]. Otherwise other processes may access to
> +		 * new ring pages which are not fully initialized.
> +		 */
> +		smp_wmb();
> +
>  		idx = old->index;
>  		if (idx < (pgoff_t)ctx->nr_pages) {
>  			/* And only do the move if things haven't changed */
> -- 
> 1.8.3.1

-- 
"Thought is the essence of where you are now."
--
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/