Messages in this thread | | | Date | Sun, 9 Mar 2008 10:27:20 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: bisected boot regression post 2.6.25-rc3.. please revert |
| |
On Sun, 9 Mar 2008, Ingo Molnar wrote: > > The best fix is the one below (it should solve Arjan's regression with > that now-reverted patch redone), as it is the right thing to do [that > way sign auto-extend trickles over into PAGE_MASK as well].
This *really* makes me worry.
I do agree that there are good reasons that "PAGE_MASK" should be signed (since we do want the top bit to extend), but changing "PAGE_SIZE" to signed seems to be a rather big change, considering that it's used *everywhere*.
In particular, it's quite possibly used for things like
offset = something % PAGE_SIZE;
etc, where a signed divide is positively wrong.
But even for PAGE_MASK, we literally have code like this:
if ((size_avail & PAGE_MASK) < rg.size) {
where it so _happens_ that "size_avail" is unsigned, but what if it wasn't? It could turn a unsigned comparison into a signed one, and introduce any number of security bugs etc.
Sam goes even more for PAGE_SIZE. At least there are only about a thousand users of PAGE_MASK in the kernel, PAGE_SIZE is used about six times as many times, and just a _trivial_ grep like
git grep 'PAGE_SIZE.* [<>][= ]'
finds a lot of cases where I'm not at all sure that it's safe to change PAGE_SIZE to a signed value.
In other words, there are lots of things like
if (x < PAGE_SIZE) ...
where we currently get a unsigned comparison, and where for all I know a signed PAGE_SIZE means that we should use
if (x >= 0 && x < PAGE_SIZE)
instead.
In short, I refuse to apply this patch after an -rc1 release. I suspect that I shouldn't apply something like this even *before* an -rc1, because I think it's just a really bad idea to make these types signed even if it were to give you magically easier sign extensions to "unsigned long long".
So I would *very* strongly instead argue:
- "unsigned long" is the native kernel type for all address manipulation, and thus "PAGE_SIZE" and "PAGE_MASK" should continue to have that type.
- anything that uses any other type without explicitly making sure it's safe is mis-using those macros. IOW, PAGE_MASk was *never* a type that had anything what-so-ever to do with page table entry bits, and this is purely a page table entry issue!
So my suggested patch would:
- make the page table code use a specific mask that it builds up itself, and makes sure it's of the right type and has the rigth value in whatever type "struct pte_entry" is. The fact that "pte_val()" is larger than "unsigned long" on x86-32 is very clearly a PTE issue, *not* an issue for PAGE_SIZE or PAGE_MASK.
Btw, just one look at your other patch should have convinced you of that anyway. Do you really think this is a readable patch or that the result is clean:
+#define pmd_bad_v1(x) ((pmd_val(x) & (~PAGE_MASK & ~_PAGE_USER)) != _KERNPG_TABLE) +#define pmd_bad_v2(x) ((pmd_val(x) \ & ~(PAGE_MASK | _PAGE_USER | _PAGE_PSE | _PAGE_NX)) \ != _KERNPG_TABLE)
when the real problem is that the mask you build up here isn't safe o pretty to begin with!
So make that whole "~(PAGE_MASK | _PAGE_USER | _PAGE_PSE | _PAGE_NX)" expression a nice *clean* expression that is about page table entries instead, and make *that* one be the right type and have the right bits. And suddenly the problem just goes away.
In fact, if you look at that expression, you suddenly realize that PAGE_MASK was *totally* the wrong value to use in the first place, whether sign-extended o not! Notice how
PAGE_MASK | _PAGE_NX
is already a totally senseless operation if PAGE_MASK has all high bits set!
So I think your whole argument and the patch is UTTER AND UNBELIEVABLE CRAP!
Blaming it on PAGE_MASK was totally incorrect. It has nothing to do with PAGE_MASK, and everything to do with the fact that the page table checking patch was utterly failed and pure shit.
Linus
| |