On 7/12/21 1:44 PM, Dmitry Kadashev wrote: > 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! btw, that was for why we were rather reluctant about that. It could be a good idea to add READ_ONCE as you mentioned, at least would be less confusing. -- Pavel Begunkov
