[PATCH 0/7] arm64: Batch PSTATE.TCO handling in kernel nofault loops

[PATCH 0/7] arm64: Batch PSTATE.TCO handling in kernel nofault loops

[Muhammad Usama Anjum]

With Hardware Tag-Based KASAN in asynchronous or asymmetric mode, kernel
nofault loops currently set and clear PSTATE.TCO around every access.

Introduce bare nofault accessors, batching hooks, and an internal scope
guard. Convert maccess page-fault cleanup to scoped form. Skip
page-fault setup for zero-sized kernel nofault copies and zero-length
BPF string operations. Then use bare primitives in the maccess and BPF
loops. For non-empty operations, this reduces the code-derived dynamic
MSR TCO execution count from 2N to 2. Two-string BPF comparisons fall
from 4N to 2.

The BPF changes and an earlier maccess implementation with explicit
cleanup were tested with QEMU arm64 using Hardware Tag-Based KASAN in
synchronous, asynchronous, and asymmetric modes. All nine arm64 MTE
kselftests passed in each mode, as did the 138 focused BPF string_kfuncs
and varlen subtests. The focused BPF tests also passed with the default
non-MTE arm64 CPU model. Both final maccess scoped-guard patches were
arm64 cross-built. Runtime tests have not been rerun after that
conversion.

Muhammad Usama Anjum (7):
  arm64: uaccess: Add batched kernel nofault accessors
  uaccess: Add scope guard for bare kernel nofault regions
  maccess: Skip setup for zero-sized kernel nofault copies
  maccess: Use a scoped guard for page faults
  maccess: Batch TCO handling in kernel nofault loops
  bpf: Skip setup for zero-length string kfunc operations
  bpf: Batch TCO handling in string kfuncs

 arch/arm64/include/asm/uaccess.h | 71 ++++++++++++++++++++++++--------
 include/linux/uaccess.h          | 20 +++++++++
 kernel/bpf/helpers.c             | 41 ++++++++++++------
 mm/maccess.c                     | 68 ++++++++++++++++--------------
 4 files changed, 139 insertions(+), 61 deletions(-)

-- 
2.47.3

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

[Muhammad Usama Anjum]

With Hardware Tag-Based KASAN in asynchronous or asymmetric mode, arm64
sets and clears PSTATE.TCO around every kernel nofault load or store. A
loop pays that cost for every access even though tag checking can stay
disabled until the operation finishes.

Separate TCO management from the fault-tolerant access and add begin and
end hooks for callers that want to batch several accesses. Keep the
existing accessors self-contained, and provide aliases and no-op hooks
for architectures that do not need special handling.

A context switch re-enables tag checking, so a batched region must not
schedule. Continue to evaluate accessor arguments before overriding TCO,
as those expressions may block.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 arch/arm64/include/asm/uaccess.h | 71 ++++++++++++++++++++++++--------
 include/linux/uaccess.h          | 16 +++++++
 2 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 9f5bd9c69c249..1a14eb2a51dce 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)
