Christophe Leroy <christophe.le...@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
