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

[Marco Elver <elver@google.com>]

On Mon, 10 Feb 2020 at 13:16, Qian Cai <cai@lca.pw> wrote:
>
>
>
> > On Feb 10, 2020, at 2:48 AM, Marco Elver <elver@google.com> wrote:
> >
> > Here is an alternative:
> >
> > Let's say KCSAN gives you this:
> >   /* ... Assert that the bits set in mask are not written
> > concurrently; they may still be read concurrently.
> >     The access that immediately follows is assumed to access those
> > bits and safe w.r.t. data races.
> >
> >     For example, this may be used when certain bits of @flags may
> > only be modified when holding the appropriate lock,
> >     but other bits may still be modified locklessly.
> >   ...
> >  */
> >   #define ASSERT_EXCLUSIVE_BITS(flags, mask)   ....
> >
> > Then we can write page_zonenum as follows:
> >
> > static inline enum zone_type page_zonenum(const struct page *page)
> > {
> > +       ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT);
> >        return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK;
> > }
> >
> > This will accomplish the following:
> > 1. The current code is not touched, and we do not have to verify that
> > the change is correct without KCSAN.
> > 2. We're not introducing a bunch of special macros to read bits in various ways.
> > 3. KCSAN will assume that the access is safe, and no data race report
> > is generated.
> > 4. If somebody modifies ZONES bits concurrently, KCSAN will tell you
> > about the race.
> > 5. We're documenting the code.
> >
> > Anything I missed?
>
> I don’t know. Having to write the same line twice does not feel me any better than data_race() with commenting occasionally.

Point 4 above: While data_race() will ignore cause KCSAN to not report
the data race, now you might be missing a real bug: if somebody
concurrently modifies the bits accessed, you want to know about it!
Either way, it's up to you to add the ASSERT_EXCLUSIVE_BITS, but just
remember that if you decide to silence it with data_race(), you need
to be sure there are no concurrent writers to those bits.

There is no way to automatically infer all over the kernel which bits
we care about, and the most reliable is to be explicit about it. I
don't see a problem with it per se.

Thanks,
-- Marco