Re: [PATCH 2/2] powerpc/uaccess: Move might_fault() into user_access_begin()

[Michael Ellerman <mpe@ellerman.id.au>]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 08/02/2021 à 14:57, Michael Ellerman a écrit :
>> We have a might_fault() check in __unsafe_put_user_goto(), but that is
>> dangerous as it potentially calls lots of code while user access is
>> enabled.
>> 
>> It also triggers the check Alexey added to the irq restore path to
>> catch cases like that:
>> 
>>    WARNING: CPU: 30 PID: 1 at arch/powerpc/include/asm/book3s/64/kup.h:324 arch_local_irq_restore+0x160/0x190
>>    NIP arch_local_irq_restore+0x160/0x190
>>    LR  lock_is_held_type+0x140/0x200
>>    Call Trace:
>>      0xc00000007f392ff8 (unreliable)
>>      ___might_sleep+0x180/0x320
>>      __might_fault+0x50/0xe0
>>      filldir64+0x2d0/0x5d0
>>      call_filldir+0xc8/0x180
>>      ext4_readdir+0x948/0xb40
>>      iterate_dir+0x1ec/0x240
>>      sys_getdents64+0x80/0x290
>>      system_call_exception+0x160/0x280
>>      system_call_common+0xf0/0x27c
>> 
>> So remove the might fault check from unsafe_put_user().
>> 
>> Any call to unsafe_put_user() must be inside a region that's had user
>> access enabled with user_access_begin(), so move the might_fault() in
>> there. That also allows us to drop the is_kernel_addr() test, because
>> there should be no code using user_access_begin() in order to access a
>> kernel address.
>
> x86 and mips only have might_fault() on get_user() and put_user(),
> neither on __get_user() nor on __put_user() nor on the unsafe
> alternative.

Yeah, that's their choice, or perhaps it's by accident.

arm64 on the other hand has might_fault() in all variants.

A __get_user() can fault just as much as a get_user(), so there's no
reason the check should be omitted from __get_user(), other than perhaps
some historical argument about __get_user() being the "fast" case.

> When have might_fault() in __get_user_nocheck() that is used by
> __get_user() and __get_user_allowed() ie by unsafe_get_user().
>
> Shoudln't those be dropped as well ?

That was handled by Alexey's patch, which I ended up merging with this
one:

  https://git.kernel.org/torvalds/c/7d506ca97b665b95e698a53697dad99fae813c1a


ie. we still have might_fault() in __get_user_nocheck(), but it's
guarded by a check of do_allow, so we won't call it for
__get_user_allowed().

So I think the code (in my next branch) is correct, we don't have any
might_fault() calls in unsafe regions.

But I'd still be happier if we could simplify our uaccess.h more, it's a
bit of a rats nest. We could start by making __get/put_user() ==
get/put_user() the same way arm64 did.

cheers