On 6/24/21 12:11 PM, Dmitry Kadashev wrote: > On Wed, Jun 23, 2021 at 6:54 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote: >> >> On 6/23/21 7:41 AM, Dmitry Kadashev wrote: >>> 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
