Re: [PATCH] mm: make unmap_vmas() handle non-page-aligned boundary addresses

[Hugh Dickins <hugh@veritas.com>]

On Sun, 17 Aug 2008, Johannes Weiner wrote:
> Hugh Dickins <hugh@veritas.com> writes:
> 
> I will try and help debugging this further.

Thanks!

> > You're right that those pgd_addr_end() etc. loops have an implicit
> > and fragile dependence on the page alignment of addr and end.  They
> > were written that way to maximize efficiency and be homogeneous
> > across the levels, while handling the wrapped end 0 case.  But both
> > fast gup and pagewalk have stumbled on those assumptions recently.
> 
> Yeah, especially since they could cause silent page table corruption :(

Silent?  I guess those'll be the cases we've not heard about ;)

> 
> In this respect, I still think that my patch has a point.  Because yes,
> the looping depends on page aligned boundaries, but we don't check for
> this required dependency and values leading to overruns are able to pass
> through, as explained above.

I don't think the patch you sent had a lot of point: if there is a 
problem, it extends way beyond just the entry to unmap_vmas(); and
really it's not the well-established loops we have to worry about,
it's where people add new ones without thinking about alignment.

If we put alignment BUG_ONs at the start of every such loop,
yes, that would help the new ones to follow the same pattern.
Or if we put alignment VM_BUG_ONs inside p?d_addr_next(), that
might help too - I say VM_BUG_ONs because we don't really want
to slow down the usual config, though that would then miss any
cases of vma corruption in the wild.

But even if we did so, it looks like we go for a long while only
testing the page-aligned cases anyway (which, barring corruption,
is always the case coming from vm_start and vm_end: the exceptions
are things like fault addresses or atypical I/O sizes), which
would not BUG anyway.  As soon as someone does try the unaligned,
we veer off to an unbounded loop and hit something nasty quite
noisily, don't we?

I do think there's a message about review and testing here, but
not a great case for BUGs.  Well, you didn't BUG, you enforced
alignment; but if the input is wrong, you cannot tell whether
to round up or round down in there, so better to BUG or WARN.

Hugh