On 6/24/21 12:11 PM, Dmitry Kadashev wrote:

On Wed, Jun 23, 2021 at 6:54 PM Pavel Begunkov [off-list ref] wrote:

quoted

On 6/23/21 7:41 AM, Dmitry Kadashev wrote:

quoted

I'd imagine READ_ONCE is to be used in those checks though, isn't it? Some of
the existing checks like this lack it too btw. I suppose I can fix those in a
separate commit if that makes sense.

When we really use a field there should be a READ_ONCE(),
but I wouldn't care about those we check for compatibility
reasons, but that's only my opinion.

I'm not sure how the compatibility check reads are special. The code is
either correct or not. If a compatibility check has correctness problems
then it's pretty much as bad as any other part of the code having such
problems, no?

If it reads and verifies a values first, e.g. index into some internal
array, and then compiler plays a joke and reloads it, we might be
absolutely screwed expecting 'segfaults', kernel data leakages and all
the fun stuff.

If that's a compatibility check, whether it's loaded earlier or later,
or whatever, it's not a big deal, the userspace can in any case change
the memory at any moment it wishes, even tightly around the moment
we're reading it.

That said, I'll just go ahead and use the approach that the rest of the
code (or rather most of it) uses (no READ_ONCE). If it needs fixing then
the whole bunch can probably be fixed in one go (either a single patch
or a series).

Thanks for your help, Pavel!

-- 
Pavel Begunkov