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

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

[Jan Stancek]

----- Original Message -----
> 
> Hello,
> 
> We ran automated tests on a recent commit from this kernel tree:
> 
>        Kernel repo:
>        git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux-stable.git
>             Commit: d6c2c23a29f4 - Merge branch 'stable-next' of
>             ssh://chubbybox:/home/sasha/data/next into stable-next
> 
> The results of these automated tests are provided below.
> 
>     Overall result: FAILED (see details below)
>              Merge: OK
>            Compile: OK
>              Tests: FAILED
> 
> All kernel binaries, config files, and logs are available for download here:
> 
>   https://artifacts.cki-project.org/pipelines/223563
> 
> One or more kernel tests failed:
> 
>     aarch64:
>       ? LTP: openposix test suite
> 

Test [1] is passing value close to LONG_MAX, which on arm64 is now treated as tagged userspace ptr:
  057d3389108e ("mm: untag user pointers passed to memory syscalls")

And now seems to hit overflow check after sign extension (EINVAL).
Test should probably find different way to choose invalid pointer.

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c

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

[Andrey Konovalov]

On Mon, Oct 14, 2019 at 9:29 AM Jan Stancek <jstancek@redhat.com> wrote:
>
>
>
> ----- Original Message -----
> >
> > Hello,
> >
> > We ran automated tests on a recent commit from this kernel tree:
> >
> >        Kernel repo:
> >        git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux-stable.git
> >             Commit: d6c2c23a29f4 - Merge branch 'stable-next' of
> >             ssh://chubbybox:/home/sasha/data/next into stable-next
> >
> > The results of these automated tests are provided below.
> >
> >     Overall result: FAILED (see details below)
> >              Merge: OK
> >            Compile: OK
> >              Tests: FAILED
> >
> > All kernel binaries, config files, and logs are available for download here:
> >
> >   https://artifacts.cki-project.org/pipelines/223563
> >
> > One or more kernel tests failed:
> >
> >     aarch64:
> >       ? LTP: openposix test suite
> >
>
> Test [1] is passing value close to LONG_MAX, which on arm64 is now treated as tagged userspace ptr:
>   057d3389108e ("mm: untag user pointers passed to memory syscalls")
>
> And now seems to hit overflow check after sign extension (EINVAL).
> Test should probably find different way to choose invalid pointer.
>
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c

+Catalin and Vincenzo

Per Documentation/arm64/tagged-address-abi.rst we now have:

User addresses not accessed by the kernel but used for address space
management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
of valid tagged pointers in this context is always allowed.

However this breaks the test above.

What do you think we should do here?

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

[Catalin Marinas]

+ Will

On Mon, Oct 14, 2019 at 02:54:17PM +0200, Andrey Konovalov wrote:
> On Mon, Oct 14, 2019 at 9:29 AM Jan Stancek <jstancek@redhat.com> wrote:
> > > We ran automated tests on a recent commit from this kernel tree:
> > >
> > >        Kernel repo:
> > >        git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux-stable.git
> > >             Commit: d6c2c23a29f4 - Merge branch 'stable-next' of
> > >             ssh://chubbybox:/home/sasha/data/next into stable-next
> > >
> > > The results of these automated tests are provided below.
> > >
> > >     Overall result: FAILED (see details below)
> > >              Merge: OK
> > >            Compile: OK
> > >              Tests: FAILED
> > >
> > > All kernel binaries, config files, and logs are available for download here:
> > >
> > >   https://artifacts.cki-project.org/pipelines/223563
> > >
> > > One or more kernel tests failed:
> > >
> > >     aarch64:
> > >       ? LTP: openposix test suite
> > >
> >
> > Test [1] is passing value close to LONG_MAX, which on arm64 is now treated as tagged userspace ptr:
> >   057d3389108e ("mm: untag user pointers passed to memory syscalls")
> >
> > And now seems to hit overflow check after sign extension (EINVAL).
> > Test should probably find different way to choose invalid pointer.
> >
> > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c
> 
> Per Documentation/arm64/tagged-address-abi.rst we now have:
> 
> User addresses not accessed by the kernel but used for address space
> management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
> of valid tagged pointers in this context is always allowed.
> 
> However this breaks the test above.

So the problem is that user space passes a 0x7fff_ffff_ffff_f000 start
address and untagged_addr sign-extends it to 0xffff_ffff_ffff_f000. The
subsequent check in apply_vma_lock_flags() finds that start+PAGE_SIZE is
smaller than start, hence -EINVAL instead of -ENOMEM.

> What do you think we should do here?

It is an ABI break as the man page clearly states that the above case
should return -ENOMEM. The options I see:

1. Revert commit 057d3389108e and try again to document that the memory
   syscalls do not support tagged pointers

2. Change untagged_addr() to only 0-extend from bit 55 or leave the
   tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
   than sign-extend) but if we had an mlock test passing ULONG_MASK,
   then we get -ENOMEM instead of -EINVAL

3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
   break the ABI for applications opting in to this new ABI. We did look
   at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
   whether we check the ptrace'd process or the debugger flags

4. Leave things as they are, consider the address space 56-bit and
   change the test to not use LONG_MAX on arm64. This needs to be passed
   by the sparc guys since they probably have a similar issue

It's slightly annoying to find this now. We did run (part of) LTP but I
guess we never run the POSIX conformance tests.

