Re: [PATCH v2 03/15] powerpc/align: Convert emulate_spe() to user_access_begin

[Daniel Axtens <dja@axtens.net>]

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