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