My preference is 2 with a quick attempt below. This basically means
clear the tag if it resembles a valid (tagged) user pointer, otherwise
don't touch it (bit 55 set always means an invalid user pointer). Not
sure how the generated code will look like but we could probably do
something better in assembly directly.

---------8<-------------------------------
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index b61b50bf68b1..6b36d080a633 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -215,12 +215,15 @@ 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)	\
+	((__force u64)(addr) & BIT(55) ? (addr) : __untagged_addr(addr))
+
 #ifdef CONFIG_KASAN_SW_TAGS
 #define __tag_shifted(tag)	((u64)(tag) << 56)
-#define __tag_reset(addr)	untagged_addr(addr)
+#define __tag_reset(addr)	__untagged_addr(addr)
 #define __tag_get(addr)		(__u8)((u64)(addr) >> 56)
 #else
 #define __tag_shifted(tag)	0UL

-- 
Catalin

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

[Will Deacon]

On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
> On Mon, Oct 14, 2019 at 02:54:17PM +0200, Andrey Konovalov wrote:
> > On Mon, Oct 14, 2019 at 9:29 AM Jan Stancek <jstancek@redhat.com> wrote:
> > > > We ran automated tests on a recent commit from this kernel tree:
> > > >
> > > >        Kernel repo:
> > > >        git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux-stable.git
> > > >             Commit: d6c2c23a29f4 - Merge branch 'stable-next' of
> > > >             ssh://chubbybox:/home/sasha/data/next into stable-next
> > > >
> > > > The results of these automated tests are provided below.
> > > >
> > > >     Overall result: FAILED (see details below)
> > > >              Merge: OK
> > > >            Compile: OK
> > > >              Tests: FAILED
> > > >
> > > > All kernel binaries, config files, and logs are available for download here:
> > > >
> > > >   https://artifacts.cki-project.org/pipelines/223563
> > > >
> > > > One or more kernel tests failed:
> > > >
> > > >     aarch64:
> > > >       ? LTP: openposix test suite
> > > >
> > >
> > > Test [1] is passing value close to LONG_MAX, which on arm64 is now treated as tagged userspace ptr:
> > >   057d3389108e ("mm: untag user pointers passed to memory syscalls")
> > >
> > > And now seems to hit overflow check after sign extension (EINVAL).
> > > Test should probably find different way to choose invalid pointer.
> > >
> > > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c
> > 
> > Per Documentation/arm64/tagged-address-abi.rst we now have:
> > 
> > User addresses not accessed by the kernel but used for address space
> > management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
> > of valid tagged pointers in this context is always allowed.
> > 
> > However this breaks the test above.
> 
> So the problem is that user space passes a 0x7fff_ffff_ffff_f000 start
> address and untagged_addr sign-extends it to 0xffff_ffff_ffff_f000. The
> subsequent check in apply_vma_lock_flags() finds that start+PAGE_SIZE is
> smaller than start, hence -EINVAL instead of -ENOMEM.
> 
> > What do you think we should do here?
> 
> It is an ABI break as the man page clearly states that the above case
> should return -ENOMEM.

Although I agree with your analysis, I also thought that these sorts of
ABI breaks (changes in error codes) were unfortunately common so we
shouldn't throw the baby out with the bath water here.

> The options I see:
> 
> 1. Revert commit 057d3389108e and try again to document that the memory
>    syscalls do not support tagged pointers
> 
> 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
>    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
>    than sign-extend) but if we had an mlock test passing ULONG_MASK,
>    then we get -ENOMEM instead of -EINVAL
> 
> 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
>    break the ABI for applications opting in to this new ABI. We did look
>    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
>    whether we check the ptrace'd process or the debugger flags
> 
> 4. Leave things as they are, consider the address space 56-bit and
>    change the test to not use LONG_MAX on arm64. This needs to be passed
>    by the sparc guys since they probably have a similar issue

I'm in favour of (2) or (4) as long as there's also an update to the docs.

> It's slightly annoying to find this now. We did run (part of) LTP but I
> guess we never run the POSIX conformance tests.

Yes, and this stuff was in linux-next for a while so it's worrying that
kernelci didn't spot it either. Hmm.

> My preference is 2 with a quick attempt below. This basically means
> clear the tag if it resembles a valid (tagged) user pointer, otherwise
> don't touch it (bit 55 set always means an invalid user pointer). Not
> sure how the generated code will look like but we could probably do
> something better in assembly directly.
> 
> ---------8<-------------------------------
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index b61b50bf68b1..6b36d080a633 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -215,12 +215,15 @@ 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)	\
> +	((__force u64)(addr) & BIT(55) ? (addr) : __untagged_addr(addr))

It would be nice not to resort to asm for this, but I think we can do a bit
better with something like the code below which just introduces an
additional AND instruction.

Will

--->8

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)	({					\
+	u64 __addr = (__force u64)addr;					\
+	__addr &= __untagged_addr(__addr);				\
+	(__force __typeof__(addr))__addr;				\
+})
+
 #ifdef CONFIG_KASAN_SW_TAGS
 #define __tag_shifted(tag)	((u64)(tag) << 56)
-#define __tag_reset(addr)	untagged_addr(addr)
+#define __tag_reset(addr)	__untagged_addr(addr)
 #define __tag_get(addr)		(__u8)((u64)(addr) >> 56)
 #else
 #define __tag_shifted(tag)	0UL

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

[Catalin Marinas]

