linux-kernel - Re: [PATCH v2 1/4] mm: Fix multiple evaluvations of totalram_pages and managed

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: <c2862bb0-ced1-add1-c816-21c0d4e76bbe@suse.cz>
Date:   Wed, 7 Nov 2018 09:44:00 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Michal Hocko <mhocko@...nel.org>, Arun KS <arunks@...eaurora.org>
Cc:     akpm@...ux-foundation.org, keescook@...omium.org,
        khlebnikov@...dex-team.ru, minchan@...nel.org, osalvador@...e.de,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        getarunks@...il.com
Subject: Re: [PATCH v2 1/4] mm: Fix multiple evaluvations of totalram_pages
 and managed_pages

On 11/7/18 9:20 AM, Michal Hocko wrote:
> On Tue 06-11-18 21:51:47, Arun KS wrote:

Hi,

there's typo in subject: evaluvations -> evaluations.

However, "fix" is also misleading (more below), so I'd suggest something
like:

mm: reference totalram_pages and managed_pages once per function

>> This patch is in preparation to a later patch which converts totalram_pages
>> and zone->managed_pages to atomic variables. This patch does not introduce
>> any functional changes.
> 
> I forgot to comment on this one. The patch makes a lot of sense. But I
> would be little bit more conservative and won't claim "no functional
> changes". As things stand now multiple reads in the same function are
> racy (without holding the lock). I do not see any example of an
> obviously harmful case but claiming the above is too strong of a
> statement. I would simply go with something like "Please note that
> re-reading the value might lead to a different value and as such it
> could lead to unexpected behavior. There are no known bugs as a result
> of the current code but it is better to prevent from them in principle."

However, the new code doesn't use READ_ONCE(), so the compiler is free
to read the value multiple times, and before the patch it was free to
read it just once, as the variables are not volatile. So strictly
speaking this is indeed not a functional change (if compiler decides
differently based on the patch, it's an implementation detail).

So even in my suggested subject above, 'reference' is meant as a source
code reference, not really a memory read reference. Couldn't think of a
better word though.