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

> commit 582a0a0c5c8589832f42bc3de95bce3c2fba5778
> Author: Muhammad Usama Anjum <[email protected]>
>
>     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 <[email protected]>

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