Right, that was my suspicion too..
> After a little grep:
>
> andrea@laser:/usr/src/linux/mm$ grep clear_page_tables *.c
> memory.c:void clear_page_tables(struct mm_struct *mm, unsigned long first,
> int nr)
> memory.c: * clear_page_tables().
> mmap.c: clear_page_tables(mm, first, last-first);
> mmap.c: clear_page_tables(mm, 0, USER_PTRS_PER_PGD);
>
> I seen that the only piece of code that are recalling clear_page_tables()
> are exit_mmap() and free_pgtables().
Indeed. But also notice how free_pgtables() only calls it on page tables
that should have been individually zapped for all mapped areas with
"zap_page_range()" for the whole mapped area. As such, if there are page
tables in the free lists that haven't been cleared, then there is a real
bug somewhere else.
That's why I still maintain that your patch just hides the _real_ bug,
which is that something continues to use the page tables after the area
has been zapped (or possibly something fills in the page tables _beyond_
the area that the mapping was for, which would also explain why the page
table zapping didn't work).
Note that if we have page tables with stale data, that's a security
problem. And it mustn't be just hidden by making sure that we clear the
page tables at some later date .. (your patch makes sure that we clear the
page directory entries before we free them, but it still means that in the
meantime the entries have some stale stuff that we don't have a vma for,
see?)
So now do you wonder why I was looking at the shm code? The stuff I
removed was largely the code that was initializing and playing with the
page tables directly.. Exactly the kind of thing that might cause
something like this if it wasn't careful enough.
Which is why I'd like people to test out my patch (I just verified that it
at least runs doom, so while it is probably broken, it's at least not
_completely_ broken, and that makes me happier about it all).
> So my conclusing is been that free_pgtables() was calling
> clear_page_tables on not-yet-cleared pte.
I agree with your conclusion. I just disagree with your patch ;)
> So I thought a bit about the issue and the `first' and `last' variable of
> free_pgtables() looked buggy to me.
>
> My point is that zap_page_range run (in the past) starting from A to B. If
> then you call clear_page_tables() starting from `A & PGDIR_MASK' to `B +
> PGDIR_SIZE - 1', you are going to call pte_free() also on pagetables that
> are not been previously cleared by zap_page_range (outside the range
> A-B)...
Sure. It's _designed_ to clear out extra pgtable entries that can cover
areas outside the range A-B. That's the whole idea.
But they had better have been cleared already: the whole point with the
prev and next checking is to make sure that there are no active mappings
in the area, and that in turn means that they had better have been cleared
by zap_page_range() earlier. See my point?
> If this free_pgtables() aggressive-behaviour is completly safe I would
> like to understand why though ;). And if there are leaking-memory testcase
> (Alan?) I would like to take a look to understand better why we need such
> aggressive-behavior ;))). Thanks.
Think about it. free_pgtables() only gets rid of page directories that
have no mappings, and as such they must be clear. No?
Think something on the order of (written on-the-fly, I won't guarantee
this is correct, but I think you'll get the idea).
char *base = (char *)4096;
for (;;) {
base = mmap(base, 4096, PROT_READ, MAP_PRIVATE | MAP_ANON, -1, 0);
if (-1 == (int) base) {
perror("mmap");
sleep(100);
exit(0);
}
munmap(base, 4096);
base += 4096;
}
and think about what happens to the page tables with a plain 2.2.5 and
with your patches..
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/