Adding Szabolcs and Dave from ARM as we've discussed this internally
briefly but we should include the wider audience.

On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
> On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
> > On Mon, Oct 14, 2019 at 02:54:17PM +0200, Andrey Konovalov wrote:
> > > On Mon, Oct 14, 2019 at 9:29 AM Jan Stancek <jstancek@redhat.com> wrote:
> > > > > We ran automated tests on a recent commit from this kernel tree:
> > > > >
> > > > >        Kernel repo:
> > > > >        git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux-stable.git
> > > > >             Commit: d6c2c23a29f4 - Merge branch 'stable-next' of
> > > > >             ssh://chubbybox:/home/sasha/data/next into stable-next
> > > > >
> > > > > The results of these automated tests are provided below.
> > > > >
> > > > >     Overall result: FAILED (see details below)
> > > > >              Merge: OK
> > > > >            Compile: OK
> > > > >              Tests: FAILED
> > > > >
> > > > > All kernel binaries, config files, and logs are available for download here:
> > > > >
> > > > >   https://artifacts.cki-project.org/pipelines/223563
> > > > >
> > > > > One or more kernel tests failed:
> > > > >
> > > > >     aarch64:
> > > > >       ? LTP: openposix test suite
> > > > >
> > > >
> > > > Test [1] is passing value close to LONG_MAX, which on arm64 is now treated as tagged userspace ptr:
> > > >   057d3389108e ("mm: untag user pointers passed to memory syscalls")
> > > >
> > > > And now seems to hit overflow check after sign extension (EINVAL).
> > > > Test should probably find different way to choose invalid pointer.
> > > >
> > > > [1] https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c
> > > 
> > > Per Documentation/arm64/tagged-address-abi.rst we now have:
> > > 
> > > User addresses not accessed by the kernel but used for address space
> > > management (e.g. ``mmap()``, ``mprotect()``, ``madvise()``). The use
> > > of valid tagged pointers in this context is always allowed.
> > > 
> > > However this breaks the test above.
> > 
> > So the problem is that user space passes a 0x7fff_ffff_ffff_f000 start
> > address and untagged_addr sign-extends it to 0xffff_ffff_ffff_f000. The
> > subsequent check in apply_vma_lock_flags() finds that start+PAGE_SIZE is
> > smaller than start, hence -EINVAL instead of -ENOMEM.
> > 
> > > What do you think we should do here?
> > 
> > It is an ABI break as the man page clearly states that the above case
> > should return -ENOMEM.
> 
> Although I agree with your analysis, I also thought that these sorts of
> ABI breaks (changes in error codes) were unfortunately common so we
> shouldn't throw the baby out with the bath water here.
> 
> > The options I see:
> > 
> > 1. Revert commit 057d3389108e and try again to document that the memory
> >    syscalls do not support tagged pointers
> > 
> > 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
> >    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
> >    than sign-extend) but if we had an mlock test passing ULONG_MASK,
> >    then we get -ENOMEM instead of -EINVAL
> > 
> > 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
> >    break the ABI for applications opting in to this new ABI. We did look
> >    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
> >    whether we check the ptrace'd process or the debugger flags
> > 
> > 4. Leave things as they are, consider the address space 56-bit and
> >    change the test to not use LONG_MAX on arm64. This needs to be passed
> >    by the sparc guys since they probably have a similar issue
> 
> I'm in favour of (2) or (4) as long as there's also an update to the docs.

With (4) we'd start differing from other architectures supported by
Linux. This works if we consider the test to be broken. However, reading
the mlock man page:

       EINVAL The result of the addition addr+len was less than addr
       (e.g., the addition may have resulted in an overflow).

       ENOMEM Some of the specified address range does not correspond to
       mapped pages in the address space of the process.

There is no mention of EINVAL outside the TASK_SIZE, seems to fall more
within the ENOMEM description above.

> > It's slightly annoying to find this now. We did run (part of) LTP but I
> > guess we never run the POSIX conformance tests.
> 
> Yes, and this stuff was in linux-next for a while so it's worrying that
> kernelci didn't spot it either. Hmm.

For some reason the mlock test was skipped around the time we pushed the
TBI patches into -next:

https://qa-reports.linaro.org/lkft/linux-next-oe/tests/ltp-open-posix-tests/mlock_8-1?&page=2

Internally I don't think we've configured LTP with
--with-open-posix-testsuite, so this explains why we missed it.

> > My preference is 2 with a quick attempt below. This basically means
> > clear the tag if it resembles a valid (tagged) user pointer, otherwise
> > don't touch it (bit 55 set always means an invalid user pointer). Not
> > sure how the generated code will look like but we could probably do
> > something better in assembly directly.
[...]
> 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)	({					\
> +	u64 __addr = (__force u64)addr;					\
> +	__addr &= __untagged_addr(__addr);				\
> +	(__force __typeof__(addr))__addr;				\
> +})
> +
>  #ifdef CONFIG_KASAN_SW_TAGS
>  #define __tag_shifted(tag)	((u64)(tag) << 56)
> -#define __tag_reset(addr)	untagged_addr(addr)
> +#define __tag_reset(addr)	__untagged_addr(addr)
>  #define __tag_get(addr)		(__u8)((u64)(addr) >> 56)
>  #else
>  #define __tag_shifted(tag)	0UL

This works for me. Szabolcs also suggested just zeroing the top byte but
we wouldn't catch the overflow EINVAL case above, so I'd rather only
mask the tag out if it was a user address (i.e. bit 55 is 0).

