On 6/28/21 9:17 AM, Dmitry Kadashev wrote: > On Thu, Jun 24, 2021 at 7:22 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote: >> >> 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. > > Sorry for the slow reply, I have to balance this with my actual job that > is not directly related to the kernel development :) > > I'm no kernel concurrency expert (actually I'm not any kind of kernel > expert), but my understanding is READ_ONCE does not just mean "do not > read more than once", but rather "read exactly once" (and more than > that), and if it's not applied then the compiler is within its rights to > optimize the read out, so the compatibility check can effectively be > disabled. Yep, as they say it's about all the "inventive" transformations compilers can do, double read is just one of those that may turn very nasty for us. One big difference for me is whether it have a potential to crash the kernel or not, though it's just one side. Compilers can't drop the check just because, it first should be proven to be safe to do, and there are all sorts barriers around and limitations on how CQEs and SQEs are used, making impossible to alias memory. E.g. CQEs and SQEs can't be reused in a single syscall, they're only written and read respectively, and so on. Maybe, the only one I'd worry about is the call to io_commit_sqring(), i.e. for SQE reads not happening after it, but we need to take a look whether it's theoretically possible. > I don't think it's likely to happen, but "bad things do not happen in > practice" and "it is technically correct" are two different things :) > > FWIW I'm not arguing it has to be changed, I just want to understand > things better (and if it helps to spot a bug at some point then great). > So if my reasoning is wrong then please point out where. And if it's > just the simplicity / clarity of the code that is the goal here and any > negative effects are considered to be unlikely then it's OK, I can > understand that. -- Pavel Begunkov
