lkml.org 
[lkml]   [2021]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 03/15] powerpc/align: Convert emulate_spe() to user_access_begin
From
Date


Le 10/03/2021 à 23:31, Daniel Axtens a écrit :
> Hi Christophe,
>
>> This patch converts emulate_spe() to using user_access_being
> s/being/begin/ :)
>> logic.
>>
>> Since commit 662bbcb2747c ("mm, sched: Allow uaccess in atomic with
>> pagefault_disable()"), might_fault() doesn't fire when called from
>> sections where pagefaults are disabled, which must be the case
>> when using _inatomic variants of __get_user and __put_user. So
>> the might_fault() in user_access_begin() is not a problem.
> (likewise with the might_fault() in __get_user_nocheck, called from
> unsafe_get_user())

unsafe_get_user() call __get_user_nocheck() with do_allow = false, so there is no might_fault() there.

>
>> There was a verification of user_mode() together with the access_ok(),
>> but the function returns in case !user_mode() immediately after
>> the access_ok() verification, so removing that test has no effect.
>
> I agree that removing the test is safe.
>
>> - /* Verify the address of the operand */
>> - if (unlikely(user_mode(regs) &&
>> - !access_ok(addr, nb)))
>> - return -EFAULT;
>> -
>
> I found the reasoning a bit confusing: I think it's safe to remove
> because:

Ok, I'll see if I can rephrase it.

>
> - we have the usermode check immediately following it:
>
>> /* userland only */
>> if (unlikely(!user_mode(regs)))
>> return 0;
>
> - and then we have the access_ok() check as part of
> user_read_access_begin later on in the function:
>
>> + if (!user_read_access_begin(addr, nb))
>> + return -EFAULT;
>> +
>
>
>> switch (nb) {
>> case 8:
>> - ret |= __get_user_inatomic(temp.v[0], p++);
>> - ret |= __get_user_inatomic(temp.v[1], p++);
>> - ret |= __get_user_inatomic(temp.v[2], p++);
>> - ret |= __get_user_inatomic(temp.v[3], p++);
>> + unsafe_get_user(temp.v[0], p++, Efault_read);
>> + unsafe_get_user(temp.v[1], p++, Efault_read);
>> + unsafe_get_user(temp.v[2], p++, Efault_read);
>> + unsafe_get_user(temp.v[3], p++, Efault_read);
>
> This will bail early rather than trying every possible read. I think
> that's OK.

It tries every possible read, but at the end it bails out with EFAULT, so I see no point.

> I can't think of a situation where we could fail to read the
> first byte and then successfully read later bytes, for example. Also I
> can't think of a sane way userspace could depend on that behaviour. So I
> agree with this change (and the change to the write path).
>
>> fallthrough;
>> case 4:
>> - ret |= __get_user_inatomic(temp.v[4], p++);
>> - ret |= __get_user_inatomic(temp.v[5], p++);
>> + unsafe_get_user(temp.v[4], p++, Efault_read);
>> + unsafe_get_user(temp.v[5], p++, Efault_read);
>> fallthrough;
>> case 2:
>> - ret |= __get_user_inatomic(temp.v[6], p++);
>> - ret |= __get_user_inatomic(temp.v[7], p++);
>> - if (unlikely(ret))
>> - return -EFAULT;
>> + unsafe_get_user(temp.v[6], p++, Efault_read);
>> + unsafe_get_user(temp.v[7], p++, Efault_read);
>> }
>> + user_read_access_end();
>>
>> switch (instr) {
>> case EVLDD:
>> @@ -255,31 +250,41 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
>>
>> /* Store result to memory or update registers */
>> if (flags & ST) {
>> - ret = 0;
>> p = addr;
>> +
>> + if (!user_read_access_begin(addr, nb))
>
> That should be a user_write_access_begin.

Good catch thanks.

>
>> + return -EFAULT;
>> +
>
>
>>
>> return 1;
>> +
>> +Efault_read:
>
> Checkpatch complains that this is CamelCase, which seems like a
> checkpatch problem. Efault_{read,write} seem like good labels to me.

I'm not keen of names mixing capital letters and lowercase, but Efault is the label that has been
used almost everywhere with unsafe_get/put_user(), so I inclined myself.

>
> (You don't need to change anything, I just like to check the checkpatch
> results when reviewing a patch.)
>
>> + user_read_access_end();
>> + return -EFAULT;
>> +
>> +Efault_write:
>> + user_write_access_end();
>> + return -EFAULT;
>> }
>> #endif /* CONFIG_SPE */
>>
>
> With the user_write_access_begin change:
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel
>

\
 
 \ /
  Last update: 2021-03-11 06:46    [W:0.219 / U:1.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site