linux-kernel - Re: [v3 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node

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, 5 Dec 2019 14:41:07 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Qian Cai <cai@....pw>, Yang Shi <yang.shi@...ux.alibaba.com>
CC:     <fabecassis@...dia.com>, <mhocko@...e.com>, <cl@...ux.com>,
        <vbabka@...e.cz>, <mgorman@...hsingularity.net>,
        <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [v3 PATCH] mm: move_pages: return valid node id in status if the
 page is already on the target node

On 12/5/19 2:23 PM, Qian Cai wrote:
>> On Dec 5, 2019, at 5:09 PM, Yang Shi <yang.shi@...ux.alibaba.com> wrote:
>>
>> As I said the status return value issue is a regression, but the -ENOENT issue has been there since the syscall was introduced (The visual inspection shows so I didn't actually run test against 2.6.x kernel, but it returns 0 for >= 3.10 at least). It does need further clarification (doc problem or code problem).
> 
> The question is why we should care about this change of behavior. It is arguably you are even trying to fix an ambiguous part of the manpage, but instead leave a more obviously one still broken. It is really difficult to understand the logical here.
> 

Please recall how this started: it was due to a report from a real end user, who was 
seeing a real problem. After a few emails, it was clear that there's not a good
work around available for cases like this:

* User space calls move_pages(), gets 0 (success) returned, and based on that,
proceeds to iterate through the status array.

* The status array remains untouched by the move_pages() call, so confusion and
wrong behavior ensues.

After some further discussion, we decided that the current behavior really is 
incorrect, and that it needs fixing in the kernel. Which this patch does.

>>
>> Michal also noticed several inconsistencies when he was reworking move_pages(), and I agree with him that we'd better not touch them without a clear usecase.
> 
> It could argue that there is no use case to restore the behavior either.
> 

So far, there are no reports from the field, and that's probably the key
difference between these two situations.

Hope that clears up the reasoning for you. I might add that, were you to study
all the emails in these threads, and the code and the man page, you would
probably agree with the conclusions above. You might disagree with the underlying
philosophies (such as "user space is really important and we fix it if it
breaks", etc), but that's a different conversation.

thanks,
-- 
John Hubbard
NVIDIA