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

[Dave Martin <Dave.Martin@arm.com>]

On Wed, Oct 16, 2019 at 03:52:38PM +0100, Catalin Marinas wrote:
> 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?

It looks like SUSv7 has m{,un}local() and mprotect() aligned with one
another regarding ENOMEM versus EINVAL:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/mprotect.html
https://pubs.opengroup.org/onlinepubs/9699919799/functions/mlock.html

So whatever we do, it should probably have the same effect on both
calls.


There's another wrinkle that occurrs to me though.  Rounding "kernel"
addresses up can spuriously change ENOMEM to EINVAL -- but the fix for
userspace addresses (i.e., rounding down) can likewise spuriously change
EINVAL to ENOMEM.

Maybe this is OK -- the SUSv7 wording doesn't seem to call out address
wraparound as a special case, and therefore supposedly doesn't require
EINVAL to be returned for it.

The asymmetry is concerning though, and a bit ugly.

Cheers
---Dave