linux-kernel - Re: [RESEND v2 PATCH 1/2] aio, memory-hotplug: Fix confliction when migrating and accessing ring pages.

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]

Message-ID: <531FEFC6.50709@cn.fujitsu.com>
Date:	Wed, 12 Mar 2014 13:25:26 +0800
From:	Tang Chen <tangchen@...fujitsu.com>
To:	Benjamin LaHaise <bcrl@...ck.org>
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, miaox@...fujitsu.com
Subject: Re: [RESEND v2 PATCH 1/2] aio, memory-hotplug: Fix confliction when
 migrating and accessing ring pages.


Hi Ben,

Sorry for the delay.

On 03/12/2014 02:46 AM, Benjamin LaHaise wrote:
> On Mon, Mar 10, 2014 at 04:15:33PM +0800, Tang Chen wrote:
>> IO ring page migration has been implemented by the following patch:
>>
>>          https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/fs/aio.c?id=36bc08cc01709b4a9bb563b35aa530241ddc63e3
>>
>> In this patch, ctx->completion_lock is used to prevent other processes
>> from accessing the ring page being migrated.
>>
>> But in aio_setup_ring(), ioctx_add_table() and aio_read_events_ring(),
>> when writing to the ring page, they didn't take ctx->completion_lock.
>
>> As a result, for example, we have the following problem:
> ...
>> As above, the new ring page will not be updated.
>>
>> The solution is taking ctx->completion_lock in thread 2, which means,
>> in aio_setup_ring(), ioctx_add_table() and aio_read_events_ring() when
>> writing to ring pages.
>
> Upon review, there are still two accesses of ->ring_pages that are not
> protected by any spinlocks which could potentially race with migration.  One
> is in aio_setup_ring(), which can be easily resolved by moving the assignment
> of ->ring_pages above the unlock_page().

Yes， you are right. Followed, thanks.

>Another spot is in
> aio_read_events_ring() where head and tail are fetched from the ring without
> any locking.  I also fear we'll be introducing new performance issues with
> all the additonal spinlock bouncing, despite the fact that is only ever
> needed for migration.  I'm going to continue looking into this today and
> will try to send out a followup to this email later.

In the beginning of aio_read_events_ring(), it reads head and tail, not 
write.
So even if ring pages are migrated, the contents of the pages will not 
be changed.
So reading it is OK, from old page or from the new page, I think.


>
> 		-ben
--
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/