Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\))
Subject: Re: [PATCH] mm/util: fix a data race in __vm_enough_memory()
From:   Qian Cai <cai@lca.pw>
In-Reply-To: <20200130181834.633c201c7d0a2638aacbc7ba@linux-foundation.org>
Date:   Thu, 30 Jan 2020 21:22:05 -0500
Cc:     Marco Elver <elver@google.com>,
        Matthew Wilcox <willy@infradead.org>, dennis@kernel.org,
        tj@kernel.org, Christoph Lameter <cl@linux.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <9B86F7FF-C892-4F58-A24E-E0728D2637BC@lca.pw>
References: <20200130042011.GI6615@bombadil.infradead.org>
 <1135BD67-4CCB-4700-8150-44E7E323D385@lca.pw>
 <CANpmjNNr_Kq6Do+UYrR-3aF0sO3++psUfN7Ppt8mmgcF5ynzrA@mail.gmail.com>
 <20200130181834.633c201c7d0a2638aacbc7ba@linux-foundation.org>
To:     Andrew Morton <akpm@linux-foundation.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Jan 30, 2020, at 9:18 PM, Andrew Morton <akpm@linux-foundation.org> =
wrote:
>=20
> On Thu, 30 Jan 2020 13:35:18 +0100 Marco Elver <elver@google.com> =
wrote:
>=20
>> On Thu, 30 Jan 2020 at 12:50, Qian Cai <cai@lca.pw> wrote:
>>>=20
>>>> On Jan 29, 2020, at 11:20 PM, Matthew Wilcox <willy@infradead.org> =
wrote:
>>>>=20
>>>> I'm really not a fan of exposing the internals of a percpu_counter =
outside
>>>> the percpu_counter.h file.  Why shouldn't this be fixed by putting =
the
>>>> READ_ONCE() inside percpu_counter_read()?
>>>=20
>>> It is because not all places suffer from a data race. For example, =
in __wb_update_bandwidth(), it was protected by a lock. I was a bit =
worry about blindly adding READ_ONCE() inside percpu_counter_read() =
might has unexpected side-effect. For example, it is unnecessary to have =
READ_ONCE() for a volatile variable. So, I thought just to keep the =
change minimal with a trade off by exposing a bit internal details as =
you mentioned.
>>>=20
>>> However, I had also copied the percpu maintainers to see if they =
have any preferences?
>>=20
>> I would not add READ_ONCE to percpu_counter_read(), given the writes
>> (increments) are not atomic either, so not much is gained.
>>=20
>> Notice that this is inside a WARN_ONCE, so you may argue that a data
>> race here doesn't matter to the correct behaviour of the system
>> (except if you have panic_on_warn on).
>>=20
>> For the warning to trigger, vm_committed_as must decrease. Assume =
that
>> a data race (assuming bad compiler optimizations) can somehow
>> accomplish this, then the load or write must cause a transient value
>> to somehow be less than a stable value. My hypothesis is this is very
>> unlikely.
>>=20
>> Given the fact this is a WARN_ONCE, and the fact that a transient
>> decrease in the value is unlikely, you may consider
>> 'VM_WARN_ONCE(data_race(percpu_counter_read(&vm_committed_as)) <
>> ...)'. That way you won't modify percpu_counter_read and still catch
>> unintended races elsewhere.
>>=20
>=20
> That, or add an alternative version of per_cpu_counter_read() to the
> percpu API.  A very carefully commented version!

 I send a patch to use data_race() which should be sufficient,

https://lore.kernel.org/linux-mm/20200130145649.1240-1-cai@lca.pw/=