[LTP] ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)

[Catalin Marinas]

On Wed, Oct 16, 2019 at 03:44:25PM +0100, Dave P Martin wrote:
> On Wed, Oct 16, 2019 at 05:29:33AM +0100, Will Deacon wrote:
> > diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> > index b61b50bf68b1..c23c47360664 100644
> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -215,12 +215,18 @@ static inline unsigned long kaslr_offset(void)
> >   * up with a tagged userland pointer. Clear the tag to get a sane pointer to
> >   * pass on to access_ok(), for instance.
> >   */
> > -#define untagged_addr(addr)	\
> > +#define __untagged_addr(addr)	\
> >  	((__force __typeof__(addr))sign_extend64((__force u64)(addr), 55))
> >  
> > +#define untagged_addr(addr)	({					\
> 
> Having the same informal name ("untagged") for two different address
> representations seems like a recipe for confusion.  Can we rename one of
> them?  (Say, untagged_address_if_user()?)

I agree it's confusing. We can rename the __* one but the other is
spread around the kernel (it can be done, though as a subsequent
exercise; untagged_uaddr?).

> > +	__addr &= __untagged_addr(__addr);				\
> > +	(__force __typeof__(addr))__addr;				\
> > +})
> 
> Is there any reason why we can't just have
> 
> #define untagged_addr ((__force __typeof__(addr))(	\
> 	(__force u64)(addr) & GENMASK_ULL(63, 56)))

I guess you meant ~GENMASK_ULL(63,56) or GENMASK_ULL(55,0).

This changes the overflow case for mlock() which would return -ENOMEM
instead of -EINVAL (not sure we have a test for it though). Does it
matter?

-- 
Catalin