Re: [PATCHv3 11/17] arm64: uaccess: refactor __{get,put}_user

From: Mark Rutland <mark.rutland@arm.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: catalin.marinas@arm.com, will@kernel.org, james.morse@arm.com,
	hch@lst.de, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCHv3 11/17] arm64: uaccess: refactor __{get,put}_user
Date: Mon, 2 Nov 2020 10:25:18 +0000	[thread overview]
Message-ID: <20201102102518.GB71783@C02TD0UTHF1T.local> (raw)
In-Reply-To: <bf2bb221-f25c-fb8f-6c76-d63540f17fd5@arm.com>

On Tue, Oct 27, 2020 at 06:03:19PM +0000, Robin Murphy wrote:
> On 2020-10-26 13:31, Mark Rutland wrote:
> > As a step towards implementing __{get,put}_kernel_nofault(), this patch
> > splits most user-memory specific logic out of __{get,put}_user(), with
> > the memory access and fault handling in new __{raw_get,put}_mem()
> > helpers.
> > 
> > For now the LDR/LDTR patching is left within the *get_mem() helpers, and
> > will be removed in a subsequent patch.
> > 
> > There should be no functional change as a result of this patch.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >   arch/arm64/include/asm/uaccess.h | 40 +++++++++++++++++++++++++---------------
> >   1 file changed, 25 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index d6a4e496ebc64..4ad2990241d78 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -253,7 +253,7 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> >    * The "__xxx_error" versions set the third argument to -EFAULT if an error
> >    * occurs, and leave it unchanged on success.
> >    */
> > -#define __get_user_asm(instr, alt_instr, reg, x, addr, err, feature)	\
> > +#define __get_mem_asm(instr, alt_instr, reg, x, addr, err, feature)	\
> >   	asm volatile(							\
> >   	"1:"ALTERNATIVE(instr "     " reg "1, [%2]\n",			\
> >   			alt_instr " " reg "1, [%2]\n", feature)		\
> > @@ -268,35 +268,40 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
> >   	: "+r" (err), "=&r" (x)						\
> >   	: "r" (addr), "i" (-EFAULT))
> > -#define __raw_get_user(x, ptr, err)					\
> > +#define __raw_get_mem(x, ptr, err)					\
> >   do {									\
> >   	unsigned long __gu_val;						\
> >   	__chk_user_ptr(ptr);						\
> 
> Should this move out as well? 

Yes, it looks like it should!

> It seems logical that wherever we figure out whether a pointer is
> "kernel" or "user" in order to call the appropriate low-level routine,
> the __user annotation could be dropped from the "kernel" path at that
> point - or have I misunderstood?

Agreed; I can't see any reason to keep this here, and it does look like
it's liable to cause sparse warnings when we add the kernel accsessors.
I'll shuffle this into the user wrappers.

Mark.

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