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
