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
--
