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

[bot+bpf-ci@kernel.org]

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