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

[Christophe Leroy]

Michael Ellerman <mpe at ellerman.id.au> a écrit :

> Christophe Leroy <christophe.leroy at 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.

I agree there are several easy simplifications to do there. I'll look  
at that in the coming weeks.

I'm not sure it is good to make __get/put_user equal get/put_user as  
it would mean calling access_ok() everytime. But we could most likely  
make something simpler with get_user() calling access_ok() then  
__get_user().

I think we should also audit our use of the _inatomic variants.  
might_fault() voids when pagefault is disabled so I think the inatomic  
variants should be needed. As far as I can see, powerpc is the only  
arch having that.

Need to also check why we still need that is_kernel_addr() check  
because since the removal of set_fs(), __get/put_user() helpers  
shouldn't be used anymore for kernel addresses

Christophe