-- 
Catalin

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

[Vincenzo Frascino]

Hi Catalin,

On 10/15/19 4:26 PM, Catalin Marinas wrote:
> Adding Szabolcs and Dave from ARM as we've discussed this internally
> briefly but we should include the wider audience.
> 
> On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
>> On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
>>> On Mon, Oct 14, 2019 at 02:54:17PM +0200, Andrey Konovalov wrote:
>>>> On Mon, Oct 14, 2019 at 9:29 AM Jan Stancek <jstancek@redhat.com> wrote:
>>>>>> We ran automated tests on a recent commit from this kernel tree:
>>>>>>
>>>>>>        Kernel repo:
>>>>>>        git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux-stable.git
>>>>>>             Commit: d6c2c23a29f4 - Merge branch 'stable-next' of
>>>>>>             ssh://chubbybox:/home/sasha/data/next into stable-next
>>>>>>
>>>>>> The results of these automated tests are provided below.
>>>>>>
>>>>>>     Overall result: FAILED (see details below)
>>>>>>              Merge: OK
>>>>>>            Compile: OK
>>>>>>              Tests: FAILED
>>>>>>
>>>>>> All kernel binaries, config files, and logs are available for download here:
>>>>>>
>>>>>>   https://artifacts.cki-project.org/pipelines/223563
>>>>>>
>>>>>> One or more kernel tests failed:
>>>>>>
>>>>>>     aarch64:
>>>>>>       ? LTP: openposix test suite
>>>>>>
>>>>>
>>>>> Test [1] is passing value close to LONG_MAX, which on arm64 is now treated as tagged userspace ptr:
>>>>>   057d3389108e ("mm: untag user pointers passed to memory syscalls")
>>>>>
>>>>> And now seems to hit overflow check after sign extension (EINVAL).
>>>>> Test should probably find different way to choose invalid pointer.
>>>>>
>>>>> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c
>>>>

[...]

>>> The options I see:
>>>
>>> 1. Revert commit 057d3389108e and try again to document that the memory
>>>    syscalls do not support tagged pointers
>>>
>>> 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
>>>    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
>>>    than sign-extend) but if we had an mlock test passing ULONG_MASK,
>>>    then we get -ENOMEM instead of -EINVAL
>>>
>>> 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
>>>    break the ABI for applications opting in to this new ABI. We did look
>>>    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
>>>    whether we check the ptrace'd process or the debugger flags
>>>
>>> 4. Leave things as they are, consider the address space 56-bit and
>>>    change the test to not use LONG_MAX on arm64. This needs to be passed
>>>    by the sparc guys since they probably have a similar issue
>>
>> I'm in favour of (2) or (4) as long as there's also an update to the docs.
> 
> With (4) we'd start differing from other architectures supported by
> Linux. This works if we consider the test to be broken. However, reading
> the mlock man page:
> 
>        EINVAL The result of the addition addr+len was less than addr
>        (e.g., the addition may have resulted in an overflow).
> 
>        ENOMEM Some of the specified address range does not correspond to
>        mapped pages in the address space of the process.
> 
> There is no mention of EINVAL outside the TASK_SIZE, seems to fall more
> within the ENOMEM description above.
>

I agree with your analysis and vote for option (2) as well, because of what you
reported about mlock() error meanings and because being this ABI I would prefer
where possible to not diverge from other architectures.
 [...]

-- 
Regards,
Vincenzo

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

[Will Deacon]

On Tue, Oct 15, 2019 at 04:26:51PM +0100, Catalin Marinas wrote:
> On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
> > On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
> > > On Mon, Oct 14, 2019 at 02:54:17PM +0200, Andrey Konovalov wrote:
> > > > What do you think we should do here?
> > > 
> > > It is an ABI break as the man page clearly states that the above case
> > > should return -ENOMEM.
> > 
> > Although I agree with your analysis, I also thought that these sorts of
> > ABI breaks (changes in error codes) were unfortunately common so we
> > shouldn't throw the baby out with the bath water here.
> > 
> > > The options I see:
> > > 
> > > 1. Revert commit 057d3389108e and try again to document that the memory
> > >    syscalls do not support tagged pointers
> > > 
> > > 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
> > >    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
> > >    than sign-extend) but if we had an mlock test passing ULONG_MASK,
> > >    then we get -ENOMEM instead of -EINVAL
> > > 
> > > 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
> > >    break the ABI for applications opting in to this new ABI. We did look
> > >    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
> > >    whether we check the ptrace'd process or the debugger flags
> > > 
> > > 4. Leave things as they are, consider the address space 56-bit and
> > >    change the test to not use LONG_MAX on arm64. This needs to be passed
> > >    by the sparc guys since they probably have a similar issue
> > 
> > I'm in favour of (2) or (4) as long as there's also an update to the docs.
> 
> With (4) we'd start differing from other architectures supported by
> Linux. This works if we consider the test to be broken. However, reading
> the mlock man page:
> 
>        EINVAL The result of the addition addr+len was less than addr
>        (e.g., the addition may have resulted in an overflow).
> 
>        ENOMEM Some of the specified address range does not correspond to
>        mapped pages in the address space of the process.
> 
> There is no mention of EINVAL outside the TASK_SIZE, seems to fall more
> within the ENOMEM description above.

