On Wed, Jul 7, 2021 at 9:06 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote: > > 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. Ah, that makes sense. > 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. Thanks for the explanation, Pavel! -- Dmitry Kadashev
