Re: [PATCH 4/7] maccess: Use a scoped guard for page faults
From: bot+bpf-ci@kernel.org
Date: 2026-08-24 17:02:45
Also in:
bpf, linux-mm, lkml
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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