Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Tue, Jun 25, 2024 at 07:54:30AM +0200, Arnd Bergmann wrote:
> On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> > I believe the output constraints "=m" is not necessary, because
> > the instruction itself is "write", we don't need the compiler
> > to "write" for us. So tell compiler we read from memory instead
> > of writing.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> 
> I think this is a bit too confusing: clearly there is no
> read access from the __user pointer, so what you add in here
> is not correct. There also needs to be a code comment about

Here is my understanding: the __put_user is implemented with
sd(or its less wider variant, sw etc.), w/o considering the
ex_table, the previous code can be simplified as below:

__asm__ __volatile__ (
	"sw	%z2, %1\n"
	: "+r" (err), "=m" (*(ptr))
	: "rJ" (__x));

Here ptr is really an input, just tells gcc where to store,
And the "store" action is from the "sw" instruction, I don't
need the gcc generates "store" instruction for me. so IMHO,
there's no need to use output constraints here. so I changed
it to

__asm__ __volatile__ (
	"sw	%z1, %2\n"
	: "+r" (err)
	: "rJ" (__x), "m"(*(ptr)));

The key here: is this correct?


Here is the put_user piece code and comments from x86

/*
 * Tell gcc we read from memory instead of writing: this is because
 * we do not write to any memory gcc knows about, so there are no
 * aliasing issues.
 */
#define __put_user_goto(x, addr, itype, ltype, label)                   \
        asm goto("\n"                                                   \
                "1:     mov"itype" %0,%1\n"                             \
                _ASM_EXTABLE_UA(1b, %l2)                                \
                : : ltype(x), "m" (__m(addr))                           \
                : : label)


As can be seen, x86 also doesn't put the (addr) in output constraints,
I think x86 version did similar modification in history, but when I tried
to searh the git history, the comment is there from the git first day.

Any hint or suggestion is appreciated!

> why you do it this way, as it's not clear that this is
> a workaround for old compilers without
> CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
> 
> > index 09d4ca37522c..84b084e388a7 100644
> > --- a/arch/riscv/include/asm/uaccess.h
> > +++ b/arch/riscv/include/asm/uaccess.h
> > @@ -186,11 +186,11 @@ do {								\
> >  	__typeof__(*(ptr)) __x = x;				\
> >  	__asm__ __volatile__ (					\
> >  		"1:\n"						\
> > -		"	" insn " %z2, %1\n"			\
> > +		"	" insn " %z1, %2\n"			\
> >  		"2:\n"						\
> >  		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
> > -		: "+r" (err), "=m" (*(ptr))			\
> > -		: "rJ" (__x));					\
> > +		: "+r" (err)			\
> > +		: "rJ" (__x), "m"(*(ptr)));					\
> >  } while (0)
> > 
> 
> I suspect this could just be a "r" constraint instead of
> "m", treating the __user pointer as a plain integer.

I tried "r", the generated code is not as good as "m"

for example
__put_user(0x12, &frame->uc.uc_flags);

with "m", the generated code will be

...
csrs    sstatus,a5
li      a4,18
sd      a4,128(s1)
csrc    sstatus,a5
...


with "r", the generated code will be

...
csrs    sstatus,a5
li      a4,18
addi    s1,s1,128
sd      a4,0(s1)
csrc    sstatus,a5
...

As can be seen, "m" can make use of the 'offset' of
sd, so save one instruction.

> 
> For kernel pointers, using "m" and "=m" constraints
> correctly is necessary since gcc will often access the
> same data from C code as well. For __user pointers, we
> can probably get away without it since no C code is
> ever allowed to just dereference them. If you do that,
> you may want to have the same thing in the __get_user
> side.
> 
>       Arnd