On Wed, Mar 9, 2022 at 9:19 PM Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Wed, Mar 9, 2022 at 11:35 AM Andreas Gruenbacher <agruenba@xxxxxxxxxx> wrote: > > > > That's better, thanks. > > Ok, can you give this one more test? > > It has that simplified loop, but it also replaced the > FAULT_FLAG_KILLABLE with just passing in 'unlocked'. > > I thought I didn't need to do that, but the "retry" loop inside > fixup_user_fault() will actually set that 'unlocked' thing even if the > caller doesn't care whether the mmap_sem was unlocked during the call, > so we have to pass in that pointer just to get that to work. > > And we don't care if mmap_sem was dropped, because this loop doesn't > cache any vma information or anything like that, but we don't want to > get a NULL pointer oops just because fixup_user_fault() tries to > inform us about something we don't care about ;) It's a moot point now, but I don't think handle_mm_fault would have returned VM_FAULT_RETRY without FAULT_FLAG_ALLOW_RETRY, so there wouldn't have been any NULL pointer accesses. > That incidentally gets us FAULT_FLAG_ALLOW_RETRY too, which is > probably a good thing anyway - it means that the mmap_sem will be > dropped if we wait for IO. Not likely a huge deal, but it's the > RightThing(tm) to do. Looking good. > So this has some other changes there too, but on the whole the > function is now really quite simple. But it would be good to have one > final round of testing considering how many small details changed.. Sure, we'll put it through all our tests. Thanks, Andreas
