On Fri, 2020-04-10 at 15:24 -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.
>
> Or maybe I'm the one missing something. Did you actually see a problem?
Thanks for the explanation, it is just a coverity issue, not a real
problem.
I worry about the following case: someone passes FAULT_FLAG_ALLOW_RETRY
to fixup_user_fault() with unlocked == NULL.
e.g.,
ret = fixup_user_fault(current, current->mm, uaddr,
flags | FAULT_FLAG_ALLOW_RETRY, NULL);
#define FAULT_FLAG_DEFAULT (FAULT_FLAG_ALLOW_RETRY | \
FAULT_FLAG_KILLABLE | \
FAULT_FLAG_INTERRUPTIBLE)
In fixup_user_fault(), it adds FAULT_FLAG_ALLOW_RETRY if unlocked is not
NULL, but it does not remove FAULT_FLAG_ALLOW_RETRY if unlocked is NULL.
int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
unsigned long address, unsigned int fault_flags,
bool *unlocked)
{
...
if (unlocked)
fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
...
}
Things could go wrong in the above case.
I checked the source code and there is no such case. I worry too much
about it.
Miles
>
> Linus
