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

[Viktor Malik]

On 10/1/26 13:33, David Hildenbrand (Arm) wrote:
> On 8/25/26 14:07, Muhammad Usama Anjum wrote:
>> 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@xxxxxxx>
>>>> ---
>>>>  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`:
> 
> Ah, sorry, I thought this was added in this series.
> 
> It's completely confusing.
> 
> 	guard(pagefault)();
> 
> meaning "disable pagefault", what?! :)
> 
> CCin Viktor: I see that the patch that introduced that has no acks from MM. I
> don't blame you, because the file is not actually listed under the MM section.

You're right, looking at it back, it's a really unfortunate name.
Renaming to nofault or disable_pagefault is certainly better.

Thanks for pointing this out.

Viktor

> 
> I think this really needs to be cleaned up to clarify that we *disable* pagefaults.
> 
> Usama, can you throw in a patch to clean that up? (calling it nofault,
> disable_pagefaults or whatever)
>