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

[Jisheng Zhang <jszhang@kernel.org>]

On Wed, Jun 26, 2024 at 08:49:59PM +0800, Jisheng Zhang wrote:
> On Wed, Jun 26, 2024 at 08:32:38PM +0800, Jisheng Zhang wrote:
> > 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@kernel.org>
> > > 
> > > 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)
> 
> Here is the simplified put_user piece code of arm64:
> 
> #define __put_mem_asm(store, reg, x, addr, err, type)                   \
>         asm volatile(                                                   \
>         "1:     " store "       " reg "1, [%2]\n"                       \
>         "2:\n"                                                          \
>         _ASM_EXTABLE_##type##ACCESS_ERR(1b, 2b, %w0)                    \
>         : "+r" (err)                                                    \
>         : "rZ" (x), "r" (addr))
> 
> no output constraints either. It just uses "r" input constraints to tell

make it accurate: by this I mean the "addr" of __put_user() isn't
in the output constraints.

> gcc to read the store address into one proper GP reg.
> 
> > 
> > 
> > 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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv