Re: [PATCH] mm: fix a data race in put_page()

[John Hubbard]

On 2/7/20 5:17 AM, Marco Elver wrote:
...
>> Yes. I'm grasping at straws now, but...what about the idiom that page_zonenum()
>> uses: a set of bits that are "always" (after a certain early point) read-only?
>> What are your thoughts on that?
> 
> Without annotations it's hard to tell. The problem is that the
> compiler can still emit a word-sized load, even if you're just
> checking 1 bit. The instrumentation emitted for KCSAN only cares about
> loads/stores, where access size is in number of bytes and not bits,
> since that's what the compiler has to emit.  So, strictly speaking
> these are data races: concurrent reads / writes where at least one
> access is plain.
> 
> With the above caveat out of the way, we already have the following
> defaults in KCSAN (after similar requests):
> 1. KCSAN ignores same-value stores, i.e. races with writes that appear
> to write the same value do not result in data race reports.
> 2. KCSAN does not demand aligned writes (including things like 'var++'
> if there are no concurrent writers) up to word size to be marked (with
> READ_ONCE etc.), assuming there is no way for the compiler to screw
> these up. [I still recommend writes to be marked though, if at all
> possible, because I'm still not entirely convinced it's always safe!]
> 
> So, because of (2), KCSAN will not complain if you have something like
> 'flags |= SOME_FLAG' (where the read is marked). Because of (1), it'll
> still complain about 'flags & SOME_FLAG' though, since the load is not
> marked, and only sees this is a word-sized access (assuming flags is a
> long) where a bit changed.
> 
> I don't think it's possible to easily convey to KCSAN which bits of an
> access you only care about, so that we could extend (1).   Ideas?


I'm drawing a blank as far as easy ways forward, so I'm just going accept
the compiler word-level constraints as a "given". I was hoping maybe you
had some magic available, just checking. :)


> 
>>>> A similar thing was brought up before, i.e., anything compared to zero is immune to load-tearing
>>>> issues, but it is rather difficult to implement it in the compiler, so it was settled to use data_race(),
>>>>
>>>> https://lore.kernel.org/lkml/CANpmjNN8J1oWtLPHTgCwbbtTuU_Js-8HD=cozW5cYkm8h-GTBg@xxxxxxxxxxxxxx/#r
>>>>
>>>
>>> Thanks for that link to the previous discussion, good context.
>>>
>>>>>
>>>>> b) Add a new, better-named macro to indicate what's going on. Initial bikeshed-able
>>>>>  candidates:
>>>>>
>>>>>     READ_RO_BITS()
>>>>>     READ_IMMUTABLE_BITS()
>>>>>     ...etc...
>>>>>
> 
> This could work, but 'READ_BITS()' is enough, if KCSAN's same-value
> filter is default on anyway.  Although my preference is also to avoid
> more macros if possible.


So it looks like we're probably stuck with having to annotate the code. Given
that, there is a balance between how many macros, and how much commenting. For
example, if there is a single macro (data_race, for example), then we'll need to
add comments for the various cases, explaining which data_race situation is 
happening.

That's still true, but to a lesser extent if more macros are added. In this case,
I suspect that READ_BITS() makes the commenting easier and shorter. So I'd tentatively
lead towards adding it, but what do others on the list think?



thanks,
-- 
John Hubbard
NVIDIA

> 
>>>> Actually, Linus might hate those kinds of complication rather than a simple data_race() macro,
>>>>
>>>> https://lore.kernel.org/linux-fsdevel/CAHk-=wg5CkOEF8DTez1Qu0XTEFw_oHhxN98bDnFqbY7HL5AB2g@xxxxxxxxxxxxxx/
>>>>
>>>
>>> Another good link. However, my macros above haven't been proposed yet, and I'm perfectly
>>> comfortable proposing something that Linus *might* (or might not!) hate. No point in
>>> guessing about it, IMHO.
>>>
>>> If you want, I'll be happy to put on my flame suit and post a patchset proposing
>>> READ_IMMUTABLE_BITS() (or a better-named thing, if someone has another name idea).  :)
>>>
>>
>> BTW, the current comment said (note, it is called âaccessâ which in this case it does read the whole word
>> rather than those 3 bits, even though it is only those bits are of interested for us),
>>
>> /*
>>  * data_race(): macro to document that accesses in an expression may conflict with
>>  * other concurrent accesses resulting in data races, but the resulting
>>  * behaviour is deemed safe regardless.
>>  *
>>  * This macro *does not* affect normal code generation, but is a hint to tooling
>>  * that data races here should be ignored.
>>  */
>>
>> Macro might have more to say.
> 
> I agree that data_race() would be the most suitable here, since
> technically it's still a data race. It just so happens that we end up
> "throwing away" most of the bits of the access, and just care about a
> few bits.
> 
> Thanks,
> -- Marco
>