Sorry, I was being too vague in my wording. What I was trying to say is I'm
ok with (2) or (4), but either way we need to update our ABI documentation
under Documentation/arm64/. I personally don't think userspace will care
about EINVAL vs ENOMEM because the kernel is already horribly unreliable
when it comes to keeping error codes stable, which is why I think we could
get away with (4). For example, Jan (who reported this issue) wrote this
change to LTP last year for one of the mmap tests:

https://github.com/linux-test-project/ltp/commit/e7bab61882847609be3132a2f0d94f7469ff5d3e

The fact that we have tagging at all already means that we differ from
many other architectures.

> > > It's slightly annoying to find this now. We did run (part of) LTP but I
> > > guess we never run the POSIX conformance tests.
> > 
> > Yes, and this stuff was in linux-next for a while so it's worrying that
> > kernelci didn't spot it either. Hmm.
> 
> For some reason the mlock test was skipped around the time we pushed the
> TBI patches into -next:
> 
> https://qa-reports.linaro.org/lkft/linux-next-oe/tests/ltp-open-posix-tests/mlock_8-1?&page=2

Coincidence?

> Internally I don't think we've configured LTP with
> --with-open-posix-testsuite, so this explains why we missed it.

Ok, hopefully you can fix that now.

> > > My preference is 2 with a quick attempt below. This basically means
> > > clear the tag if it resembles a valid (tagged) user pointer, otherwise
> > > don't touch it (bit 55 set always means an invalid user pointer). Not
> > > sure how the generated code will look like but we could probably do
> > > something better in assembly directly.
> [...]
> > 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)	({					\
> > +	u64 __addr = (__force u64)addr;					\
> > +	__addr &= __untagged_addr(__addr);				\
> > +	(__force __typeof__(addr))__addr;				\
> > +})
> > +
> >  #ifdef CONFIG_KASAN_SW_TAGS
> >  #define __tag_shifted(tag)	((u64)(tag) << 56)
> > -#define __tag_reset(addr)	untagged_addr(addr)
> > +#define __tag_reset(addr)	__untagged_addr(addr)
> >  #define __tag_get(addr)		(__u8)((u64)(addr) >> 56)
> >  #else
> >  #define __tag_shifted(tag)	0UL
> 
> This works for me. Szabolcs also suggested just zeroing the top byte but
> we wouldn't catch the overflow EINVAL case above, so I'd rather only
> mask the tag out if it was a user address (i.e. bit 55 is 0).

I'll spin it as a proper patch while we decide whether we want to do
anything.

Will

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

[Will Deacon]

On Tue, Oct 15, 2019 at 05:14:53PM +0100, Will Deacon wrote:
> On Tue, Oct 15, 2019 at 04:26:51PM +0100, Catalin Marinas wrote:
> > On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
> > > On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
> > > > The options I see:
> > > > 
> > > > 1. Revert commit 057d3389108e and try again to document that the memory
> > > >    syscalls do not support tagged pointers
> > > > 
> > > > 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
> > > >    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
> > > >    than sign-extend) but if we had an mlock test passing ULONG_MASK,
> > > >    then we get -ENOMEM instead of -EINVAL
> > > > 
> > > > 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
> > > >    break the ABI for applications opting in to this new ABI. We did look
> > > >    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
> > > >    whether we check the ptrace'd process or the debugger flags
> > > > 
> > > > 4. Leave things as they are, consider the address space 56-bit and
> > > >    change the test to not use LONG_MAX on arm64. This needs to be passed
> > > >    by the sparc guys since they probably have a similar issue
> > > 
> > > I'm in favour of (2) or (4) as long as there's also an update to the docs.
> > 
> > With (4) we'd start differing from other architectures supported by
> > Linux. This works if we consider the test to be broken. However, reading
> > the mlock man page:
> > 
> >        EINVAL The result of the addition addr+len was less than addr
> >        (e.g., the addition may have resulted in an overflow).
> > 
> >        ENOMEM Some of the specified address range does not correspond to
> >        mapped pages in the address space of the process.
> > 
> > There is no mention of EINVAL outside the TASK_SIZE, seems to fall more
> > within the ENOMEM description above.
> 
> Sorry, I was being too vague in my wording. What I was trying to say is I'm
> ok with (2) or (4), but either way we need to update our ABI documentation
> under Documentation/arm64/.

Having looked at making that change, I actually think the text is ok as-is
if we go with option (2). We only make guarantees about "valid tagged
pointer", which are defined to "reference an address in the user process
address space" and therefore must have bit 55 == 0.

Untested patch below.

Will

--->8

From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001
From: Will Deacon <will@kernel.org>
Date: Tue, 15 Oct 2019 21:04:18 -0700
Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via TTBR1

Sign-extending TTBR1 addresses when converting to an untagged address
breaks the documented POSIX semantics for mlock() in some obscure error
cases where we end up returning -EINVAL instead of -ENOMEM as a direct
result of rewriting the upper address bits.

Rework the untagged_addr() macro to preserve the upper address bits for
TTBR1 addresses and only clear the tag bits for user addresses. This
matches the behaviour of the 'clear_address_tag' assembly macro, so
rename that and align the implementations at the same time so that they
use the same instruction sequences for the tag manipulation.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Link: https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/
Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/asm-uaccess.h |  7 +++----
 arch/arm64/include/asm/memory.h      | 10 ++++++++--
 arch/arm64/kernel/entry.S            |  4 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
