Re: [PATCH 1/7] arm64: uaccess: Add batched kernel nofault accessors

[email protected]
Newsgroups org.kvack.linux-mm,org.infradead.lists.linux-arm-kernel,org.kernel.vger.bpf,org.kernel.vger.linux-kernel
Message-ID <4c2bee332eb4173076cebaff6988f2d2ee283d7c6fe43993e957335cdc319a6f@mail.kernel.org>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 9f5bd9c69c24..1a14eb2a51dc 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -270,28 +270,43 @@ do {									\
>  #define get_user	__get_user
>
>  /*
> - * We must not call into the scheduler between __mte_enable_tco_async() and
> - * __mte_disable_tco_async(). As `dst` and `src` may contain blocking
> - * functions, we must evaluate these outside of the critical section.
> + * Nofault load without TCO management for use inside a
> + * __begin/__end_kernel_nofault_bare() region.
>   */
> -#define __get_kernel_nofault(dst, src, type, err_label)			\
> +#define __get_kernel_nofault_bare(dst, src, type, err_label)		\
>  do {									\
>  	__typeof__(dst) __gkn_dst = (dst);				\
>  	__typeof__(src) __gkn_src = (src);				\
>  	do { 								\
>  		__label__ __gkn_label;					\
> -									\
> -		__mte_enable_tco_async();				\
>  		__raw_get_mem("ldr", *((type *)(__gkn_dst)),		\
>  		      (__force type *)(__gkn_src), __gkn_label, K);	\
> -		__mte_disable_tco_async();				\
>  		break;							\
>  	__gkn_label:							\
> -		__mte_disable_tco_async();				\
>  		goto err_label;						\
>  	} while (0);							\
>  } while (0)
>
>  /*
> + * We must not call into the scheduler between __mte_enable_tco_async() and
> + * __mte_disable_tco_async(). As dst and src may contain blocking functions,
> + * evaluate them before overriding TCO.
> + */
> +#define __get_kernel_nofault(dst, src, type, err_label)			\
> +do {									\
> +	__label__ __gkn_tco_err;					\
> +	__typeof__(dst) __gkn_tco_dst = (dst);				\
> +	__typeof__(src) __gkn_tco_src = (src);				\
> +	__mte_enable_tco_async();					\
> +	__get_kernel_nofault_bare(__gkn_tco_dst, __gkn_tco_src, type,	\
> +				     __gkn_tco_err);			\
> +	__mte_disable_tco_async();					\
> +	break;								\
> +__gkn_tco_err:								\
> +	__mte_disable_tco_async();					\
> +	goto err_label;							\
> +} while (0)

This isn't a bug, but the get and put variants now carry the same rule
in two different wordings - would it read better to keep the two
comments identical, or to have the put side just point at the get side?

The comment above __get_kernel_nofault says "As dst and src may contain
blocking functions, evaluate them before overriding TCO," while the
comment above __put_kernel_nofault keeps the original "As `dst` and
`src` may contain blocking functions, we must evaluate these outside of
the critical section" - including the backticks that the get variant
dropped.

Also, should the wrapped argument line up under the open paren here
(four tabs plus two spaces rather than five)?

> +	__get_kernel_nofault_bare(__gkn_tco_dst, __gkn_tco_src, type,	\
> +				     __gkn_tco_err);			\

The continuation line sits at column 37, while the open parenthesis of
__get_kernel_nofault_bare( sits at column 34. The same three-column
offset appears in __put_kernel_nofault at line 412.

[ ... ]

> @@ -366,28 +381,48 @@ do {									\
>
>  #define put_user	__put_user
>
> -/*
> - * We must not call into the scheduler between __mte_enable_tco_async() and
> - * __mte_disable_tco_async(). As `dst` and `src` may contain blocking
> - * functions, we must evaluate these outside of the critical section.
> - */
> -#define __put_kernel_nofault(dst, src, type, err_label)			\
> +/* Nofault store without TCO management; see __get_kernel_nofault_bare. */
> +#define __put_kernel_nofault_bare(dst, src, type, err_label)		\
>  do {									\
>  	__typeof__(dst) __pkn_dst = (dst);				\
>  	__typeof__(src) __pkn_src = (src);				\
>  									\
>  	do {								\
>  		__label__ __pkn_err;					\
> -		__mte_enable_tco_async();				\
>  		__raw_put_mem("str", *((type *)(__pkn_src)),		\
>  			      (__force type *)(__pkn_dst), __pkn_err, K);	\
> -		__mte_disable_tco_async();				\
>  		break;							\
>  	__pkn_err:							\
> -		__mte_disable_tco_async();				\
>  		goto err_label;						\
>  	} while (0);							\
> -} while(0)
> +} while (0)
> +
> +/*
> + * We must not call into the scheduler between __mte_enable_tco_async() and
> + * __mte_disable_tco_async(). As `dst` and `src` may contain blocking
> + * functions, we must evaluate these outside of the critical section.
> + */

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32751368606
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.