Re: [GIT PULL] x86/mm for 6.4

Dave Hansen
2023-05-02T15:42:55Z

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On 4/28/23 17:38, Kirill A. Shutemov wrote:
> BTW, I think the static check can be relaxed. Checking size against
> PAGE_SIZE is rather conservative: there's 8 TB (or 4 PB for 5-level
> paging) guard hole at the begging of kernel address space.

Whatever we relax it to, let's make sure we get a note in
Documentation/x86/x86_64/mm.rst. But that's totally minor and we can
fix it up later.

Have anyone seen any actual code generation difference between:

return (long)ptr >= 0;

and

return !((unsigned long)ptr & (1UL<<(BITS_PER_LONG-1)));

? I'm seeing gcc generate the same code for both the <=PAGE_SIZE side
and the 'sum' side.

It's longer, but I'd rather read the explicit "check bit 63" than the
positive/negative address space thing. I certainly grok both, but have
to think through the "(long)ptr >= 0" check every time.

I guess it also wouldn't matter as much either if we hid it in a helper
like the attached patch and I didn't have to read it twice. ;)

[-- Attachment #2: ptr_in_user_half.patch --]
[-- Type: text/x-patch, Size: 2540 bytes --]

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 123135d60f72..7bb11d5a7f8f 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -77,15 +77,28 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,

#ifdef CONFIG_X86_64
/*
- * On x86-64, we may have tag bits in the user pointer. Rather than
- * mask them off, just change the rules for __access_ok().
+ * The virtual address space space is logically divided into a kernel
+ * half and a user half. When cast to a signed type, user pointers
+ * are positive and kernel pointers are negative.
+ */
+static inline bool ptr_in_user_half(void *ptr)
+{
+ return (long)ptr >= 0;
+}
+
+/*
+ * User pointers can have tag bits on x86-64. This scheme tolerates
+ * arbitrary values in those bits rather masking them off.
+ *
+ * Enforce two rules:
+ * 1. 'ptr' must be in the user half of the address space
+ * 2. 'ptr+size' must not overflow (back into the kernel half)
*
- * Make the rule be that 'ptr+size' must not overflow, and must not
- * have the high bit set. Compilers generally understand about
- * unsigned overflow and the CF bit and generate reasonable code for
- * this. Although it looks like the combination confuses at least
- * clang (and instead of just doing an "add" followed by a test of
- * SF and CF, you'll see that unnecessary comparison).
+ * Compilers generally understand about unsigned overflow and the CF
+ * bit and generate reasonable code for this. Although it looks like
+ * the combination confuses at least clang (and instead of just doing
+ * an "add" followed by a test of SF and CF, you'll see that
+ * unnecessary comparison).
*
* For the common case of small sizes that can be checked at compile
* time, don't even bother with the addition, and just check that the
@@ -93,11 +106,16 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
*/
static inline bool __access_ok(const void __user *ptr, unsigned long size)
{
+ /*
+ * Check only the pointer (not ptr+size) for small accesses.
+ * This is OK because the kernel address space begins with a
+ * >=PAGE_SIZE guard hole.
+ */
if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
- return (long)ptr >= 0;
+ return ptr_in_user_half(ptr);
} else {
unsigned long sum = size + (unsigned long)ptr;
- return (long) sum >= 0 && sum >= (unsigned long)ptr;
+ return ptr_in_user_half(ptr) && sum >= (unsigned long)ptr;
}
}
#define __access_ok __access_ok