index f74909ba29bd..5bf963830b17 100644
--- a/arch/arm64/include/asm/asm-uaccess.h
+++ b/arch/arm64/include/asm/asm-uaccess.h
@@ -78,10 +78,9 @@ alternative_else_nop_endif
 /*
  * Remove the address tag from a virtual address, if present.
  */
-	.macro	clear_address_tag, dst, addr
-	tst	\addr, #(1 << 55)
-	bic	\dst, \addr, #(0xff << 56)
-	csel	\dst, \dst, \addr, eq
+	.macro	untagged_addr, dst, addr
+	sbfx	\dst, \addr, #0, #56
+	and	\dst, \dst, \addr
 	.endm
 
 #endif
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)	({					\
+	u64 __addr = (__force u64)addr;					\
+	__addr &= __untagged_addr(__addr);				\
+	(__force __typeof__(addr))__addr;				\
+})
+
 #ifdef CONFIG_KASAN_SW_TAGS
 #define __tag_shifted(tag)	((u64)(tag) << 56)
-#define __tag_reset(addr)	untagged_addr(addr)
+#define __tag_reset(addr)	__untagged_addr(addr)
 #define __tag_get(addr)		(__u8)((u64)(addr) >> 56)
 #else
 #define __tag_shifted(tag)	0UL
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index e304fe04b098..9ae336cc5833 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -604,7 +604,7 @@ el1_da:
 	 */
 	mrs	x3, far_el1
 	inherit_daif	pstate=x23, tmp=x2
-	clear_address_tag x0, x3
+	untagged_addr	x0, x3
 	mov	x2, sp				// struct pt_regs
 	bl	do_mem_abort
 
@@ -808,7 +808,7 @@ el0_da:
 	mrs	x26, far_el1
 	ct_user_exit_irqoff
 	enable_daif
-	clear_address_tag x0, x26
+	untagged_addr	x0, x26
 	mov	x1, x25
 	mov	x2, sp
 	bl	do_mem_abort
-- 
2.23.0.700.g56cf767bdb-goog

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

[Catalin Marinas]

On Wed, Oct 16, 2019 at 05:29:33AM +0100, Will Deacon wrote:
> From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Tue, 15 Oct 2019 21:04:18 -0700
> Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via TTBR1
> 
> Sign-extending TTBR1 addresses when converting to an untagged address
> breaks the documented POSIX semantics for mlock() in some obscure error
> cases where we end up returning -EINVAL instead of -ENOMEM as a direct
> result of rewriting the upper address bits.
> 
> Rework the untagged_addr() macro to preserve the upper address bits for
> TTBR1 addresses and only clear the tag bits for user addresses. This
> matches the behaviour of the 'clear_address_tag' assembly macro, so
> rename that and align the implementations at the same time so that they
> use the same instruction sequences for the tag manipulation.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Link: https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Will Deacon <will@kernel.org>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Tested-by: Catalin Marinas <catalin.marinas@arm.com>

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

[Vincenzo Frascino]

On 10/16/19 5:29 AM, Will Deacon wrote:
> From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Tue, 15 Oct 2019 21:04:18 -0700
> Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via TTBR1
> 
> Sign-extending TTBR1 addresses when converting to an untagged address
> breaks the documented POSIX semantics for mlock() in some obscure error
> cases where we end up returning -EINVAL instead of -ENOMEM as a direct
> result of rewriting the upper address bits.
> 
> Rework the untagged_addr() macro to preserve the upper address bits for
> TTBR1 addresses and only clear the tag bits for user addresses. This
> matches the behaviour of the 'clear_address_tag' assembly macro, so
> rename that and align the implementations at the same time so that they
> use the same instruction sequences for the tag manipulation.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Link: https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/asm-uaccess.h |  7 +++----
>  arch/arm64/include/asm/memory.h      | 10 ++++++++--
>  arch/arm64/kernel/entry.S            |  4 ++--
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 

Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

-- 
Regards,
Vincenzo

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

[Andrey Konovalov]

