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@xxxxxxxxxx>
>>>>>>> Suggested-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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...
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>