On Wed, Mar 9, 2022 at 8:22 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
Ouch, that was stupid. Same for the "return size".
> 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).
That's better, thanks.
Andreas