On Wed, Oct 16, 2019 at 6:29 AM Will Deacon <will@kernel.org> wrote:
>
> On Tue, Oct 15, 2019 at 05:14:53PM +0100, Will Deacon wrote:
> > On Tue, Oct 15, 2019 at 04:26:51PM +0100, Catalin Marinas wrote:
> > > On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
> > > > On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
> > > > > The options I see:
> > > > >
> > > > > 1. Revert commit 057d3389108e and try again to document that the memory
> > > > >    syscalls do not support tagged pointers
> > > > >
> > > > > 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
> > > > >    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
> > > > >    than sign-extend) but if we had an mlock test passing ULONG_MASK,
> > > > >    then we get -ENOMEM instead of -EINVAL
> > > > >
> > > > > 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
> > > > >    break the ABI for applications opting in to this new ABI. We did look
> > > > >    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
> > > > >    whether we check the ptrace'd process or the debugger flags
> > > > >
> > > > > 4. Leave things as they are, consider the address space 56-bit and
> > > > >    change the test to not use LONG_MAX on arm64. This needs to be passed
> > > > >    by the sparc guys since they probably have a similar issue
> > > >
> > > > I'm in favour of (2) or (4) as long as there's also an update to the docs.
> > >
> > > With (4) we'd start differing from other architectures supported by
> > > Linux. This works if we consider the test to be broken. However, reading
> > > the mlock man page:
> > >
> > >        EINVAL The result of the addition addr+len was less than addr
> > >        (e.g., the addition may have resulted in an overflow).
> > >
> > >        ENOMEM Some of the specified address range does not correspond to
> > >        mapped pages in the address space of the process.
> > >
> > > There is no mention of EINVAL outside the TASK_SIZE, seems to fall more
> > > within the ENOMEM description above.
> >
> > Sorry, I was being too vague in my wording. What I was trying to say is I'm
> > ok with (2) or (4), but either way we need to update our ABI documentation
> > under Documentation/arm64/.
>
> Having looked at making that change, I actually think the text is ok as-is
> if we go with option (2). We only make guarantees about "valid tagged
> pointer", which are defined to "reference an address in the user process
> address space" and therefore must have bit 55 == 0.
>
> Untested patch below.
>
> Will
>
> --->8
>
> From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Tue, 15 Oct 2019 21:04:18 -0700
> Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via TTBR1
>
> Sign-extending TTBR1 addresses when converting to an untagged address
> breaks the documented POSIX semantics for mlock() in some obscure error
> cases where we end up returning -EINVAL instead of -ENOMEM as a direct
> result of rewriting the upper address bits.
>
> Rework the untagged_addr() macro to preserve the upper address bits for
> TTBR1 addresses and only clear the tag bits for user addresses. This
> matches the behaviour of the 'clear_address_tag' assembly macro, so
> rename that and align the implementations at the same time so that they
> use the same instruction sequences for the tag manipulation.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Link: https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Will Deacon <will@kernel.org>

Reviewed-by: Andrey Konovalov <andreyknvl@google.com>

Thanks!

> ---
>  arch/arm64/include/asm/asm-uaccess.h |  7 +++----
>  arch/arm64/include/asm/memory.h      | 10 ++++++++--
>  arch/arm64/kernel/entry.S            |  4 ++--
>  3 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
> index f74909ba29bd..5bf963830b17 100644
> --- a/arch/arm64/include/asm/asm-uaccess.h
> +++ b/arch/arm64/include/asm/asm-uaccess.h
> @@ -78,10 +78,9 @@ alternative_else_nop_endif
>  /*
>   * Remove the address tag from a virtual address, if present.
>   */
> -       .macro  clear_address_tag, dst, addr
> -       tst     \addr, #(1 << 55)
> -       bic     \dst, \addr, #(0xff << 56)
> -       csel    \dst, \dst, \addr, eq
> +       .macro  untagged_addr, dst, addr
> +       sbfx    \dst, \addr, #0, #56
> +       and     \dst, \dst, \addr
>         .endm
>
>  #endif
> 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)    ({                                      \
> +       u64 __addr = (__force u64)addr;                                 \
> +       __addr &= __untagged_addr(__addr);                              \
> +       (__force __typeof__(addr))__addr;                               \
> +})
> +
>  #ifdef CONFIG_KASAN_SW_TAGS
>  #define __tag_shifted(tag)     ((u64)(tag) << 56)
> -#define __tag_reset(addr)      untagged_addr(addr)
> +#define __tag_reset(addr)      __untagged_addr(addr)
>  #define __tag_get(addr)                (__u8)((u64)(addr) >> 56)
>  #else
>  #define __tag_shifted(tag)     0UL
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index e304fe04b098..9ae336cc5833 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -604,7 +604,7 @@ el1_da:
>          */
>         mrs     x3, far_el1
>         inherit_daif    pstate=x23, tmp=x2
> -       clear_address_tag x0, x3
> +       untagged_addr   x0, x3
>         mov     x2, sp                          // struct pt_regs
>         bl      do_mem_abort
>
> @@ -808,7 +808,7 @@ el0_da:
>         mrs     x26, far_el1
>         ct_user_exit_irqoff
>         enable_daif
> -       clear_address_tag x0, x26
> +       untagged_addr   x0, x26
>         mov     x1, x25
>         mov     x2, sp
>         bl      do_mem_abort
> --
> 2.23.0.700.g56cf767bdb-goog
>

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

[Jan Stancek]

----- Original Message -----
> 
> From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Tue, 15 Oct 2019 21:04:18 -0700
> Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via
> TTBR1
> 
> Sign-extending TTBR1 addresses when converting to an untagged address
> breaks the documented POSIX semantics for mlock() in some obscure error
> cases where we end up returning -EINVAL instead of -ENOMEM as a direct
> result of rewriting the upper address bits.
> 
> Rework the untagged_addr() macro to preserve the upper address bits for
> TTBR1 addresses and only clear the tag bits for user addresses. This
> matches the behaviour of the 'clear_address_tag' assembly macro, so
> rename that and align the implementations at the same time so that they
> use the same instruction sequences for the tag manipulation.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Link:
> https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Will Deacon <will@kernel.org>

No regressions observed with LTP syscalls/sched/mm/commands and open_posix_testsuite.

Tested-by: Jan Stancek <jstancek@redhat.com>

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

[Dave Martin]

