On Wed, Mar 9, 2022 at 10:42 AM Andreas Gruenbacher <agruenba@xxxxxxxxxx> wrote:
>
> + while (start != end) {
> + if (fixup_user_fault(mm, start, fault_flags, NULL))
> + goto out;
> + start += PAGE_SIZE;
> + }
> + mmap_read_unlock(mm);
> +
> +out:
> + if (size > (unsigned long)uaddr - start)
> + return size - ((unsigned long)uaddr - start);
> + return 0;
> }
What?
That "goto out" is completely broken. It explicitly avoids the
"mmap_read_unlock()" for some reason I can't for the life of me
understand.
You must have done that on purpose, since a simple "break" would have
been the sane and simple thing to do, but it looks *entirely* wrong to
me.
I think the whole loop should just be
mmap_read_lock(mm);
do {
if (fixup_user_fault(mm, start, fault_flags, NULL))
break;
start = (start + PAGE_SIZE) & PAGE_MASK;
} while (start != end);
mmap_read_unlock(mm);
which also doesn't need that first unlooped iteration (not that I
think that passing in the non-masked starting address for the first
case actually helps, but that's a different thing).
Linus
