[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZnwVKs3vI9Zrtvb-@xhacker>
Date: Wed, 26 Jun 2024 21:18:34 +0800
From: Jisheng Zhang <jszhang@...nel.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of
__put_user
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@...nel.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
Powered by blists - more mailing lists