[PATCH] powerpc: Fix __clear_user() with KUAP enabled

[Christophe Leroy]

Le 09/12/2019 à 14:26, Andrew Donnellan a écrit :
> On 9/12/19 10:50 pm, Christophe Leroy wrote:
>>> -extern unsigned long __clear_user(void __user *addr, unsigned long 
>>> size);
>>> +extern unsigned long clear_user_asm(void __user *addr, unsigned long 
>>> size);
>>>   static inline unsigned long clear_user(void __user *addr, unsigned 
>>> long size)
>>>   {
>>> @@ -409,12 +409,17 @@ static inline unsigned long clear_user(void 
>>> __user *addr, unsigned long size)
>>>       might_fault();
>>>       if (likely(access_ok(addr, size))) {
>>>           allow_write_to_user(addr, size);
>>> -        ret = __clear_user(addr, size);
>>> +        ret = clear_user_asm(addr, size);
>>>           prevent_write_to_user(addr, size);
>>>       }
>>
>> What about changing the above by the following ?
>>
>>         if (likely(access_ok(addr, size)))        ret = 
>> __clear_user(addr, size);
>>
>>>       return ret;
>>>   }
>>> +static inline unsigned long __clear_user(void __user *addr, unsigned 
>>> long size)
>>> +{
>>> +        return clear_user(addr, size);
>>> +}
>>> +
>>
>> Then
>>
>> static inline unsigned long __clear_user(void __user *addr, unsigned 
>> long size)
>> {
>>      allow_write_to_user(addr, size);
>>      ret = clear_user_asm(addr, size);
>>      prevent_write_to_user(addr, size);
>>
>>      return ret;
>> }
> 
> This is exactly the patch I initially wrote, I ran it past mpe privately 
> and he suggested aliasing clear_user() -> __clear_user() instead, as 
> there's not much point keeping a separate path around for a single user 
> for a basically non-existent performance gain.
> 

But other arches do call __clear_user() from clear_user() and do the 
access unlock / lock in __clear_user() (at least arm and x86/64).

I think it would be better to do the same way as other arches, 
especially as __clear_user() can be called from anywhere in the kernel, 
so I'd expect it to behave the same on all arches.

I don't see it as the separate path, to me it looks more like code 
refactoring. And it follows the same logic as many other functions in 
the kernel: __the_said_function() is the light version of 
the_said_function().

Christophe