lkml.org 
[lkml]   [2021]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 03/15] powerpc/align: Convert emulate_spe() to user_access_begin
Date
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())

> 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:

- 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. 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.

> + 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.

(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-10 23:33    [W:0.138 / U:0.860 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site