On Fri, Nov 13, 2026 at 09:29:37AM +0800, zhaoguohan@kylinos.cn wrote:
On Thu, Nov 12, 2026 at 05:53:47AM -0500, Michael S. Tsirkin wrote:
quoted
vhost had a TODO to replace page pinning with a version
of set_bit that works on userspace memory, since forever.
It is really best to avoid GUP on fast path entirely.
Implement set_user_bit and use it in vhost, to do just that.
[...]
quoted
lightly tested. pls test and resubmit with your tool.
Thanks, this is much simpler than my version. Two comments on
set_bit_user() before I resubmit:
quoted
+ int __err = 0; \
+ __uaccess_begin(); \
+ asm volatile("\n" \
+ "1: " LOCK_PREFIX "orb %[mask], %[ptr]\n" \
+ "2:\n" \
+ _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, \
+ %[errout]) \
+ : [errout] "=r" (__err), \
+ [ptr] "+m" (*(u8 __user *)(addr)) \
errout is only written on the fixup path, so it needs "+r", as in
__try_cmpxchg_user_asm(). With "=r" gcc drops the initialisation; here
(gcc 13.3, x86_64_defconfig) it even shares a register with mask:
1a3: lock or %dl,(%rax)
1a9: test %edx,%edx
1ab: jne 1bd <log_write+0x5d>
...
1bd: mov $0xfffffff2,%eax
so every call returns -EFAULT, and log_write() stops after the first
page.
quoted
+ __uaccess_begin(); \
No access_ok() here. vhost checks the log range when it is set up
exactly
and
I couldn't find a way around that, but GUP used to reject kernel
addresses by itself. user_access_begin() would keep the check (and the
nospec barrier) local.
What we *definitely* don't want is nospec on fast path.
quoted hunk ↗ jump to hunk
Something like this, build-tested on x86_64 and
i386 only:
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -597,19 +597,23 @@ _label: \
/* set_bit() on a byte of user memory; returns -EFAULT on fault. */
#define set_bit_user(nr, addr) \
({ \
- int __err = 0; \
- __uaccess_begin(); \
- asm volatile("\n" \
- "1: " LOCK_PREFIX "orb %[mask], %[ptr]\n" \
- "2:\n" \
- _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, \
- %[errout]) \
- : [errout] "=r" (__err), \
- [ptr] "+m" (*(u8 __user *)(addr)) \
- : [mask] "q" ((unsigned char)BIT(nr)) \
- : "memory", "cc"); \
- __uaccess_end(); \
- __err ? -EFAULT : 0; \
+ u8 __user *__p = (addr); \
+ int __err = -EFAULT; \
+ if (user_access_begin(__p, sizeof(*__p))) { \
+ __err = 0; \
+ asm volatile("\n" \
+ "1: " LOCK_PREFIX "orb %[mask], %[ptr]\n" \
+ "2:\n" \
+ _ASM_EXTABLE_TYPE_REG(1b, 2b, \
+ EX_TYPE_EFAULT_REG, \
+ %[errout]) \
+ : [errout] "+r" (__err), \
+ [ptr] "+m" (*__p) \
+ : [mask] "q" ((unsigned char)BIT(nr)) \
+ : "memory", "cc"); \
+ user_access_end(); \
+ } \
+ __err; \
})
/*
If this looks right, I'll fold it in, run my test in a VM and resubmit
with the test as a second patch.
Thanks,
Khan
Given the point is to optimize this code, let's optimize.
vhost pre-validates addresses so accesses are safe without
re-checking later.
--
MST