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

[Christophe Leroy]

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@xxxxxxxxxx>
>
> Kind regards,
> Daniel