Re: [patch V3 07/12] uaccess: Provide scoped masked user access regions

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2025-10-17 06:09, Thomas Gleixner wrote:

> +/**
> + * __scoped_user_access_begin - Start the masked user access
> + * @_mode:	The mode of the access class (read, write, rw)
> + * @_uptr:	The pointer to access user space memory
> + * @_size:	Size of the access
> + * @_elbl:	Error label to goto when the access region is rejected.
> + *
> + * Internal helper for __scoped_masked_user_access(). Don't use directly
> + */

^ general comment about ending sentences with '.' across this patch
(nit).


> +#define __scoped_user_access_begin(_mode, _uptr, _size, _elbl)		\
> +({									\
> +	typeof((_uptr)) ____ret;					\
> +									\
> +	if (can_do_masked_user_access()) {				\
> +		____ret = masked_user_##_mode##_access_begin((_uptr));	\

I don't think the extra () are needed here, or is there something
special happening within this macro that requires it ?

> +	} else {							\
> +		____ret = _uptr;					\
> +		if (!user_##_mode##_access_begin(_uptr, (_size)))	\

likewise around _size.

> +*/
> +#define __scoped_masked_user_access(_mode, _uptr, _size, _elbl)					\
> +for (bool ____stop = false; !____stop; ____stop = true)						\
> +	for (typeof((_uptr)) _tmpptr = __scoped_user_access_begin(_mode, _uptr, _size, _elbl);	\

The extra () around _uptr seems useless.

> +	     !____stop; ____stop = true)							\
> +		for (CLASS(masked_user_##_mode##_access, scope) (_tmpptr); !____stop;		\

Removing the space before (_tmpptr) would make it clearer that it
behaves as arguments to CLASS(masked_user_##_mode##_access, scope),
similarly to what is done in cleanup.h:scoped_class().

Nesting those constructs will cause variables to be hidden by inner
definitions. I recommend using __UNIQUE_ID() to make sure the "stop" and
"tmpptr" variables don't clash with external ones rather than trying to
solve the issue with a random amount of leading underscores.

> +		     ____stop = true)					\
> +			/* Force modified pointer usage within the scope */			\
> +			for (const typeof((_uptr)) _uptr = _tmpptr; !____stop; ____stop = true)	\

I'm puzzled that it does not trigger compiler warnings as it shadows
_uptr if _uptr is a variable defined outside of this scope.

> +				if (1)
> +

^ can be removed (as pointed out by someone else already).

[...]
> +#define scoped_masked_user_read_access_size(_usrc, _size, _elbl)		\
> +	__scoped_masked_user_access(read, (_usrc), (_size), _elbl)

Useless () around _usrc and _size.


> +#define scoped_masked_user_read_access(_usrc, _elbl)				\
> +	scoped_masked_user_read_access_size((_usrc), sizeof(*(_usrc)), _elbl)

() around the first argument are useless.


> +#define scoped_masked_user_write_access_size(_udst, _size, _elbl)		\
> +	__scoped_masked_user_access(write, (_udst),  (_size), _elbl)

Useless () around _udst and _size.

> + */
> +#define scoped_masked_user_write_access(_udst, _elbl)				\
> +	scoped_masked_user_write_access_size((_udst), sizeof(*(_udst)), _elbl)

() around the first argument are useless.

> +#define scoped_masked_user_rw_access_size(_uptr, _size, _elbl)			\
> +	__scoped_masked_user_access(rw, (_uptr), (_size), _elbl)

Useless () around _uptr and _size.

> +#define scoped_masked_user_rw_access(_uptr, _elbl)				\
> +	scoped_masked_user_rw_access_size((_uptr), sizeof(*(_uptr)), _elbl)

() around the first argument are useless.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com