On Wed, Oct 16, 2019 at 05:29:33AM +0100, Will Deacon wrote:
> On Tue, Oct 15, 2019 at 05:14:53PM +0100, Will Deacon wrote:
> > On Tue, Oct 15, 2019 at 04:26:51PM +0100, Catalin Marinas wrote:
> > > On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
> > > > On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
> > > > > The options I see:
> > > > > 
> > > > > 1. Revert commit 057d3389108e and try again to document that the memory
> > > > >    syscalls do not support tagged pointers
> > > > > 
> > > > > 2. Change untagged_addr() to only 0-extend from bit 55 or leave the
> > > > >    tag unchanged if bit 55 is 1. We could mask out the tag (0 rather
> > > > >    than sign-extend) but if we had an mlock test passing ULONG_MASK,
> > > > >    then we get -ENOMEM instead of -EINVAL
> > > > > 
> > > > > 3. Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only
> > > > >    break the ABI for applications opting in to this new ABI. We did look
> > > > >    at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on
> > > > >    whether we check the ptrace'd process or the debugger flags
> > > > > 
> > > > > 4. Leave things as they are, consider the address space 56-bit and
> > > > >    change the test to not use LONG_MAX on arm64. This needs to be passed
> > > > >    by the sparc guys since they probably have a similar issue
> > > > 
> > > > I'm in favour of (2) or (4) as long as there's also an update to the docs.
> > > 
> > > With (4) we'd start differing from other architectures supported by
> > > Linux. This works if we consider the test to be broken. However, reading
> > > the mlock man page:
> > > 
> > >        EINVAL The result of the addition addr+len was less than addr
> > >        (e.g., the addition may have resulted in an overflow).
> > > 
> > >        ENOMEM Some of the specified address range does not correspond to
> > >        mapped pages in the address space of the process.
> > > 
> > > There is no mention of EINVAL outside the TASK_SIZE, seems to fall more
> > > within the ENOMEM description above.
> > 
> > Sorry, I was being too vague in my wording. What I was trying to say is I'm
> > ok with (2) or (4), but either way we need to update our ABI documentation
> > under Documentation/arm64/.
> 
> Having looked at making that change, I actually think the text is ok as-is
> if we go with option (2). We only make guarantees about "valid tagged
> pointer", which are defined to "reference an address in the user process
> address space" and therefore must have bit 55 == 0.
> 
> Untested patch below.
> 
> Will
> 
> --->8
> 
> From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001
> From: Will Deacon <will@kernel.org>
> Date: Tue, 15 Oct 2019 21:04:18 -0700
> Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via TTBR1
> 
> Sign-extending TTBR1 addresses when converting to an untagged address
> breaks the documented POSIX semantics for mlock() in some obscure error
> cases where we end up returning -EINVAL instead of -ENOMEM as a direct
> result of rewriting the upper address bits.
> 
> Rework the untagged_addr() macro to preserve the upper address bits for
> TTBR1 addresses and only clear the tag bits for user addresses. This
> matches the behaviour of the 'clear_address_tag' assembly macro, so
> rename that and align the implementations at the same time so that they
> use the same instruction sequences for the tag manipulation.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Link: https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/
> Reported-by: Jan Stancek <jstancek@redhat.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/asm-uaccess.h |  7 +++----
>  arch/arm64/include/asm/memory.h      | 10 ++++++++--
>  arch/arm64/kernel/entry.S            |  4 ++--
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
> index f74909ba29bd..5bf963830b17 100644
> --- a/arch/arm64/include/asm/asm-uaccess.h
> +++ b/arch/arm64/include/asm/asm-uaccess.h
> @@ -78,10 +78,9 @@ alternative_else_nop_endif
>  /*
>   * Remove the address tag from a virtual address, if present.
>   */
> -	.macro	clear_address_tag, dst, addr
> -	tst	\addr, #(1 << 55)
> -	bic	\dst, \addr, #(0xff << 56)
> -	csel	\dst, \dst, \addr, eq
> +	.macro	untagged_addr, dst, addr
> +	sbfx	\dst, \addr, #0, #56
> +	and	\dst, \dst, \addr
>  	.endm
>  
>  #endif
> 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()?)

> +	u64 __addr = (__force u64)addr;					\

Missing () round addr.

Also, nit: needlessly fragile macro?  (OK, callers are unlikely to pass
"__addr" for addr, but the __addr variable doesn't do a lot here other
than to avoid repeated evaluation of the argument -- I don't expect this
to matter given how this macro is used.)

> +	__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)))

?

Either way, "kernel" addresses (bit 55 set) become unusable garbage,
but we _want_ such addresses passed from userspace to be unusable.
Comparison against TASK_SIZE will still police them accurately.

Simply zero-extending would be a less obfuscated way of only ever
rounding the address down -- it's the rounding up that spuriously
triggers address wraparound and leads to the -EINVAL return.


(Tests for error codes are inherently fragile, and MTE requires
POSIX wording to be interpreted in a context not anticipated by the
authors -- so I'm still not totally convinced we need a band-aid for
this.  But anyway...)


[...]

Cheers
---Dave

[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

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

[Dave Martin]

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

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

[Szabolcs Nagy]

On 16/10/2019 17:35, Dave Martin wrote:
> 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.

well this was the point of the bit 55 check, to avoid both.
(with the assumption that getting EINVAL is not important if
overflow happens with len > 2^55 and 0 bit 55)

as far as i can tell the EINVAL for overflow is linux specific.

i think returning ENOMEM for invalid addr,len pairs should be
fine, i.e. zero extension is ok.

i think this is consistent with posix requirements, and arguably
even with current linux manual which documents EINVAL for overflow
in mlock, but also ENOMEM for unmapped pages in the range, so both
errors are ok on overflow.

so the bit 55 check only matters if something somewhere relies
on the error code in a significant way when calling syscalls with
nonsensical arguments.

> > 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
>