* ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-14 2:19 CKI Project
2019-10-14 7:28 ` [LTP] " Jan Stancek
0 siblings, 1 reply; 33+ messages in thread
From: CKI Project @ 2019-10-14 2:19 UTC (permalink / raw)
To: Linux Stable maillist; +Cc: Memory Management, Jan Stancek
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
We hope that these logs can help you find the problem quickly. For the full
detail on our testing procedures, please scroll to the bottom of this message.
Please reply to this email if you have any questions about the tests that we
ran or if you have any suggestions on how to make future tests more effective.
,-. ,-.
( C ) ( K ) Continuous
`-',-.`-' Kernel
( I ) Integration
`-'
______________________________________________________________________________
Compile testing
---------------
We compiled the kernel for 3 architectures:
aarch64:
make options: -j30 INSTALL_MOD_STRIP=1 targz-pkg
ppc64le:
make options: -j30 INSTALL_MOD_STRIP=1 targz-pkg
x86_64:
make options: -j30 INSTALL_MOD_STRIP=1 targz-pkg
Hardware testing
----------------
We booted each kernel and ran the following tests:
aarch64:
Host 1:
✅ Boot test
✅ xfstests: ext4
✅ xfstests: xfs
✅ selinux-policy: serge-testsuite
✅ lvm thinp sanity
✅ storage: software RAID testing
🚧 ✅ Storage blktests
Host 2:
✅ Boot test
✅ Podman system integration test (as root)
✅ Podman system integration test (as user)
✅ Loopdev Sanity
✅ jvm test suite
✅ Memory function: memfd_create
✅ AMTU (Abstract Machine Test Utility)
❌ LTP: openposix test suite
✅ Ethernet drivers sanity
✅ Networking socket: fuzz
✅ Networking sctp-auth: sockopts test
✅ Networking: igmp conformance test
✅ Networking TCP: keepalive test
✅ Networking UDP: socket
✅ Networking tunnel: gre basic
✅ Networking tunnel: vxlan basic
✅ audit: audit testsuite test
✅ httpd: mod_ssl smoke sanity
✅ iotop: sanity
✅ tuned: tune-processes-through-perf
✅ Usex - version 1.9-29
✅ storage: SCSI VPD
🚧 ✅ LTP lite
🚧 ✅ CIFS Connectathon
🚧 ✅ POSIX pjd-fstest suites
🚧 ✅ Memory function: kaslr
🚧 ✅ Networking bridge: sanity
🚧 ✅ Networking MACsec: sanity
🚧 ✅ Networking route: pmtu
🚧 ✅ Networking tunnel: geneve basic test
🚧 ✅ L2TP basic test
🚧 ✅ Networking vnic: ipvlan/basic
🚧 ✅ ALSA PCM loopback test
🚧 ✅ ALSA Control (mixer) Userspace Element test
🚧 ✅ storage: dm/common
🚧 ✅ trace: ftrace/tracer
🚧 ✅ Networking route_func: local
🚧 ✅ Networking route_func: forward
🚧 ✅ Networking ipsec: basic netns transport
🚧 ✅ Networking ipsec: basic netns tunnel
ppc64le:
Host 1:
✅ Boot test
✅ xfstests: ext4
✅ xfstests: xfs
✅ selinux-policy: serge-testsuite
✅ lvm thinp sanity
✅ storage: software RAID testing
🚧 ✅ Storage blktests
Host 2:
✅ Boot test
✅ Podman system integration test (as root)
✅ Podman system integration test (as user)
✅ Loopdev Sanity
✅ jvm test suite
✅ Memory function: memfd_create
✅ AMTU (Abstract Machine Test Utility)
✅ LTP: openposix test suite
✅ Ethernet drivers sanity
✅ Networking socket: fuzz
✅ Networking sctp-auth: sockopts test
✅ Networking TCP: keepalive test
✅ Networking UDP: socket
✅ Networking tunnel: gre basic
✅ Networking tunnel: vxlan basic
✅ audit: audit testsuite test
✅ httpd: mod_ssl smoke sanity
✅ iotop: sanity
✅ tuned: tune-processes-through-perf
✅ Usex - version 1.9-29
🚧 ✅ LTP lite
🚧 ✅ CIFS Connectathon
🚧 ✅ POSIX pjd-fstest suites
🚧 ✅ Memory function: kaslr
🚧 ✅ Networking bridge: sanity
🚧 ✅ Networking MACsec: sanity
🚧 ✅ Networking route: pmtu
🚧 ✅ Networking tunnel: geneve basic test
🚧 ✅ L2TP basic test
🚧 ✅ Networking ipsec: basic netns tunnel
🚧 ✅ Networking vnic: ipvlan/basic
🚧 ✅ ALSA PCM loopback test
🚧 ✅ ALSA Control (mixer) Userspace Element test
🚧 ✅ storage: dm/common
🚧 ✅ trace: ftrace/tracer
🚧 ✅ Networking route_func: local
🚧 ✅ Networking route_func: forward
x86_64:
Host 1:
✅ Boot test
🚧 ✅ IPMI driver test
🚧 ✅ IPMItool loop stress test
Host 2:
✅ Boot test
✅ Podman system integration test (as root)
✅ Podman system integration test (as user)
✅ Loopdev Sanity
✅ jvm test suite
✅ Memory function: memfd_create
✅ AMTU (Abstract Machine Test Utility)
✅ LTP: openposix test suite
✅ Ethernet drivers sanity
✅ Networking socket: fuzz
✅ Networking sctp-auth: sockopts test
✅ Networking: igmp conformance test
✅ Networking TCP: keepalive test
✅ Networking UDP: socket
✅ Networking tunnel: gre basic
✅ Networking tunnel: vxlan basic
✅ audit: audit testsuite test
✅ httpd: mod_ssl smoke sanity
✅ iotop: sanity
✅ tuned: tune-processes-through-perf
✅ pciutils: sanity smoke test
✅ Usex - version 1.9-29
✅ storage: SCSI VPD
✅ stress: stress-ng
🚧 ✅ LTP lite
🚧 ✅ CIFS Connectathon
🚧 ✅ POSIX pjd-fstest suites
🚧 ✅ Memory function: kaslr
🚧 ✅ Networking bridge: sanity
🚧 ✅ Networking MACsec: sanity
🚧 ✅ Networking route: pmtu
🚧 ✅ Networking tunnel: geneve basic test
🚧 ✅ L2TP basic test
🚧 ✅ Networking vnic: ipvlan/basic
🚧 ✅ ALSA PCM loopback test
🚧 ✅ ALSA Control (mixer) Userspace Element test
🚧 ✅ storage: dm/common
🚧 ✅ trace: ftrace/tracer
🚧 ✅ Networking route_func: local
🚧 ✅ Networking route_func: forward
🚧 ✅ Networking ipsec: basic netns transport
🚧 ✅ Networking ipsec: basic netns tunnel
Host 3:
✅ Boot test
✅ Storage SAN device stress - megaraid_sas
Host 4:
✅ Boot test
✅ Storage SAN device stress - mpt3sas driver
Host 5:
✅ Boot test
✅ xfstests: ext4
✅ xfstests: xfs
✅ selinux-policy: serge-testsuite
✅ lvm thinp sanity
✅ storage: software RAID testing
🚧 ✅ IOMMU boot test
🚧 ✅ Storage blktests
Test sources: https://github.com/CKI-project/tests-beaker
💚 Pull requests are welcome for new tests or improvements to existing tests!
Waived tests
------------
If the test run included waived tests, they are marked with 🚧. Such tests are
executed but their results are not taken into account. Tests are waived when
their results are not reliable enough, e.g. when they're just introduced or are
being fixed.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-14 2:19 ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next) CKI Project
@ 2019-10-14 7:28 ` Jan Stancek
0 siblings, 0 replies; 33+ messages in thread
From: Jan Stancek @ 2019-10-14 7:28 UTC (permalink / raw)
To: CKI Project, andreyknvl, LTP List
Cc: Linux Stable maillist, Memory Management
----- 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-14 7:28 ` Jan Stancek
0 siblings, 0 replies; 33+ messages in thread
From: Jan Stancek @ 2019-10-14 7:28 UTC (permalink / raw)
To: ltp
----- 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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-14 7:28 ` [LTP] " Jan Stancek
@ 2019-10-14 12:54 ` Andrey Konovalov
-1 siblings, 0 replies; 33+ messages in thread
From: Andrey Konovalov @ 2019-10-14 12:54 UTC (permalink / raw)
To: Jan Stancek, Catalin Marinas, Vincenzo Frascino
Cc: CKI Project, LTP List, Linux Stable maillist, Memory Management
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?
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-14 12:54 ` Andrey Konovalov
0 siblings, 0 replies; 33+ messages in thread
From: Andrey Konovalov @ 2019-10-14 12:54 UTC (permalink / raw)
To: ltp
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?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-14 12:54 ` [LTP] " Andrey Konovalov
@ 2019-10-14 16:26 ` Catalin Marinas
-1 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-14 16:26 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Jan Stancek, Vincenzo Frascino, CKI Project, LTP List,
Linux Stable maillist, Memory Management, Will Deacon
+ 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-14 16:26 ` Catalin Marinas
0 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-14 16:26 UTC (permalink / raw)
To: ltp
+ 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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-14 16:26 ` [LTP] " Catalin Marinas
@ 2019-10-14 21:33 ` Will Deacon
-1 siblings, 0 replies; 33+ messages in thread
From: Will Deacon @ 2019-10-14 21:33 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrey Konovalov, Jan Stancek, Vincenzo Frascino, CKI Project,
LTP List, Linux Stable maillist, Memory Management
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-14 21:33 ` Will Deacon
0 siblings, 0 replies; 33+ messages in thread
From: Will Deacon @ 2019-10-14 21:33 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-14 21:33 ` [LTP] " Will Deacon
@ 2019-10-15 15:26 ` Catalin Marinas
-1 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-15 15:26 UTC (permalink / raw)
To: Will Deacon
Cc: Andrey Konovalov, Jan Stancek, Vincenzo Frascino, CKI Project,
LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy, Dave P Martin
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-15 15:26 ` Catalin Marinas
0 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-15 15:26 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-15 15:26 ` [LTP] " Catalin Marinas
@ 2019-10-15 16:02 ` Vincenzo Frascino
-1 siblings, 0 replies; 33+ messages in thread
From: Vincenzo Frascino @ 2019-10-15 16:02 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon
Cc: Andrey Konovalov, Jan Stancek, CKI Project, LTP List,
Linux Stable maillist, Memory Management, Szabolcs Nagy,
Dave P Martin
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-15 16:02 ` Vincenzo Frascino
0 siblings, 0 replies; 33+ messages in thread
From: Vincenzo Frascino @ 2019-10-15 16:02 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-15 15:26 ` [LTP] " Catalin Marinas
@ 2019-10-15 16:14 ` Will Deacon
-1 siblings, 0 replies; 33+ messages in thread
From: Will Deacon @ 2019-10-15 16:14 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrey Konovalov, Jan Stancek, Vincenzo Frascino, CKI Project,
LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy, Dave P Martin
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-15 16:14 ` Will Deacon
0 siblings, 0 replies; 33+ messages in thread
From: Will Deacon @ 2019-10-15 16:14 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-15 16:14 ` [LTP] " Will Deacon
@ 2019-10-16 4:29 ` Will Deacon
-1 siblings, 0 replies; 33+ messages in thread
From: Will Deacon @ 2019-10-16 4:29 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrey Konovalov, Jan Stancek, Vincenzo Frascino, CKI Project,
LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy, Dave P Martin
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 4:29 ` Will Deacon
0 siblings, 0 replies; 33+ messages in thread
From: Will Deacon @ 2019-10-16 4:29 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 4:29 ` [LTP] " Will Deacon
@ 2019-10-16 8:12 ` Catalin Marinas
-1 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-16 8:12 UTC (permalink / raw)
To: Will Deacon
Cc: Andrey Konovalov, Jan Stancek, Vincenzo Frascino, CKI Project,
LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy, Dave P Martin
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 8:12 ` Catalin Marinas
0 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-16 8:12 UTC (permalink / raw)
To: ltp
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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 4:29 ` [LTP] " Will Deacon
@ 2019-10-16 8:18 ` Vincenzo Frascino
-1 siblings, 0 replies; 33+ messages in thread
From: Vincenzo Frascino @ 2019-10-16 8:18 UTC (permalink / raw)
To: Will Deacon, Catalin Marinas
Cc: Andrey Konovalov, Jan Stancek, CKI Project, LTP List,
Linux Stable maillist, Memory Management, Szabolcs Nagy,
Dave P Martin
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 8:18 ` Vincenzo Frascino
0 siblings, 0 replies; 33+ messages in thread
From: Vincenzo Frascino @ 2019-10-16 8:18 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 4:29 ` [LTP] " Will Deacon
@ 2019-10-16 13:55 ` Andrey Konovalov
-1 siblings, 0 replies; 33+ messages in thread
From: Andrey Konovalov @ 2019-10-16 13:55 UTC (permalink / raw)
To: Will Deacon
Cc: Catalin Marinas, Jan Stancek, Vincenzo Frascino, CKI Project,
LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy, Dave P Martin
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 13:55 ` Andrey Konovalov
0 siblings, 0 replies; 33+ messages in thread
From: Andrey Konovalov @ 2019-10-16 13:55 UTC (permalink / raw)
To: ltp
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 4:29 ` [LTP] " Will Deacon
@ 2019-10-16 14:38 ` Jan Stancek
-1 siblings, 0 replies; 33+ messages in thread
From: Jan Stancek @ 2019-10-16 14:38 UTC (permalink / raw)
To: Will Deacon
Cc: Catalin Marinas, Andrey Konovalov, Vincenzo Frascino,
CKI Project, LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy, Dave P Martin
----- 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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 14:38 ` Jan Stancek
0 siblings, 0 replies; 33+ messages in thread
From: Jan Stancek @ 2019-10-16 14:38 UTC (permalink / raw)
To: ltp
----- 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>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 4:29 ` [LTP] " Will Deacon
@ 2019-10-16 14:44 ` Dave Martin
-1 siblings, 0 replies; 33+ messages in thread
From: Dave Martin @ 2019-10-16 14:44 UTC (permalink / raw)
To: Will Deacon
Cc: Catalin Marinas, Andrey Konovalov, Jan Stancek,
Vincenzo Frascino, CKI Project, LTP List, Linux Stable maillist,
Memory Management, Szabolcs Nagy
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 14:44 ` Dave Martin
0 siblings, 0 replies; 33+ messages in thread
From: Dave Martin @ 2019-10-16 14:44 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 14:44 ` [LTP] " Dave Martin
@ 2019-10-16 14:52 ` Catalin Marinas
-1 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-16 14:52 UTC (permalink / raw)
To: Dave Martin
Cc: Will Deacon, Andrey Konovalov, Jan Stancek, Vincenzo Frascino,
CKI Project, LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 14:52 ` Catalin Marinas
0 siblings, 0 replies; 33+ messages in thread
From: Catalin Marinas @ 2019-10-16 14:52 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 14:52 ` [LTP] " Catalin Marinas
@ 2019-10-16 16:35 ` Dave Martin
-1 siblings, 0 replies; 33+ messages in thread
From: Dave Martin @ 2019-10-16 16:35 UTC (permalink / raw)
To: Catalin Marinas
Cc: Will Deacon, Andrey Konovalov, Jan Stancek, Vincenzo Frascino,
CKI Project, LTP List, Linux Stable maillist, Memory Management,
Szabolcs Nagy
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 16:35 ` Dave Martin
0 siblings, 0 replies; 33+ messages in thread
From: Dave Martin @ 2019-10-16 16:35 UTC (permalink / raw)
To: ltp
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
2019-10-16 16:35 ` [LTP] " Dave Martin
@ 2019-10-16 18:10 ` Szabolcs Nagy
-1 siblings, 0 replies; 33+ messages in thread
From: Szabolcs Nagy @ 2019-10-16 18:10 UTC (permalink / raw)
To: Dave P Martin, Catalin Marinas
Cc: nd, Will Deacon, Andrey Konovalov, Jan Stancek,
Vincenzo Frascino, CKI Project, LTP List, Linux Stable maillist,
Memory Management
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [LTP] ? FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next)
@ 2019-10-16 18:10 ` Szabolcs Nagy
0 siblings, 0 replies; 33+ messages in thread
From: Szabolcs Nagy @ 2019-10-16 18:10 UTC (permalink / raw)
To: ltp
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
>
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2019-10-16 18:10 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-14 2:19 ❌ FAIL: Test report for kernel 5.4.0-rc2-d6c2c23.cki (stable-next) CKI Project
2019-10-14 7:28 ` Jan Stancek
2019-10-14 7:28 ` [LTP] " Jan Stancek
2019-10-14 12:54 ` Andrey Konovalov
2019-10-14 12:54 ` [LTP] " Andrey Konovalov
2019-10-14 16:26 ` Catalin Marinas
2019-10-14 16:26 ` [LTP] " Catalin Marinas
2019-10-14 21:33 ` Will Deacon
2019-10-14 21:33 ` [LTP] " Will Deacon
2019-10-15 15:26 ` Catalin Marinas
2019-10-15 15:26 ` [LTP] " Catalin Marinas
2019-10-15 16:02 ` Vincenzo Frascino
2019-10-15 16:02 ` [LTP] " Vincenzo Frascino
2019-10-15 16:14 ` Will Deacon
2019-10-15 16:14 ` [LTP] " Will Deacon
2019-10-16 4:29 ` Will Deacon
2019-10-16 4:29 ` [LTP] " Will Deacon
2019-10-16 8:12 ` Catalin Marinas
2019-10-16 8:12 ` [LTP] " Catalin Marinas
2019-10-16 8:18 ` Vincenzo Frascino
2019-10-16 8:18 ` [LTP] " Vincenzo Frascino
2019-10-16 13:55 ` Andrey Konovalov
2019-10-16 13:55 ` [LTP] " Andrey Konovalov
2019-10-16 14:38 ` Jan Stancek
2019-10-16 14:38 ` [LTP] " Jan Stancek
2019-10-16 14:44 ` ? " Dave Martin
2019-10-16 14:44 ` [LTP] " Dave Martin
2019-10-16 14:52 ` Catalin Marinas
2019-10-16 14:52 ` [LTP] " Catalin Marinas
2019-10-16 16:35 ` Dave Martin
2019-10-16 16:35 ` [LTP] " Dave Martin
2019-10-16 18:10 ` Szabolcs Nagy
2019-10-16 18:10 ` [LTP] " Szabolcs Nagy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.