linux-kernel - Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg

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: <5d4accd3-e26b-d23f-5417-debe9ad7148a@de.ibm.com>
Date:   Wed, 7 Dec 2016 11:03:29 +0100
From:   Christian Borntraeger <borntraeger@...ibm.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Vlastimil Babka <vbabka@...e.cz>, Xishi Qiu <qiuxishi@...wei.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Yaowei Bai <baiyaowei@...s.chinamobile.com>,
        Linux MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Yisheng Xie <xieyisheng1@...wei.com>
Subject: Re: [RFC PATCH v3] mm: use READ_ONCE in page_cpupid_xchg_last()

On 12/07/2016 10:59 AM, Michal Hocko wrote:
> On Wed 07-12-16 10:40:47, Christian Borntraeger wrote:
>> On 12/07/2016 10:29 AM, Vlastimil Babka wrote:
>>> On 12/07/2016 09:58 AM, Michal Hocko wrote:
>>>> On Wed 07-12-16 09:48:52, Vlastimil Babka wrote:
>>>>> On 12/07/2016 09:43 AM, Michal Hocko wrote:
>>>>>> On Tue 06-12-16 09:53:14, Xishi Qiu wrote:
>>>>>>> A compiler could re-read "old_flags" from the memory location after reading
>>>>>>> and calculation "flags" and passes a newer value into the cmpxchg making 
>>>>>>> the comparison succeed while it should actually fail.
>>>>>>>
>>>>>>> Signed-off-by: Xishi Qiu <qiuxishi@...wei.com>
>>>>>>> Suggested-by: Christian Borntraeger <borntraeger@...ibm.com>
>>>>>>> ---
>>>>>>>  mm/mmzone.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/mm/mmzone.c b/mm/mmzone.c
>>>>>>> index 5652be8..e0b698e 100644
>>>>>>> --- a/mm/mmzone.c
>>>>>>> +++ b/mm/mmzone.c
>>>>>>> @@ -102,7 +102,7 @@ int page_cpupid_xchg_last(struct page *page, int cpupid)
>>>>>>>  	int last_cpupid;
>>>>>>>  
>>>>>>>  	do {
>>>>>>> -		old_flags = flags = page->flags;
>>>>>>> +		old_flags = flags = READ_ONCE(page->flags);
>>>>>>>  		last_cpupid = page_cpupid_last(page);
>>>>>>
>>>>>> what prevents compiler from doing?
>>>>>> 		old_flags = READ_ONCE(page->flags);
>>>>>> 		flags = READ_ONCE(page->flags);
>>>>>
>>>>> AFAIK, READ_ONCE tells the compiler that page->flags is volatile. It
>>>>> can't read from volatile location more times than being told?
>>>>
>>>> But those are two different variables which we assign to so what
>>>> prevents the compiler from applying READ_ONCE on each of them
>>>> separately?
>>>
>>> I would naively expect that it's assigned to flags first, and then from
>>> flags to old_flags. But I don't know exactly the C standard evaluation
>>> rules that apply here.
>>>
>>>> Anyway, this could be addressed easily by
>>>
>>> Yes, that way there should be no doubt.
>>
>> That change would make it clearer, but the code is correct anyway,
>> as assignments in C are done from right to left, so 
>> old_flags = flags = READ_ONCE(page->flags);
>>
>> is equivalent to 
>>
>> flags = READ_ONCE(page->flags);
>> old_flags = flags;
> 
> OK, I guess you are right. For some reason I thought that the compiler
> is free to bypass flags and split an assignment
> a = b = c; into b = c; a = c
> which would still follow from right to left rule. I guess I am over
> speculating here though, so sorry for the noise.

Hmmm, just rereading C, I am no longer sure...
I cannot find anything right now, that adds a sequence point in here.
Still looking...