Hi, Hugh Dickins <hugh@xxxxxxxxxxx> writes: > On Sun, 17 Aug 2008, Johannes Weiner wrote: >> Hugh Dickins <hugh@xxxxxxxxxxx> 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 ;) Or we couldn't associate the problem with the source :) >> 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. The loops might have been there for long but the usage and input is prone to change. For example remap_pfn_range is used by drivers and it has the same alignment requirements. Perhaps an explicit comment in the kerneldoc? Iff there is even a problem with all these things, still looking through callsites, rereading your mails and thinking about it.. Hey, this thing is big and I try hard to get a clue ;) > 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? Yeah, I think so. > 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. Agreed. Well, in the unmap_vmas() case you can not unmap partial pages, so you would probably be able to guess correct. But I agree it should be up to the callsite. Hannes -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
