Re: [PATCH RFC] vhost: set dirty log bits without pinning pages on x86

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] vhost: missing drivers/vhost/vhost.c changes to use
  set_bit_user()
- [Low] x86/uaccess: missing local __label__ efault in set_bit_user()
--

commit 0fc009f80e8e18bd72e753ada02155e494eedeca
Author: Michael S. Tsirkin <mst@redhat.com>

vhost: set dirty log bits without pinning pages on x86

This commit adds the set_bit_user() macro in arch/x86/include/asm/uaccess.h
to atomically set a bit in a byte of user memory using scoped_user_rw_access().

> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 3a0dd3c2b233e..0bffa43e02179 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -594,6 +594,28 @@ _label:									\
>  	__ret;								\
>  							})
>  
> +/* set_bit() on a byte of user memory; returns -EFAULT on fault. */
> +#define set_bit_user(nr, addr)					\

[Severity: Medium]
Is this commit missing the changes in drivers/vhost/vhost.c to call
set_bit_user()?

The commit appears to have been applied directly from a mailing-list reply
that only contained a replacement hunk for arch/x86/include/asm/uaccess.h.
As a result, set_bit_user() has no callers in the tree and
set_bit_to_user() in drivers/vhost/vhost.c still pins user pages with
pin_user_pages_fast().

> +({									\
> +	int __err = -EFAULT;						\
> +	u8 __user *__p = (addr);					\
> +	scoped_user_rw_access(__p, efault) {				\
> +		__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");				\
> +	}								\
> +efault:									\
> +	__err;								\
> +})

[Severity: Low]
Should set_bit_user() declare __label__ efault; at the start of the
statement expression, as get_user_inline() and put_user_inline() do in
include/linux/uaccess.h?

Without __label__ efault;, the efault label has function scope, so
expanding set_bit_user() more than once in the same function (or inside a
function that already defines an efault label) will fail to compile with a
duplicate label error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113083351-mutt-send-email-mst@kernel.org?part=1