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

Jisheng Zhang
2024-06-26T13:03:56Z

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