Re: [PATCH v4 4/8] arm64: uaccess: Add additional userspace GCS accessors

[Jeremy Linton <jeremy.linton@arm.com>]

Hi,


Thanks for looking at this.

On 7/23/25 4:50 AM, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
>> +/*
>> + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
>> + * an opening for non GCS pages to be used to source data. Therefore this
>> + * should only be used in contexts where that is acceptable.
>> + */
> 
> Even in user space, the GCS pages can be read with normal loads, so
> already usable as a data source if one wants to (not that it's of much
> use). So not sure the comment here is needed.

Right, but userspace isn't using it in a privileged context to emulate 
operations that have a permission check performed as part of the read 
when performed by the HW.

This comment was added in V2 following a number of conversations about 
whether this was an actual risk or something that is only a problem if a 
long set of pre-conditions hold true. Conditions which can be summarized 
as "it is too late anyway".

Hence the comment to remind people that this routine isn't assuring the 
page is correctly marked.

I will reword it a bit if that is ok.


> 
>> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
> 
> Nitpick: name this get_user_gcs() for symmetry with put_user_gcs().
> 
>> +{
>> +	unsigned long ret;
>> +	u64 load = 0;
>> +
>> +	gcsb_dsync();
> 
> Might be worth a comment here, see the one in gcs_restore_signal().

Sure,

> 
>> +	ret = copy_from_user(&load, addr, sizeof(load));
>> +	if (ret != 0)
>> +		*err = ret;
>> +	return load;
>> +}
> 
> Otherwise the patch looks fine:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>