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

[Charlie Jenkins <charlie@rivosinc.com>]

On Fri, Jan 31, 2025 at 12:31:38PM +1100, Cyril Bur wrote:
> 
> 
> On 18/1/2025 9:34 am, Jessica Clarke wrote:
> > On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
> > > 
> > > From: Jisheng Zhang <jszhang@kernel.org>
> > > 
> > > 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.
> > 
> > That’s not what =m does. =m tells the compiler “this assembly will be
> > writing to this memory location, and needs the address of the location
> > for that operand”. If you move it from an output to an input then you
> > get the same assembly generated, but the compiler believes you are
> > *reading* from that memory, not *writing* to it, and so does not
> > believe the memory may be clobbered.
> > 
> > Now, it may well be that, by virtue of this being userspace memory that
> > is only ever accessed via assembly, and any inline assembly being
> > marked volatile, this in effect doesn’t matter. But it is still
> > technically wrong to model it that way, and you cannot use the
> > justification you are using, because it is false, and demonstrates a
> > lack of understanding for how inline assembly works. It has to be
> > correctly justified.
> > 
> > (I do note that neither x86 nor arm64 seems to model this as an output)
> > 
> > Jess
> 
> Good points, I agree. I am happy to respin this patch
> move the =m to output constraint.
> 
> @Charlie Please let me know if you would like me to
> keep your tags on the tweaked version.

Can you remove the tags? Copy me on the next version and I will review
it again.

- Charlie

> 
> Thanks,
> 
> Cyril
> > 
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
> > > ---
> > > arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
> > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> > > 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)
> > > 
> > > #ifdef CONFIG_64BIT
> > > @@ -203,16 +203,16 @@ do { \
> > > u64 __x = (__typeof__((x)-(x)))(x); \
> > > __asm__ __volatile__ ( \
> > > "1:\n" \
> > > - " sw %z3, %1\n" \
> > > + " sw %z1, %3\n" \
> > > "2:\n" \
> > > - " sw %z4, %2\n" \
> > > + " sw %z2, %4\n" \
> > > "3:\n" \
> > > _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0) \
> > > _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0) \
> > > - : "+r" (err), \
> > > - "=m" (__ptr[__LSW]), \
> > > - "=m" (__ptr[__MSW]) \
> > > - : "rJ" (__x), "rJ" (__x >> 32)); \
> > > + : "+r" (err) \
> > > + : "rJ" (__x), "rJ" (__x >> 32), \
> > > + "m" (__ptr[__LSW]), \
> > > + "m" (__ptr[__MSW])); \
> > > } while (0)
> > > #endif /* CONFIG_64BIT */
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > 
> 

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