On Fri, Apr 10, 2020 at 03:24:37PM -0700, Linus Torvalds wrote: > On Fri, Apr 10, 2020 at 2:32 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > In fixup_user_fault(), it is possible that unlocked is NULL, > > so we should test unlocked before using it. > > This seems wrong. > > > For example, in arch/arc/kernel/process.c, NULL is passed > > to fixup_user_fault(). > > > > ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr, > > FAULT_FLAG_WRITE, NULL); > > Yes, but it doesn't set FAULT_FLAG_ALLOW_RETRY, exactly _because_ > 'unlocked' is NULL. > > Basically, retry is fundamentally tied to that "unlocked" flag. You > can't ask for retry without also saying "please tell me if you > unlocked the mmap_sem during the retry". So the two go hand in hand > there. > > So I think this is just coverity not understanding the rules. Yeah that's also my understanding - VM_FAULT_RETRY should depend on FAULT_FLAG_ALLOW_RETRY, which further depends on unlocked != NULL. Though I'm not sure how to teach Coverity about the fact. Maybe a "BUG_ON(unlocked == NULL)" (which contains an unreachable() inside) before referencing unlocked? Thanks, > > Or maybe I'm the one missing something. Did you actually see a problem? > > Linus > -- Peter Xu