+
 #define __put_mem_asm(store, reg, x, addr, label, type)			\
 	asm goto(							\
 	"1:	" store "	" reg "0, [%1]\n"			\
@@ -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.
+ */
+#define __put_kernel_nofault(dst, src, type, err_label)			\
+do {									\
+	__label__ __pkn_tco_err;					\
+	__typeof__(dst) __pkn_tco_dst = (dst);				\
+	__typeof__(src) __pkn_tco_src = (src);				\
+	__mte_enable_tco_async();					\
+	__put_kernel_nofault_bare(__pkn_tco_dst, __pkn_tco_src, type,	\
+				     __pkn_tco_err);			\
+	__mte_disable_tco_async();					\
+	break;								\
+__pkn_tco_err:								\
+	__mte_disable_tco_async();					\
+	goto err_label;							\
+} while (0)
+
+/*
+ * A context switch re-enables tag checking, hence the no-scheduling
+ * requirement for a bare nofault region.
+ */
+#define __begin_kernel_nofault_bare()	__mte_enable_tco_async()
+#define __end_kernel_nofault_bare()	__mte_disable_tco_async()
 
 extern unsigned long __must_check __arch_copy_from_user(void *to, const void __user *from, unsigned long n);
 #define raw_copy_from_user(to, from, n)					\
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index eddbbb65ccc4f..7ae1854673471 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -637,6 +637,22 @@ do {							\
 
 #endif  /* !__get_kernel_nofault */
 
+/*
+ * Architectures may use the begin/end hooks to establish state shared by a
+ * sequence of bare nofault accesses. Every path out of the region must call
+ * the end hook. The region, including expressions passed to the bare
+ * accessors, must not call into the scheduler.
+ */
+#ifndef __get_kernel_nofault_bare
+#define __get_kernel_nofault_bare	__get_kernel_nofault
+#define __put_kernel_nofault_bare	__put_kernel_nofault
+#endif
+
+#ifndef __begin_kernel_nofault_bare
+#define __begin_kernel_nofault_bare()	do {} while (0)
+#define __end_kernel_nofault_bare()	do {} while (0)
+#endif
+
 /**
  * get_kernel_nofault(): safely attempt to read from a location
  * @val: read into this variable
-- 
2.47.3

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4507 bytes --]

> 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

[PATCH 2/7] uaccess: Add scope guard for bare kernel nofault regions

[Muhammad Usama Anjum]

A batched nofault region must run its end hook on every exit. Pairing the
hooks by hand makes early returns and error paths easy to get wrong.

Add a scope guard that starts the region on entry and ends it when the
scope is left. Callers can keep their natural control flow while the
architecture state remains balanced on success and failure.

No functional change until a caller uses the guard.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 include/linux/uaccess.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 7ae1854673471..b25e778ddc38a 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -653,6 +653,10 @@ do {							\
 #define __end_kernel_nofault_bare()	do {} while (0)
 #endif
 
+DEFINE_LOCK_GUARD_0(__kernel_nofault_bare,
+		    __begin_kernel_nofault_bare(),
+		    __end_kernel_nofault_bare())
+
 /**
  * get_kernel_nofault(): safely attempt to read from a location
  * @val: read into this variable
-- 
2.47.3

Re: [PATCH 2/7] uaccess: Add scope guard for bare kernel nofault regions

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 7ae185467347..b25e778ddc38 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -653,6 +653,10 @@ do {							\
>  #define __end_kernel_nofault_bare()	do {} while (0)
>  #endif
>
> +DEFINE_LOCK_GUARD_0(__kernel_nofault_bare,
> +		    __begin_kernel_nofault_bare(),
> +		    __end_kernel_nofault_bare())
> +

This isn't a bug, but would an unprefixed guard name such as
kernel_nofault_bare read more consistently at the call sites, alongside
guard(pagefault) a few hundred lines up in the same header?

Every other guard class in the tree uses an unprefixed name, including
the directly comparable one at line 303 in this same file:
DEFINE_LOCK_GUARD_0(pagefault, pagefault_disable(), pagefault_enable()).
The '__' prefix is meaningful on __begin/__end_kernel_nofault_bare()
because those are arch-implementation hooks, but the guard name is the
caller-facing spelling.

>  /**
>   * get_kernel_nofault(): safely attempt to read from a location
>   * @val: read into this variable


---
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

[PATCH 3/7] maccess: Skip setup for zero-sized kernel nofault copies

[Muhammad Usama Anjum]

A zero-sized kernel nofault copy does not enter an access loop, but it
still disables and re-enables page faults.

Zero sizes are valid. BPF probe-read helpers accept them, and KGDB memory
packets may carry a zero length.

Return before changing page-fault state when there is nothing to copy.
For reads, keep architecture-specific address validation before the fast
path so its behavior is unchanged.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 mm/maccess.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/maccess.c b/mm/maccess.c
index 486559d688583..c59a0e092d24a 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -35,6 +35,8 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
 
 	if (!copy_from_kernel_nofault_allowed(src, size))
 		return -ERANGE;
+	if (!size)
+		return 0;
 
 	pagefault_disable();
 	if (!(align & 7))
@@ -65,6 +67,9 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
 {
 	unsigned long align = 0;
 
+	if (!size)
+		return 0;
+
 	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
 		align = (unsigned long)dst | (unsigned long)src;
 
-- 
2.47.3

Re: [PATCH 3/7] maccess: Skip setup for zero-sized kernel nofault copies

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

> A zero-sized kernel nofault copy does not enter an access loop, but it
> still disables and re-enables page faults.
>
> Zero sizes are valid. BPF probe-read helpers accept them, and KGDB memory
> packets may carry a zero length.
>
> Return before changing page-fault state when there is nothing to copy.
> For reads, keep architecture-specific address validation before the fast
> path so its behavior is unchanged.

This isn't a bug, but would it help to say in the changelog that a later
patch in the series wraps these loops in a bare nofault (TCO) region and
relies on the operation never being empty, rather than framing this purely
as skipping the page-fault state change?

The current description focuses on the mechanical effect (skip
pagefault_disable/enable when there is nothing to copy), but on its own the
change appears to be a no-op: pagefault_disable and pagefault_enable only
increment and decrement current->pagefault_disabled, so a zero-size copy
already returned 0 without touching memory.


---
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

[PATCH 4/7] maccess: Use a scoped guard for page faults

[Muhammad Usama Anjum]

Kernel nofault copy and string paths open-code page-fault disable and
enable around label-based loops, duplicating cleanup on success and
failure.

Use a page-fault scope guard instead. Leaving the scope now re-enables
page faults on both paths without separate cleanup at the fault label.

No functional change.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 mm/maccess.c | 53 +++++++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/mm/maccess.c b/mm/maccess.c
index c59a0e092d24a..f695ceefe6fcc 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -38,18 +38,17 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
 	if (!size)
 		return 0;
 
-	pagefault_disable();
-	if (!(align & 7))
-		copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
-	if (!(align & 3))
-		copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
-	if (!(align & 1))
-		copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
-	copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
-	pagefault_enable();
+	scoped_guard(pagefault) {
+		if (!(align & 7))
+			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
+		if (!(align & 3))
+			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
+		if (!(align & 1))
+			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
+		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
+	}
 	return 0;
 Efault:
-	pagefault_enable();
 	return -EFAULT;
 }
 EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
@@ -73,18 +72,17 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
 	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
 		align = (unsigned long)dst | (unsigned long)src;
 
-	pagefault_disable();
-	if (!(align & 7))
-		copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
-	if (!(align & 3))
-		copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
-	if (!(align & 1))
-		copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
-	copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
-	pagefault_enable();
+	scoped_guard(pagefault) {
+		if (!(align & 7))
+			copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
+		if (!(align & 3))
+			copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
+		if (!(align & 1))
+			copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
+		copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
+	}
 	return 0;
 Efault:
-	pagefault_enable();
 	return -EFAULT;
 }
 
@@ -97,18 +95,17 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
 	if (!copy_from_kernel_nofault_allowed(unsafe_addr, count))
 		return -ERANGE;
 
-	pagefault_disable();
-	do {
-		__get_kernel_nofault(dst, src, u8, Efault);
-		dst++;
-		src++;
-	} while (dst[-1] && src - unsafe_addr < count);
-	pagefault_enable();
+	scoped_guard(pagefault) {
+		do {
+			__get_kernel_nofault(dst, src, u8, Efault);
+			dst++;
+			src++;
+		} while (dst[-1] && src - unsafe_addr < count);
+	}
 
 	dst[-1] = '\0';
 	return src - unsafe_addr;
 Efault:
-	pagefault_enable();
 	dst[0] = '\0';
 	return -EFAULT;
 }
-- 
2.47.3

Re: [PATCH 4/7] maccess: Use a scoped guard for page faults

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4289 bytes --]

> diff --git a/mm/maccess.c b/mm/maccess.c
> index c59a0e092d24..f695ceefe6fc 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -38,18 +38,17 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
>  	if (!size)
>  		return 0;
>
> -	pagefault_disable();
> -	if (!(align & 7))
> -		copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> -	if (!(align & 3))
> -		copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> -	if (!(align & 1))
> -		copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> -	copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> -	pagefault_enable();
> +	scoped_guard(pagefault) {
> +		if (!(align & 7))
> +			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> +		if (!(align & 3))
> +			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> +		if (!(align & 1))
> +			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> +		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> +	}
>  	return 0;
>  Efault:
> -	pagefault_enable();
>  	return -EFAULT;
>  }

A subsystem pattern flags this as potentially concerning: include/linux/
cleanup.h states "the expectation is that usage of 'goto' and cleanup
helpers is never mixed in the same function. I.e. for a given routine,
convert all resources that need a 'goto' cleanup to scope-based cleanup,
or convert none of them."

After this change, copy_from_kernel_nofault() declares scoped_guard(
pagefault) while still using the goto-based Efault: label. Each
copy_from_kernel_nofault_loop() expands to __get_kernel_nofault(...,
Efault), which does 'goto Efault' from inside the guard scope to a label
outside it.

The concern is whether jumping out of the guard scope with goto is the
pattern cleanup.h discourages. Is this mixing safe in practice, or should
the function be fully converted to scope-based cleanup without the goto
label?

> @@ -73,18 +72,17 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
>  	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
>  		align = (unsigned long)dst | (unsigned long)src;
>
> -	pagefault_disable();
> -	if (!(align & 7))
> -		copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
> -	if (!(align & 3))
> -		copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
> -	if (!(align & 1))
> -		copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
> -	copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
> -	pagefault_enable();
> +	scoped_guard(pagefault) {
> +		if (!(align & 7))
> +			copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
> +		if (!(align & 3))
> +			copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
> +		if (!(align & 1))
> +			copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
> +		copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
> +	}
>  	return 0;
>  Efault:
> -	pagefault_enable();
>  	return -EFAULT;
>  }

Same pattern here: copy_to_kernel_nofault() now has scoped_guard(pagefault)
with copy_to_kernel_nofault_loop() calls that 'goto Efault' out of the
guard scope.

> @@ -97,18 +95,17 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
>  	if (!copy_from_kernel_nofault_allowed(unsafe_addr, count))
>  		return -ERANGE;
>
> -	pagefault_disable();
> -	do {
> -		__get_kernel_nofault(dst, src, u8, Efault);
> -		dst++;
> -		src++;
> -	} while (dst[-1] && src - unsafe_addr < count);
> -	pagefault_enable();
> +	scoped_guard(pagefault) {
> +		do {
> +			__get_kernel_nofault(dst, src, u8, Efault);
> +			dst++;
> +			src++;
> +		} while (dst[-1] && src - unsafe_addr < count);
> +	}
>
>  	dst[-1] = '\0';
>  	return src - unsafe_addr;
>  Efault:
> -	pagefault_enable();
>  	dst[0] = '\0';
>  	return -EFAULT;
>  }

And again in strncpy_from_kernel_nofault(): scoped_guard(pagefault) with
__get_kernel_nofault(dst, src, u8, Efault) jumping out of the guard scope.

Does this refactoring align with the guidance in cleanup.h about not mixing
goto and cleanup helpers, or is there precedent for this pattern in uaccess
code?


---
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

Re: [PATCH 4/7] maccess: Use a scoped guard for page faults

[David Hildenbrand (Arm)]

On 8/24/26 18:04, Muhammad Usama Anjum wrote:
> Kernel nofault copy and string paths open-code page-fault disable and
> enable around label-based loops, duplicating cleanup on success and
> failure.
> 
> Use a page-fault scope guard instead. Leaving the scope now re-enables
> page faults on both paths without separate cleanup at the fault label.
> 
> No functional change.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
> ---
>  mm/maccess.c | 53 +++++++++++++++++++++++++---------------------------
>  1 file changed, 25 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/maccess.c b/mm/maccess.c
> index c59a0e092d24a..f695ceefe6fcc 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -38,18 +38,17 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
>  	if (!size)
>  		return 0;
>  
> -	pagefault_disable();
> -	if (!(align & 7))
> -		copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> -	if (!(align & 3))
> -		copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> -	if (!(align & 1))
> -		copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> -	copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> -	pagefault_enable();
> +	scoped_guard(pagefault) {
> +		if (!(align & 7))
> +			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> +		if (!(align & 3))
> +			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> +		if (!(align & 1))
> +			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> +		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> +	}

While I understand what pagefault_disable+pagefault_enable does, it is
completely unclear what scoped_guard(pagefault) does.

Should this be scoped_guard(disabled_pagefaults) or sth like that?

-- 
Cheers,

David

Re: [PATCH 4/7] maccess: Use a scoped guard for page faults

[Muhammad Usama Anjum]

On 25/08/2026 11:36 am, David Hildenbrand (Arm) wrote:
> On 8/24/26 18:04, Muhammad Usama Anjum wrote:
>> Kernel nofault copy and string paths open-code page-fault disable and
>> enable around label-based loops, duplicating cleanup on success and
>> failure.
>>
>> Use a page-fault scope guard instead. Leaving the scope now re-enables
>> page faults on both paths without separate cleanup at the fault label.
>>
>> No functional change.
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
>> ---
>>  mm/maccess.c | 53 +++++++++++++++++++++++++---------------------------
>>  1 file changed, 25 insertions(+), 28 deletions(-)
>>
>> diff --git a/mm/maccess.c b/mm/maccess.c
>> index c59a0e092d24a..f695ceefe6fcc 100644
>> --- a/mm/maccess.c
>> +++ b/mm/maccess.c
>> @@ -38,18 +38,17 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
>>  	if (!size)
>>  		return 0;
>>  
>> -	pagefault_disable();
>> -	if (!(align & 7))
>> -		copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
>> -	if (!(align & 3))
>> -		copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
>> -	if (!(align & 1))
>> -		copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
>> -	copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
>> -	pagefault_enable();
>> +	scoped_guard(pagefault) {
>> +		if (!(align & 7))
>> +			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
>> +		if (!(align & 3))
>> +			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
>> +		if (!(align & 1))
>> +			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
>> +		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
>> +	}
> 
> While I understand what pagefault_disable+pagefault_enable does, it is
> completely unclear what scoped_guard(pagefault) does.
> 
> Should this be scoped_guard(disabled_pagefaults) or sth like that?
> 

The `pagefault` guard is already defined in `include/linux/uaccess.h`:

  DEFINE_LOCK_GUARD_0(pagefault, pagefault_disable(), pagefault_enable())

Most users use `guard(pagefault)()`, which calls `pagefault_disable()`
in the start and `pagefault_enable()` when the enclosing scope is left.
In case of guard, the scope ends at function exit. scopped_guard() on the
other hand defines the scope with {} (probably called code block).

Use of scoped_guard makes patch 5/7 straightforward:
`scoped_guard(__kernel_nofault_bare)` is nested inside the pagefault
scope.

-- 
Thanks,
Usama

[PATCH 4/7] maccess: Use a scoped guard to re-enable page faults

[Muhammad Usama Anjum]

Kernel nofault copy and string paths open-code page-fault disable and
enable around label-based loops, duplicating cleanup on success and
failure.

Use a page-fault scope guard instead. Leaving the scope now re-enables
page faults on both paths without separate cleanup at the fault label.

No functional change.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 mm/maccess.c | 53 +++++++++++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/mm/maccess.c b/mm/maccess.c
index c59a0e092d24a..f695ceefe6fcc 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -38,18 +38,17 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
 	if (!size)
 		return 0;
 
-	pagefault_disable();
-	if (!(align & 7))
-		copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
-	if (!(align & 3))
-		copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
-	if (!(align & 1))
-		copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
-	copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
-	pagefault_enable();
+	scoped_guard(pagefault) {
+		if (!(align & 7))
+			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
+		if (!(align & 3))
+			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
+		if (!(align & 1))
+			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
+		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
+	}
 	return 0;
 Efault:
-	pagefault_enable();
 	return -EFAULT;
 }
 EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
@@ -73,18 +72,17 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
 	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS))
 		align = (unsigned long)dst | (unsigned long)src;
 
-	pagefault_disable();
-	if (!(align & 7))
-		copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
-	if (!(align & 3))
-		copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
-	if (!(align & 1))
-		copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
-	copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
-	pagefault_enable();
+	scoped_guard(pagefault) {
+		if (!(align & 7))
+			copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
+		if (!(align & 3))
+			copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
+		if (!(align & 1))
+			copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
+		copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
+	}
 	return 0;
 Efault:
-	pagefault_enable();
 	return -EFAULT;
 }
 
@@ -97,18 +95,17 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
 	if (!copy_from_kernel_nofault_allowed(unsafe_addr, count))
 		return -ERANGE;
 
-	pagefault_disable();
-	do {
-		__get_kernel_nofault(dst, src, u8, Efault);
-		dst++;
-		src++;
-	} while (dst[-1] && src - unsafe_addr < count);
-	pagefault_enable();
+	scoped_guard(pagefault) {
+		do {
+			__get_kernel_nofault(dst, src, u8, Efault);
+			dst++;
+			src++;
+		} while (dst[-1] && src - unsafe_addr < count);
+	}
 
 	dst[-1] = '\0';
 	return src - unsafe_addr;
 Efault:
-	pagefault_enable();
 	dst[0] = '\0';
 	return -EFAULT;
 }
-- 
2.47.3

[PATCH 5/7] maccess: Batch TCO handling in kernel nofault loops

[Muhammad Usama Anjum]

With Hardware Tag-Based KASAN in asynchronous or asymmetric mode, every
arm64 kernel nofault access sets and clears PSTATE.TCO. Copy and string
loops repeat that pair even though tag checking can stay disabled for the
whole operation.

Cover each non-empty operation with one bare nofault region and use bare
accessors in the loop. Leaving the region restores TCO before page faults
are enabled again, including after an access fault. Existing empty-work
checks ensure that every new region performs at least one access.

The number of dynamic MSR TCO executions therefore changes as follows:

    Work                         Before    After
    N nofault accesses          2N        2
    4 KiB nofault copy          1,024     2
    N-byte strncpy              2N        2

The 4 KiB case assumes 512 64-bit accesses. These figures come from the
control flow rather than a runtime measurement, so the time saved depends
on the CPU and workload. Generic fallbacks leave other architectures
unchanged.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 mm/maccess.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/mm/maccess.c b/mm/maccess.c
index f695ceefe6fcc..87486bf98d15a 100644
--- a/mm/maccess.c
+++ b/mm/maccess.c
@@ -19,7 +19,7 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
  */
 #define copy_from_kernel_nofault_loop(dst, src, len, type, err_label)	\
 	while (len >= sizeof(type)) {					\
-		__get_kernel_nofault(dst, src, type, err_label);	\
+		__get_kernel_nofault_bare(dst, src, type, err_label);	\
 		kmsan_check_memory(src, sizeof(type));			\
 		dst += sizeof(type);					\
 		src += sizeof(type);					\
@@ -39,13 +39,15 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
 		return 0;
 
 	scoped_guard(pagefault) {
-		if (!(align & 7))
-			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
-		if (!(align & 3))
-			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
-		if (!(align & 1))
-			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
-		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
+		scoped_guard(__kernel_nofault_bare) {
+			if (!(align & 7))
+				copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
+			if (!(align & 3))
+				copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
+			if (!(align & 1))
+				copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
+			copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
+		}
 	}
 	return 0;
 Efault:
@@ -55,7 +57,7 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
 
 #define copy_to_kernel_nofault_loop(dst, src, len, type, err_label)	\
 	while (len >= sizeof(type)) {					\
-		__put_kernel_nofault(dst, src, type, err_label);	\
+		__put_kernel_nofault_bare(dst, src, type, err_label);	\
 		instrument_write(dst, sizeof(type));			\
 		dst += sizeof(type);					\
 		src += sizeof(type);					\
@@ -73,13 +75,15 @@ long copy_to_kernel_nofault(void *dst, const void *src, size_t size)
 		align = (unsigned long)dst | (unsigned long)src;
 
 	scoped_guard(pagefault) {
-		if (!(align & 7))
-			copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
-		if (!(align & 3))
-			copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
-		if (!(align & 1))
-			copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
-		copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
+		scoped_guard(__kernel_nofault_bare) {
+			if (!(align & 7))
+				copy_to_kernel_nofault_loop(dst, src, size, u64, Efault);
+			if (!(align & 3))
+				copy_to_kernel_nofault_loop(dst, src, size, u32, Efault);
+			if (!(align & 1))
+				copy_to_kernel_nofault_loop(dst, src, size, u16, Efault);
+			copy_to_kernel_nofault_loop(dst, src, size, u8, Efault);
+		}
 	}
 	return 0;
 Efault:
@@ -96,11 +100,13 @@ long strncpy_from_kernel_nofault(char *dst, const void *unsafe_addr, long count)
 		return -ERANGE;
 
 	scoped_guard(pagefault) {
-		do {
-			__get_kernel_nofault(dst, src, u8, Efault);
-			dst++;
-			src++;
-		} while (dst[-1] && src - unsafe_addr < count);
+		scoped_guard(__kernel_nofault_bare) {
+			do {
+				__get_kernel_nofault_bare(dst, src, u8, Efault);
+				dst++;
+				src++;
+			} while (dst[-1] && src - unsafe_addr < count);
+		}
 	}
 
 	dst[-1] = '\0';
-- 
2.47.3

Re: [PATCH 5/7] maccess: Batch TCO handling in kernel nofault loops

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 4455 bytes --]

> The number of dynamic MSR TCO executions therefore changes as follows:
>
>     Work                         Before    After
>     N nofault accesses          2N        2
>     4 KiB nofault copy          1,024     2
>     N-byte strncpy              2N        2
>
> The 4 KiB case assumes 512 64-bit accesses. These figures come from the
> control flow rather than a runtime measurement, so the time saved depends
> on the CPU and workload.

This isn't a bug, but would it be worth either dropping the table in favour
of one sentence (one TCO pair per operation instead of one per access) or
backing it with a measurement on a real HW-tag-KASAN async config? Also,
could the "N nofault accesses" row say whether N accesses are within a
single batched operation?

> diff --git a/mm/maccess.c b/mm/maccess.c
> index f695ceefe6fc..87486bf98d15 100644
> --- a/mm/maccess.c
> +++ b/mm/maccess.c
> @@ -19,7 +19,7 @@ bool __weak copy_from_kernel_nofault_allowed(const void *unsafe_src,
>   */
>  #define copy_from_kernel_nofault_loop(dst, src, len, type, err_label)	\
>  	while (len >= sizeof(type)) {					\
> -		__get_kernel_nofault(dst, src, type, err_label);	\
> +		__get_kernel_nofault_bare(dst, src, type, err_label);	\
>  		kmsan_check_memory(src, sizeof(type));			\
>  		dst += sizeof(type);					\
>  		src += sizeof(type);					\
> @@ -39,13 +39,15 @@ long copy_from_kernel_nofault(void *dst, const void *src, size_t size)
>  		return 0;
>
>  	scoped_guard(pagefault) {
> -		if (!(align & 7))
> -			copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> -		if (!(align & 3))
> -			copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> -		if (!(align & 1))
> -			copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> -		copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> +		scoped_guard(__kernel_nofault_bare) {
> +			if (!(align & 7))
> +				copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);
> +			if (!(align & 3))
> +				copy_from_kernel_nofault_loop(dst, src, size, u32, Efault);
> +			if (!(align & 1))
> +				copy_from_kernel_nofault_loop(dst, src, size, u16, Efault);
> +			copy_from_kernel_nofault_loop(dst, src, size, u8, Efault);
> +		}
>  	}
>  	return 0;
>  Efault:

A subsystem pattern flags this as potentially concerning: the nested
scoped_guard(__kernel_nofault_bare) introduces a second cleanup scope
inside an existing goto-based error path. include/linux/cleanup.h states
"the expectation is that usage of 'goto' and cleanup helpers is never
mixed in the same function. I.e. for a given routine, convert all
resources that need a 'goto' cleanup to scope-based cleanup, or convert
none of them." Every goto Efault now escapes two nested __cleanup()
scopes (the inner __kernel_nofault_bare and outer pagefault), relying on
the compiler to run both destructors in the correct order. The same
pattern also appears in copy_to_kernel_nofault and
strncpy_from_kernel_nofault below.

Tracing through the implementation suggests the unwind is correct in
practice: GCC and Clang run __attribute__((cleanup)) destructors when
leaving a scope via goto, in reverse order of declaration, so goto Efault
runs __end_kernel_nofault_bare() followed by pagefault_enable() in the
expected order. include/linux/uaccess.h's scoped-access documentation also
explicitly blesses leaving these scopes "with 'return', 'break' and 'goto'
at any point".

Is this an acceptable deepening of the goto-plus-guard mixing to two
levels, or should this be restructured to avoid the nested cleanup scopes?

This isn't a bug, but could this use guard(__kernel_nofault_bare)() as the
first statement of the existing pagefault scope instead of a nested
scoped_guard(), to keep the reindentation out of the diff and match the
form used in the BPF patch later in the series?

> @@ -55,7 +57,7 @@ EXPORT_SYMBOL_GPL(copy_from_kernel_nofault);
>
>  #define copy_to_kernel_nofault_loop(dst, src, len, type, err_label)	\
>  	while (len >= sizeof(type)) {					\
> -		__put_kernel_nofault(dst, src, type, err_label);	\
> +		__put_kernel_nofault_bare(dst, src, type, err_label);	\
>  		instrument_write(dst, sizeof(type));			\
>  		dst += sizeof(type);					\
>  		src += sizeof(type);					\

[ ... ]


---
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

[PATCH 6/7] bpf: Skip setup for zero-length string kfunc operations

[Muhammad Usama Anjum]

A zero limit is valid for several length-bounded BPF string operations.
Their loops perform no load in that case, but they still enter and leave a
page-fault-disabled region.

Return the existing empty result before changing page-fault state. Keep
address validation first so an invalid pointer continues to return
-ERANGE.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 kernel/bpf/helpers.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index b3cc5c8fc8756..3574a9a5721ec 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3738,6 +3738,8 @@ static int __bpf_strncasecmp(const char *s1, const char *s2, bool ignore_case, s
 	    !copy_from_kernel_nofault_allowed(s2, 1)) {
 		return -ERANGE;
 	}
+	if (!len)
+		return 0;
 
 	guard(pagefault)();
 	for (i = 0; i < len && i < XATTR_SIZE_MAX; i++) {
@@ -3837,6 +3839,8 @@ __bpf_kfunc int bpf_strnchr(const char *s__ign, size_t count, char c)
 
 	if (!copy_from_kernel_nofault_allowed(s__ign, 1))
 		return -ERANGE;
+	if (!count)
+		return -ENOENT;
 
 	guard(pagefault)();
 	for (i = 0; i < count && i < XATTR_SIZE_MAX; i++) {
@@ -3956,6 +3960,8 @@ __bpf_kfunc int bpf_strnlen(const char *s__ign, size_t count)
 
 	if (!copy_from_kernel_nofault_allowed(s__ign, 1))
 		return -ERANGE;
+	if (!count)
+		return 0;
 
 	guard(pagefault)();
 	for (i = 0; i < count && i < XATTR_SIZE_MAX; i++) {
-- 
2.47.3

[PATCH 7/7] bpf: Batch TCO handling in string kfuncs

[Muhammad Usama Anjum]

BPF string kfuncs cannot rely on NUL termination, so they scan memory with
kernel nofault loads. With Hardware Tag-Based KASAN in asynchronous or
asymmetric mode, every arm64 load sets and clears PSTATE.TCO.

Use bare loads and hold one nofault region across each string operation.
The scope guard restores TCO before page faults are enabled again on every
exit, including an access fault.

A character comparison performs two nofault loads. For N compared
characters, the number of dynamic MSR TCO executions therefore falls from
4N to 2.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>
---
 kernel/bpf/helpers.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 3574a9a5721ec..f8c557161584e 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -3726,7 +3726,8 @@ __bpf_kfunc void __bpf_trap(void)
  *
  * Since strings are not necessarily %NUL-terminated, we cannot directly call
  * in-kernel implementations. Instead, we open-code the implementations using
- * __get_kernel_nofault instead of plain dereference to make them safe.
+ * __get_kernel_nofault_bare under guard(__kernel_nofault_bare)() to make them
+ * safe.
  */
 
 static int __bpf_strncasecmp(const char *s1, const char *s2, bool ignore_case, size_t len)
@@ -3742,9 +3743,10 @@ static int __bpf_strncasecmp(const char *s1, const char *s2, bool ignore_case, s
 		return 0;
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < len && i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&c1, s1, char, err_out);
-		__get_kernel_nofault(&c2, s2, char, err_out);
+		__get_kernel_nofault_bare(&c1, s1, char, err_out);
+		__get_kernel_nofault_bare(&c2, s2, char, err_out);
 		if (ignore_case) {
 			c1 = tolower(c1);
 			c2 = tolower(c2);
@@ -3843,8 +3845,9 @@ __bpf_kfunc int bpf_strnchr(const char *s__ign, size_t count, char c)
 		return -ENOENT;
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < count && i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&sc, s__ign, char, err_out);
+		__get_kernel_nofault_bare(&sc, s__ign, char, err_out);
 		if (sc == c)
 			return i;
 		if (sc == '\0')
@@ -3897,8 +3900,9 @@ __bpf_kfunc int bpf_strchrnul(const char *s__ign, char c)
 		return -ERANGE;
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&sc, s__ign, char, err_out);
+		__get_kernel_nofault_bare(&sc, s__ign, char, err_out);
 		if (sc == '\0' || sc == c)
 			return i;
 		s__ign++;
@@ -3929,8 +3933,9 @@ __bpf_kfunc int bpf_strrchr(const char *s__ign, int c)
 		return -ERANGE;
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&sc, s__ign, char, err_out);
+		__get_kernel_nofault_bare(&sc, s__ign, char, err_out);
 		if (sc == c)
 			last = i;
 		if (sc == '\0')
@@ -3964,8 +3969,9 @@ __bpf_kfunc int bpf_strnlen(const char *s__ign, size_t count)
 		return 0;
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < count && i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&c, s__ign, char, err_out);
+		__get_kernel_nofault_bare(&c, s__ign, char, err_out);
 		if (c == '\0')
 			return i;
 		s__ign++;
@@ -4014,12 +4020,13 @@ __bpf_kfunc int bpf_strspn(const char *s__ign, const char *accept__ign)
 	}
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&cs, s__ign, char, err_out);
+		__get_kernel_nofault_bare(&cs, s__ign, char, err_out);
 		if (cs == '\0')
 			return i;
 		for (j = 0; j < XATTR_SIZE_MAX; j++) {
-			__get_kernel_nofault(&ca, accept__ign + j, char, err_out);
+			__get_kernel_nofault_bare(&ca, accept__ign + j, char, err_out);
 			if (cs == ca || ca == '\0')
 				break;
 		}
@@ -4058,12 +4065,13 @@ __bpf_kfunc int bpf_strcspn(const char *s__ign, const char *reject__ign)
 	}
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < XATTR_SIZE_MAX; i++) {
-		__get_kernel_nofault(&cs, s__ign, char, err_out);
+		__get_kernel_nofault_bare(&cs, s__ign, char, err_out);
 		if (cs == '\0')
 			return i;
 		for (j = 0; j < XATTR_SIZE_MAX; j++) {
-			__get_kernel_nofault(&cr, reject__ign + j, char, err_out);
+			__get_kernel_nofault_bare(&cr, reject__ign + j, char, err_out);
 			if (cs == cr || cr == '\0')
 				break;
 		}
@@ -4090,9 +4098,10 @@ static int __bpf_strnstr(const char *s1, const char *s2, size_t len,
 	}
 
 	guard(pagefault)();
+	guard(__kernel_nofault_bare)();
 	for (i = 0; i < XATTR_SIZE_MAX; i++) {
 		for (j = 0; i + j <= len && j < XATTR_SIZE_MAX; j++) {
-			__get_kernel_nofault(&c2, s2 + j, char, err_out);
+			__get_kernel_nofault_bare(&c2, s2 + j, char, err_out);
 			if (c2 == '\0')
 				return i;
 			/*
@@ -4102,7 +4111,7 @@ static int __bpf_strnstr(const char *s1, const char *s2, size_t len,
 			 */
 			if (i + j == len)
 				break;
-			__get_kernel_nofault(&c1, s1 + j, char, err_out);
+			__get_kernel_nofault_bare(&c1, s1 + j, char, err_out);
 
 			if (ignore_case) {
 				c1 = tolower(c1);
-- 
2.47.3

Re: [PATCH 7/7] bpf: Batch TCO handling in string kfuncs

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3981 bytes --]

> commit 582a0a0c5c8589832f42bc3de95bce3c2fba5778
> Author: Muhammad Usama Anjum <usama.anjum@arm.com>
>
>     bpf: Batch TCO handling in string kfuncs
>
>     BPF string kfuncs cannot rely on NUL termination, so they scan memory with
>     kernel nofault loads. With Hardware Tag-Based KASAN in asynchronous or
>     asymmetric mode, every arm64 load sets and clears PSTATE.TCO.
>
>     Use bare loads and hold one nofault region across each string operation.
>     The scope guard restores TCO before page faults are enabled again on every
>     exit, including an access fault.
>
>     A character comparison performs two nofault loads. For N compared
>     characters, the number of dynamic MSR TCO executions therefore falls from
>     4N to 2.
>
>     Signed-off-by: Muhammad Usama Anjum <usama.anjum@arm.com>

This isn't a bug, but would it be worth scoping the statement to kernel
nofault accesses rather than "every arm64 load"? The TCO set/clear
happens in __get_kernel_nofault()/__put_kernel_nofault() and
load_unaligned_zeropad(), not ordinary loads. Also, the 4N-to-2 figure
applies to the two-load-per-character comparison helpers like
__bpf_strncasecmp and __bpf_strnstr, while six of the eight converted
functions (bpf_strnchr, bpf_strchrnul, bpf_strrchr, bpf_strnlen,
bpf_strspn, bpf_strcspn) perform one load per character and so go from
2N to 2. The sibling patch 022e4f83d5cb spells out its per-shape figures
in a small table.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 3574a9a5721e..f8c557161584 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -4014,12 +4020,13 @@ __bpf_kfunc int bpf_strspn(const char *s__ign, const char *accept__ign)
>  	}
>
>  	guard(pagefault)();
> +	guard(__kernel_nofault_bare)();
>  	for (i = 0; i < XATTR_SIZE_MAX; i++) {
> -		__get_kernel_nofault(&cs, s__ign, char, err_out);
> +		__get_kernel_nofault_bare(&cs, s__ign, char, err_out);
>  		if (cs == '\0')
>  			return i;
>  		for (j = 0; j < XATTR_SIZE_MAX; j++) {
> -			__get_kernel_nofault(&ca, accept__ign + j, char, err_out);
> +			__get_kernel_nofault_bare(&ca, accept__ign + j, char, err_out);
>  			if (cs == ca || ca == '\0')
>  				break;
>  		}
> @@ -4027,7 +4034,7 @@ __bpf_kfunc int bpf_strspn(const char *s__ign, const char *accept__ign)
>  	return -E2BIG;
>
>  err_out:
>  	return -EFAULT;
>  }

A subsystem pattern flags this as potentially concerning: eight
functions in this patch now contain both scope guards (guard(pagefault)()
and the newly added guard(__kernel_nofault_bare)()) and goto-based error
exit to the err_out: label. The __get_kernel_nofault_bare macro expands
to 'goto err_label' on an access fault, so bpf_strspn (shown above),
bpf_strcspn, __bpf_strnstr, and the five single-loop helpers all mix
goto and guard() cleanup in the same function. The subsystem guide
directly flags this combination.

My own reading is that this is probably unavoidable. The err_out: label
is an error-return label, not a cleanup label - its entire body is
'return -EFAULT;'. No resource is released there; both the page-fault
state and the TCO override are released by the guards' destructors,
which run on that return because err_out: sits in the same block as the
guard variables. The goto does not leave the scope, so the cleanups fire
at the return, not at the goto. The goto is imposed by the
__get_kernel_nofault*() API, which is inherently label-based. The same
mixing already existed before this patch (guard(pagefault)() plus
err_out: were introduced earlier in the series), and the sibling
conversion in mm/maccess.c (022e4f83d5cb) uses the identical shape.

Does this concern you, or is the pattern acceptable here given the
constraints of the nofault API?


---
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