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]

Date:	Sun, 16 Mar 2014 10:06:05 +0800
From:	Gu Zheng <guz.fnst@...fujitsu.com>
To:	Benjamin LaHaise <bcrl@...ck.org>
CC:	Tang Chen <tangchen@...fujitsu.com>, viro@...iv.linux.org.uk,
	jmoyer@...hat.com, kosaki.motohiro@...il.com,
	kosaki.motohiro@...fujitsu.com, isimatu.yasuaki@...fujitsu.com,
	linux-fsdevel@...r.kernel.org, linux-aio@...ck.org,
	linux-kernel@...r.kernel.org, miaox@...fujitsu.com,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [RESEND v2 PATCH 1/2] aio, memory-hotplug: Fix confliction when
 migrating and accessing ring pages.

Hi Ben,
Sorry for late.
On 03/14/2014 11:14 PM, Benjamin LaHaise wrote:

> Hi Gu,
> 
> On Fri, Mar 14, 2014 at 06:25:16PM +0800, Gu Zheng wrote:
>> Hi Ben,
>> On 03/13/2014 06:17 AM, Benjamin LaHaise wrote:
>>
>>> Hello Tang,
>>>
>>> On Wed, Mar 12, 2014 at 01:25:26PM +0800, Tang Chen wrote:
>>> ... <snip> ...
>>>
>>>>> 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.
>>>
>>> Your assumption that reading it is okay is incorrect.  Since we do not have 
>>> a reference on the page at that point, it is possible that the read of the 
>>> page takes place after the page has been freed and allocated to another part 
>>> of the kernel.  This would result in the read returning invalid information.
>>
>> What about the following patch? It adds additional reference to protect the page
>> avoid being freed when we reading it.
>> ps.It is applied on linux-next(3-13).
> 
> I think that's even worse than the spinlock approach since we'll end up 
> bouncing around the struct page's cacheline in addition to spinlock we're 
> going to end up taking anyways.

But we can not use spinlock approach to avoid this issue in aio_read_events_ring(),
because we need to copy events to user space. And on the other side, it will break
the concurrency of aio_read_events_ring() and aio_complete().
Besides, IMHO, the problem you mentioned above is almost insignificant when reading
events.
Any better solution? Other guys?

Regards,
Gu 

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