Re: [patch 1/4] uaccess: Provide common helpers for masked user access

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Aug 26 2025 at 09:04, Christophe Leroy wrote:

> Le 13/08/2025 à 17:57, Thomas Gleixner a écrit :
>> commit 2865baf54077 ("x86: support user address masking instead of
>> non-speculative conditional") provided an optimization for
>> unsafe_get/put_user(), which optimizes the Spectre-V1 mitigation in an
>> architecture specific way. Currently only x86_64 supports that.
>> 
>> The required code pattern is:
>> 
>> 	if (can_do_masked_user_access())
>> 		dst = masked_user_access_begin(dst);
>> 	else if (!user_write_access_begin(dst, sizeof(*dst)))
>> 		return -EFAULT;
>> 	unsafe_put_user(val, dst, Efault);
>> 	user_read_access_end();
>
> You previously called user_write_access_begin(), so must be a 
> user_write_access_end() here not a user_read_access_end().
>
>> 	return 0;
>> Efault:
>> 	user_read_access_end();
>
> Same.
>
>> 	return -EFAULT;
>> 
>> The futex code already grew an instance of that and there are other areas,
>> which can be optimized, when the calling code actually verified before,
>> that the user pointer is both aligned and actually in user space.
>> 
>> Use the futex example and provide generic helper inlines for that to avoid
>> having tons of copies all over the tree.
>> 
>> This provides get/put_user_masked_uNN() where $NN is the variable size in
>> bits, i.e. 8, 16, 32, 64.
>
> Couldn't the $NN be automatically determined through the type of the 
> provided user pointer (i.e. the 'from' and 'to' in patch 2) ?
>
>> 
>> The second set of helpers is to encapsulate the prologue for larger access
>> patterns, e.g. multiple consecutive unsafe_put/get_user() scenarioes:
>> 
>> 	if (can_do_masked_user_access())
>> 		dst = masked_user_access_begin(dst);
>> 	else if (!user_write_access_begin(dst, sizeof(*dst)))
>> 		return -EFAULT;
>> 	unsafe_put_user(a, &dst->a, Efault);
>> 	unsafe_put_user(b, &dst->b, Efault);
>> 	user_write_access_end();
>> 	return 0;
>> Efault:
>> 	user_write_access_end();
>> 	return -EFAULT;
>> 
>> which allows to shorten this to:
>> 
>> 	if (!user_write_masked_begin(dst))
>> 		return -EFAULT;
>> 	unsafe_put_user(a, &dst->a, Efault);
>> 	...
>
> That's nice but ... it hides even deeper the fact that 
> masked_user_access_begin() opens a read/write access to userspace. On 
> x86 it doesn't matter because all userspace accesses are read/write. But 
> on architectures like powerpc it becomes a problem if you do a 
> read/write open then only call user_read_access_end() as write access 
> might remain open.
>
> I have a patch (See [1]) that splits masked_user_access_begin() into 
> three versions, one for read-only, one for write-only and one for 
> read-write., so that they match user_read_access_end() 
> user_write_access_end() and user_access_end() respectively.
>
> [1] 
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/7b570e237f7099d564d7b1a270169428ac1f3099.1755854833.git.christophe.leroy@csgroup.eu/
>
>
>> 
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>   include/linux/uaccess.h |   78 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 78 insertions(+)
>> 
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -569,6 +569,84 @@ static inline void user_access_restore(u
>>   #define user_read_access_end user_access_end
>>   #endif
>>   
>> +/*
>> + * Conveniance macros to avoid spreading this pattern all over the place
>> + */
>> +#define user_read_masked_begin(src) ({					\
>> +	bool __ret = true;						\
>> +									\
>> +	if (can_do_masked_user_access())				\
>> +		src = masked_user_access_begin(src);			\
>
> Should call a masked_user_read_access_begin() to perform a read-only 
> masked access begin, matching the read-only access begin below
>
>> +	else if (!user_read_access_begin(src, sizeof(*src)))		\
>> +		__ret = false;						\
>> +	__ret;								\
>> +})
>> +
>> +#define user_write_masked_begin(dst) ({					\
>> +	bool __ret = true;						\
>> +									\
>> +	if (can_do_masked_user_access())				\
>> +		dst = masked_user_access_begin(dst);			\
>
> Should call masked_user_write_access_begin() to perform a write-only 
> masked access begin, matching the write-only access begin below
>
>> +	else if (!user_write_access_begin(dst, sizeof(*dst)))		\
>> +		__ret = false;						\
>> +	__ret;								\
>> +})
>
> You are missing a user_masked_begin() for read-write operations.

Duh. Let me go and rewrite this correctly. I clearly wasn't thinking straight.

>> +GEN_GET_USER_MASKED(u8)
>> +GEN_GET_USER_MASKED(u16)
>> +GEN_GET_USER_MASKED(u32)
>> +GEN_GET_USER_MASKED(u64)
>> +#undef GEN_GET_USER_MASKED
>
> Do we need four functions ? Can't we just have a get_user_masked() macro 
> that relies on the type of src , just like unsafe_get_user() ?

Tried and the resulting macro maze is completely unreadable
garbage. Having a readable implementation and the four functions for the
types supported was definitely more palatable. It's not too much asked
from a developer to pick the correct one.

Thanks,

        tglx