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@xxxxxxxxxx/
>
>
>>
>> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> ---
>> 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
