[v18,15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel
diff mbox series

Message ID 0999c80cd639b78ae27c0674069d552833227564.1561386715.git.andreyknvl@google.com
State New
Headers show
Series
  • arm64: untag user pointers passed to the kernel
Related show

Commit Message

Andrey Konovalov June 24, 2019, 2:33 p.m. UTC
This patch is a part of a series that extends kernel ABI to allow to pass
tagged user pointers (with the top byte set to something else other than
0x00) as syscall arguments.

This patch adds a simple test, that calls the uname syscall with a
tagged user pointer as an argument. Without the kernel accepting tagged
user pointers the test fails with EFAULT.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 tools/testing/selftests/arm64/.gitignore      |  1 +
 tools/testing/selftests/arm64/Makefile        | 11 +++++++
 .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
 tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
 4 files changed, 53 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/.gitignore
 create mode 100644 tools/testing/selftests/arm64/Makefile
 create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
 create mode 100644 tools/testing/selftests/arm64/tags_test.c

Comments

Kees Cook June 24, 2019, 3:02 p.m. UTC | #1
On Mon, Jun 24, 2019 at 04:33:00PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends kernel ABI to allow to pass
> tagged user pointers (with the top byte set to something else other than
> 0x00) as syscall arguments.
> 
> This patch adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  tools/testing/selftests/arm64/.gitignore      |  1 +
>  tools/testing/selftests/arm64/Makefile        | 11 +++++++
>  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
>  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
>  4 files changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/.gitignore
>  create mode 100644 tools/testing/selftests/arm64/Makefile
>  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
>  create mode 100644 tools/testing/selftests/arm64/tags_test.c
> 
> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
> new file mode 100644
> index 000000000000..e8fae8d61ed6
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/.gitignore
> @@ -0,0 +1 @@
> +tags_test
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> new file mode 100644
> index 000000000000..a61b2e743e99
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# ARCH can be overridden by the user for cross compiling
> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> +
> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> +TEST_GEN_PROGS := tags_test
> +TEST_PROGS := run_tags_test.sh
> +endif
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
> new file mode 100755
> index 000000000000..745f11379930
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +echo "--------------------"
> +echo "running tags test"
> +echo "--------------------"
> +./tags_test
> +if [ $? -ne 0 ]; then
> +	echo "[FAIL]"
> +else
> +	echo "[PASS]"
> +fi
> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
> new file mode 100644
> index 000000000000..22a1b266e373
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_test.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <sys/prctl.h>
> +#include <sys/utsname.h>
> +
> +#define SHIFT_TAG(tag)		((uint64_t)(tag) << 56)
> +#define SET_TAG(ptr, tag)	(((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> +					SHIFT_TAG(tag))
> +
> +int main(void)
> +{
> +	static int tbi_enabled = 0;
> +	struct utsname *ptr, *tagged_ptr;
> +	int err;
> +
> +	if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> +		tbi_enabled = 1;
> +	ptr = (struct utsname *)malloc(sizeof(*ptr));
> +	if (tbi_enabled)
> +		tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
> +	err = uname(tagged_ptr);
> +	free(ptr);
> +
> +	return err;
> +}
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
>
Catalin Marinas June 24, 2019, 5:38 p.m. UTC | #2
On Mon, Jun 24, 2019 at 04:33:00PM +0200, Andrey Konovalov wrote:
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_test.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <sys/prctl.h>
> +#include <sys/utsname.h>
> +
> +#define SHIFT_TAG(tag)		((uint64_t)(tag) << 56)
> +#define SET_TAG(ptr, tag)	(((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> +					SHIFT_TAG(tag))
> +
> +int main(void)
> +{
> +	static int tbi_enabled = 0;
> +	struct utsname *ptr, *tagged_ptr;
> +	int err;
> +
> +	if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> +		tbi_enabled = 1;

Nitpick: with the latest prctl() patch, you can skip the last three
arguments as they are ignored.

Either way:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Cristian Marussi Aug. 23, 2019, 1:56 p.m. UTC | #3
Hi Andrey

On 24/06/2019 15:33, Andrey Konovalov wrote:
> This patch is a part of a series that extends kernel ABI to allow to pass
> tagged user pointers (with the top byte set to something else other than
> 0x00) as syscall arguments.
> 
> This patch adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  tools/testing/selftests/arm64/.gitignore      |  1 +
>  tools/testing/selftests/arm64/Makefile        | 11 +++++++
>  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
>  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
>  4 files changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/.gitignore
>  create mode 100644 tools/testing/selftests/arm64/Makefile
>  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
>  create mode 100644 tools/testing/selftests/arm64/tags_test.c

After building a fresh Kernel from arm64/for-next-core from scratch at:

commit 239ab658bea3b387424501e7c416640d6752dc0c 
Merge: 6bfa3134bd3a 42d038c4fb00 1243cb6a676f d55c5f28afaf d06fa5a118f1 34b5560db40d
Author: Will Deacon <will@kernel.org>
Date:   Thu Aug 22 18:23:53 2019 +0100

    Merge branches 'for-next/error-injection', 'for-next/tbi', 'for-next/psci-cpuidle', 'for-next/cpu-topology' and 'for-next/52-bit-kva' into for-next/core


KSFT arm64 tests build is broken for me, both setting or not KBUILD_OUTPUT=

13:30 $ make TARGETS=arm64 kselftest-clean                       
make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
rm -f -r /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'

✔ ~/ARM/dev/src/pdsw/linux [arm64_for_next_core|…8⚑ 23]

13:30 $ make TARGETS=arm64 kselftest                  
make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
        ARCH=arm64 -C ../../.. headers_install             
  HOSTCC  scripts/basic/fixdep                     
  HOSTCC  scripts/unifdef                         
...
...
  HDRINST usr/include/asm/msgbuf.h
  HDRINST usr/include/asm/shmbuf.h
  INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/usr/include
/opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc     tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
tags_test.c: In function ‘main’:
tags_test.c:21:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
  if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
            ^~~~~~~~~~~~~~~~~~~~~~~
            PR_GET_TID_ADDRESS
tags_test.c:21:12: note: each undeclared identifier is reported only once for each function it appears in
tags_test.c:21:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
  if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
                                     ^~~~~~~~~~~~~~~~~~~~~
                                     PR_GET_DUMPABLE
../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test' failed
make[3]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test] Error 1
Makefile:136: recipe for target 'all' failed
make[2]: *** [all] Error 2
/home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1237: recipe for target 'kselftest' failed
make[1]: *** [kselftest] Error 2
make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
Makefile:179: recipe for target 'sub-make' failed
make: *** [sub-make] Error 2

Despite seeing KSFT installing Kernel Headers, they cannot be found.

Fixing this patch like this make it work for me:

diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile                                                                                        
index a61b2e743e99..f9f79fb272f0 100644                             
--- a/tools/testing/selftests/arm64/Makefile   
+++ b/tools/testing/selftests/arm64/Makefile                                                     
@@ -4,6 +4,7 @@                                                                       
 ARCH ?= $(shell uname -m 2>/dev/null || echo not)                                              
                                                       
 ifneq (,$(filter $(ARCH),aarch64 arm64))                                            
+CFLAGS += -I../../../../usr/include/                                                               
 TEST_GEN_PROGS := tags_test                                  
 TEST_PROGS := run_tags_test.sh                                    
 endif                                                             

but is not really a proper fix since it does NOT account for case in which you have
installed the Kernel Headers in a non standard location like when you use KBUILD_OUTPUT.

Am I missing something ?

Thanks

Cristian

> 
> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
> new file mode 100644
> index 000000000000..e8fae8d61ed6
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/.gitignore
> @@ -0,0 +1 @@
> +tags_test
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> new file mode 100644
> index 000000000000..a61b2e743e99
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# ARCH can be overridden by the user for cross compiling
> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> +
> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> +TEST_GEN_PROGS := tags_test
> +TEST_PROGS := run_tags_test.sh
> +endif
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
> new file mode 100755
> index 000000000000..745f11379930
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +echo "--------------------"
> +echo "running tags test"
> +echo "--------------------"
> +./tags_test
> +if [ $? -ne 0 ]; then
> +	echo "[FAIL]"
> +else
> +	echo "[PASS]"
> +fi
> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
> new file mode 100644
> index 000000000000..22a1b266e373
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_test.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <sys/prctl.h>
> +#include <sys/utsname.h>
> +
> +#define SHIFT_TAG(tag)		((uint64_t)(tag) << 56)
> +#define SET_TAG(ptr, tag)	(((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> +					SHIFT_TAG(tag))
> +
> +int main(void)
> +{
> +	static int tbi_enabled = 0;
> +	struct utsname *ptr, *tagged_ptr;
> +	int err;
> +
> +	if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> +		tbi_enabled = 1;
> +	ptr = (struct utsname *)malloc(sizeof(*ptr));
> +	if (tbi_enabled)
> +		tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
> +	err = uname(tagged_ptr);
> +	free(ptr);
> +
> +	return err;
> +}
>
Andrey Konovalov Aug. 23, 2019, 5:16 p.m. UTC | #4
On Fri, Aug 23, 2019 at 3:56 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Hi Andrey
>
> On 24/06/2019 15:33, Andrey Konovalov wrote:
> > This patch is a part of a series that extends kernel ABI to allow to pass
> > tagged user pointers (with the top byte set to something else other than
> > 0x00) as syscall arguments.
> >
> > This patch adds a simple test, that calls the uname syscall with a
> > tagged user pointer as an argument. Without the kernel accepting tagged
> > user pointers the test fails with EFAULT.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > ---
> >  tools/testing/selftests/arm64/.gitignore      |  1 +
> >  tools/testing/selftests/arm64/Makefile        | 11 +++++++
> >  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
> >  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
> >  4 files changed, 53 insertions(+)
> >  create mode 100644 tools/testing/selftests/arm64/.gitignore
> >  create mode 100644 tools/testing/selftests/arm64/Makefile
> >  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
> >  create mode 100644 tools/testing/selftests/arm64/tags_test.c
>
> After building a fresh Kernel from arm64/for-next-core from scratch at:
>
> commit 239ab658bea3b387424501e7c416640d6752dc0c
> Merge: 6bfa3134bd3a 42d038c4fb00 1243cb6a676f d55c5f28afaf d06fa5a118f1 34b5560db40d
> Author: Will Deacon <will@kernel.org>
> Date:   Thu Aug 22 18:23:53 2019 +0100
>
>     Merge branches 'for-next/error-injection', 'for-next/tbi', 'for-next/psci-cpuidle', 'for-next/cpu-topology' and 'for-next/52-bit-kva' into for-next/core
>
>
> KSFT arm64 tests build is broken for me, both setting or not KBUILD_OUTPUT=
>
> 13:30 $ make TARGETS=arm64 kselftest-clean
> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> rm -f -r /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>
> ✔ ~/ARM/dev/src/pdsw/linux [arm64_for_next_core|…8⚑ 23]
>
> 13:30 $ make TARGETS=arm64 kselftest
> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
>         ARCH=arm64 -C ../../.. headers_install
>   HOSTCC  scripts/basic/fixdep
>   HOSTCC  scripts/unifdef
> ...
> ...
>   HDRINST usr/include/asm/msgbuf.h
>   HDRINST usr/include/asm/shmbuf.h
>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/usr/include
> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc     tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
> tags_test.c: In function ‘main’:
> tags_test.c:21:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>             ^~~~~~~~~~~~~~~~~~~~~~~
>             PR_GET_TID_ADDRESS
> tags_test.c:21:12: note: each undeclared identifier is reported only once for each function it appears in
> tags_test.c:21:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>                                      ^~~~~~~~~~~~~~~~~~~~~
>                                      PR_GET_DUMPABLE
> ../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test' failed
> make[3]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test] Error 1
> Makefile:136: recipe for target 'all' failed
> make[2]: *** [all] Error 2
> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1237: recipe for target 'kselftest' failed
> make[1]: *** [kselftest] Error 2
> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> Makefile:179: recipe for target 'sub-make' failed
> make: *** [sub-make] Error 2
>
> Despite seeing KSFT installing Kernel Headers, they cannot be found.
>
> Fixing this patch like this make it work for me:
>
> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> index a61b2e743e99..f9f79fb272f0 100644
> --- a/tools/testing/selftests/arm64/Makefile
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -4,6 +4,7 @@
>  ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>
>  ifneq (,$(filter $(ARCH),aarch64 arm64))
> +CFLAGS += -I../../../../usr/include/
>  TEST_GEN_PROGS := tags_test
>  TEST_PROGS := run_tags_test.sh
>  endif
>
> but is not really a proper fix since it does NOT account for case in which you have
> installed the Kernel Headers in a non standard location like when you use KBUILD_OUTPUT.
>
> Am I missing something ?

Hm, PR_SET_TAGGED_ADDR_CTRL is defined in include/uapi/linux/prctl.h,
and the test has #include <sys/prctl.h> so as long as you've updated
your kernel headers this should work.

(I'm OOO next week, I'll see if I can reproduce this once I'm back).



>
> Thanks
>
> Cristian
>
> >
> > diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
> > new file mode 100644
> > index 000000000000..e8fae8d61ed6
> > --- /dev/null
> > +++ b/tools/testing/selftests/arm64/.gitignore
> > @@ -0,0 +1 @@
> > +tags_test
> > diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> > new file mode 100644
> > index 000000000000..a61b2e743e99
> > --- /dev/null
> > +++ b/tools/testing/selftests/arm64/Makefile
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# ARCH can be overridden by the user for cross compiling
> > +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> > +
> > +ifneq (,$(filter $(ARCH),aarch64 arm64))
> > +TEST_GEN_PROGS := tags_test
> > +TEST_PROGS := run_tags_test.sh
> > +endif
> > +
> > +include ../lib.mk
> > diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
> > new file mode 100755
> > index 000000000000..745f11379930
> > --- /dev/null
> > +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> > @@ -0,0 +1,12 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +echo "--------------------"
> > +echo "running tags test"
> > +echo "--------------------"
> > +./tags_test
> > +if [ $? -ne 0 ]; then
> > +     echo "[FAIL]"
> > +else
> > +     echo "[PASS]"
> > +fi
> > diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
> > new file mode 100644
> > index 000000000000..22a1b266e373
> > --- /dev/null
> > +++ b/tools/testing/selftests/arm64/tags_test.c
> > @@ -0,0 +1,29 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <stdint.h>
> > +#include <sys/prctl.h>
> > +#include <sys/utsname.h>
> > +
> > +#define SHIFT_TAG(tag)               ((uint64_t)(tag) << 56)
> > +#define SET_TAG(ptr, tag)    (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> > +                                     SHIFT_TAG(tag))
> > +
> > +int main(void)
> > +{
> > +     static int tbi_enabled = 0;
> > +     struct utsname *ptr, *tagged_ptr;
> > +     int err;
> > +
> > +     if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> > +             tbi_enabled = 1;
> > +     ptr = (struct utsname *)malloc(sizeof(*ptr));
> > +     if (tbi_enabled)
> > +             tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
> > +     err = uname(tagged_ptr);
> > +     free(ptr);
> > +
> > +     return err;
> > +}
> >
>
Cristian Marussi Aug. 23, 2019, 5:49 p.m. UTC | #5
Hi

On 23/08/2019 18:16, Andrey Konovalov wrote:
> On Fri, Aug 23, 2019 at 3:56 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
>>
>> Hi Andrey
>>
>> On 24/06/2019 15:33, Andrey Konovalov wrote:
>>> This patch is a part of a series that extends kernel ABI to allow to pass
>>> tagged user pointers (with the top byte set to something else other than
>>> 0x00) as syscall arguments.
>>>
>>> This patch adds a simple test, that calls the uname syscall with a
>>> tagged user pointer as an argument. Without the kernel accepting tagged
>>> user pointers the test fails with EFAULT.
>>>
>>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>>> ---
>>>  tools/testing/selftests/arm64/.gitignore      |  1 +
>>>  tools/testing/selftests/arm64/Makefile        | 11 +++++++
>>>  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
>>>  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
>>>  4 files changed, 53 insertions(+)
>>>  create mode 100644 tools/testing/selftests/arm64/.gitignore
>>>  create mode 100644 tools/testing/selftests/arm64/Makefile
>>>  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
>>>  create mode 100644 tools/testing/selftests/arm64/tags_test.c
>>
>> After building a fresh Kernel from arm64/for-next-core from scratch at:
>>
>> commit 239ab658bea3b387424501e7c416640d6752dc0c
>> Merge: 6bfa3134bd3a 42d038c4fb00 1243cb6a676f d55c5f28afaf d06fa5a118f1 34b5560db40d
>> Author: Will Deacon <will@kernel.org>
>> Date:   Thu Aug 22 18:23:53 2019 +0100
>>
>>     Merge branches 'for-next/error-injection', 'for-next/tbi', 'for-next/psci-cpuidle', 'for-next/cpu-topology' and 'for-next/52-bit-kva' into for-next/core
>>
>>
>> KSFT arm64 tests build is broken for me, both setting or not KBUILD_OUTPUT=
>>
>> 13:30 $ make TARGETS=arm64 kselftest-clean
>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>> rm -f -r /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>>
>> ✔ ~/ARM/dev/src/pdsw/linux [arm64_for_next_core|…8⚑ 23]
>>
>> 13:30 $ make TARGETS=arm64 kselftest
>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
>> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
>>         ARCH=arm64 -C ../../.. headers_install
>>   HOSTCC  scripts/basic/fixdep
>>   HOSTCC  scripts/unifdef
>> ...
>> ...
>>   HDRINST usr/include/asm/msgbuf.h
>>   HDRINST usr/include/asm/shmbuf.h
>>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/usr/include
>> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc     tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
>> tags_test.c: In function ‘main’:
>> tags_test.c:21:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>             ^~~~~~~~~~~~~~~~~~~~~~~
>>             PR_GET_TID_ADDRESS
>> tags_test.c:21:12: note: each undeclared identifier is reported only once for each function it appears in
>> tags_test.c:21:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>                                      ^~~~~~~~~~~~~~~~~~~~~
>>                                      PR_GET_DUMPABLE
>> ../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test' failed
>> make[3]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test] Error 1
>> Makefile:136: recipe for target 'all' failed
>> make[2]: *** [all] Error 2
>> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1237: recipe for target 'kselftest' failed
>> make[1]: *** [kselftest] Error 2
>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>> Makefile:179: recipe for target 'sub-make' failed
>> make: *** [sub-make] Error 2
>>
>> Despite seeing KSFT installing Kernel Headers, they cannot be found.
>>
>> Fixing this patch like this make it work for me:
>>
>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>> index a61b2e743e99..f9f79fb272f0 100644
>> --- a/tools/testing/selftests/arm64/Makefile
>> +++ b/tools/testing/selftests/arm64/Makefile
>> @@ -4,6 +4,7 @@
>>  ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>
>>  ifneq (,$(filter $(ARCH),aarch64 arm64))
>> +CFLAGS += -I../../../../usr/include/
>>  TEST_GEN_PROGS := tags_test
>>  TEST_PROGS := run_tags_test.sh
>>  endif
>>
>> but is not really a proper fix since it does NOT account for case in which you have
>> installed the Kernel Headers in a non standard location like when you use KBUILD_OUTPUT.
>>
>> Am I missing something ?
> 
> Hm, PR_SET_TAGGED_ADDR_CTRL is defined in include/uapi/linux/prctl.h,
> and the test has #include <sys/prctl.h> so as long as you've updated
> your kernel headers this should work.
> 
> (I'm OOO next week, I'll see if I can reproduce this once I'm back).

Ok. Thanks for the reply.

I think I've got it in my local tree having cloned arm64/for-next-core:

18:32 $ egrep -A 10 PR_SET_TAG ./include/uapi/linux/prctl.h
#define PR_SET_TAGGED_ADDR_CTRL         55
#define PR_GET_TAGGED_ADDR_CTRL         56
# define PR_TAGGED_ADDR_ENABLE          (1UL << 0)

#endif /* _LINUX_PRCTL_H */

and Kernel header are locally installed in my kernel src dir (by KSFT indeed)

18:34 $ egrep -RA 10 PR_SET_TAG usr/include/
usr/include/linux/prctl.h:#define PR_SET_TAGGED_ADDR_CTRL               55
usr/include/linux/prctl.h-#define PR_GET_TAGGED_ADDR_CTRL               56
usr/include/linux/prctl.h-# define PR_TAGGED_ADDR_ENABLE                (1UL << 0)
usr/include/linux/prctl.h-
usr/include/linux/prctl.h-#endif /* _LINUX_PRCTL_H */

but how are they supposed to be found if nor the test Makefile
neither the KSFT Makefile who installs them pass any -I options to the
compiler ?
I suppose <sys/prctl.h> tries to include arch specific headers from the regular system path,
but when you are cross-compiling ?

18:34 $ make TARGETS=arm64 kselftest
make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
        ARCH=arm64 -C ../../.. headers_install
  INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/usr/include
/opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc -Wall -O2 -g    tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test
tags_test.c: In function ‘main’:
tags_test.c:20:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
  if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
            ^~~~~~~~~~~~~~~~~~~~~~~
            PR_GET_TID_ADDRESS
tags_test.c:20:12: note: each undeclared identifier is reported only once for each function it appears in
tags_test.c:20:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
  if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
                                     ^~~~~~~~~~~~~~~~~~~~~
                                     PR_GET_DUMPABLE
../../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test' failed
make[4]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test] Error 1
Makefile:19: recipe for target 'all' failed
make[3]: *** [all] Error 2
Makefile:137: recipe for target 'all' failed
make[2]: *** [all] Error 2
/home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1236: recipe for target 'kselftest' failed
make[1]: *** [kselftest] Error 2
make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
Makefile:179: recipe for target 'sub-make' failed
make: *** [sub-make] Error 2


In fact many KSFT testcases seems to brutally add default headers path:

tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/uapi/
tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/
tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../usr/include/
tools/testing/selftests/net/Makefile:CFLAGS += -I../../../../usr/include/
tools/testing/selftests/membarrier/Makefile:CFLAGS += -g -I../../../../usr/include/ 
...

Cheers

Cristian
> 
> 
> 
>>
>> Thanks
>>
>> Cristian
>>
>>>
>>> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
>>> new file mode 100644
>>> index 000000000000..e8fae8d61ed6
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/arm64/.gitignore
>>> @@ -0,0 +1 @@
>>> +tags_test
>>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>>> new file mode 100644
>>> index 000000000000..a61b2e743e99
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/arm64/Makefile
>>> @@ -0,0 +1,11 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +# ARCH can be overridden by the user for cross compiling
>>> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>> +
>>> +ifneq (,$(filter $(ARCH),aarch64 arm64))
>>> +TEST_GEN_PROGS := tags_test
>>> +TEST_PROGS := run_tags_test.sh
>>> +endif
>>> +
>>> +include ../lib.mk
>>> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
>>> new file mode 100755
>>> index 000000000000..745f11379930
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
>>> @@ -0,0 +1,12 @@
>>> +#!/bin/sh
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +echo "--------------------"
>>> +echo "running tags test"
>>> +echo "--------------------"
>>> +./tags_test
>>> +if [ $? -ne 0 ]; then
>>> +     echo "[FAIL]"
>>> +else
>>> +     echo "[PASS]"
>>> +fi
>>> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
>>> new file mode 100644
>>> index 000000000000..22a1b266e373
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/arm64/tags_test.c
>>> @@ -0,0 +1,29 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +#include <stdio.h>
>>> +#include <stdlib.h>
>>> +#include <unistd.h>
>>> +#include <stdint.h>
>>> +#include <sys/prctl.h>
>>> +#include <sys/utsname.h>
>>> +
>>> +#define SHIFT_TAG(tag)               ((uint64_t)(tag) << 56)
>>> +#define SET_TAG(ptr, tag)    (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
>>> +                                     SHIFT_TAG(tag))
>>> +
>>> +int main(void)
>>> +{
>>> +     static int tbi_enabled = 0;
>>> +     struct utsname *ptr, *tagged_ptr;
>>> +     int err;
>>> +
>>> +     if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>> +             tbi_enabled = 1;
>>> +     ptr = (struct utsname *)malloc(sizeof(*ptr));
>>> +     if (tbi_enabled)
>>> +             tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
>>> +     err = uname(tagged_ptr);
>>> +     free(ptr);
>>> +
>>> +     return err;
>>> +}
>>>
>>
Andrey Konovalov Sept. 4, 2019, 2:52 p.m. UTC | #6
On Fri, Aug 23, 2019 at 7:49 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
>
> Hi
>
> On 23/08/2019 18:16, Andrey Konovalov wrote:
> > On Fri, Aug 23, 2019 at 3:56 PM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> >>
> >> Hi Andrey
> >>
> >> On 24/06/2019 15:33, Andrey Konovalov wrote:
> >>> This patch is a part of a series that extends kernel ABI to allow to pass
> >>> tagged user pointers (with the top byte set to something else other than
> >>> 0x00) as syscall arguments.
> >>>
> >>> This patch adds a simple test, that calls the uname syscall with a
> >>> tagged user pointer as an argument. Without the kernel accepting tagged
> >>> user pointers the test fails with EFAULT.
> >>>
> >>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >>> ---
> >>>  tools/testing/selftests/arm64/.gitignore      |  1 +
> >>>  tools/testing/selftests/arm64/Makefile        | 11 +++++++
> >>>  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
> >>>  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
> >>>  4 files changed, 53 insertions(+)
> >>>  create mode 100644 tools/testing/selftests/arm64/.gitignore
> >>>  create mode 100644 tools/testing/selftests/arm64/Makefile
> >>>  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
> >>>  create mode 100644 tools/testing/selftests/arm64/tags_test.c
> >>
> >> After building a fresh Kernel from arm64/for-next-core from scratch at:
> >>
> >> commit 239ab658bea3b387424501e7c416640d6752dc0c
> >> Merge: 6bfa3134bd3a 42d038c4fb00 1243cb6a676f d55c5f28afaf d06fa5a118f1 34b5560db40d
> >> Author: Will Deacon <will@kernel.org>
> >> Date:   Thu Aug 22 18:23:53 2019 +0100
> >>
> >>     Merge branches 'for-next/error-injection', 'for-next/tbi', 'for-next/psci-cpuidle', 'for-next/cpu-topology' and 'for-next/52-bit-kva' into for-next/core
> >>
> >>
> >> KSFT arm64 tests build is broken for me, both setting or not KBUILD_OUTPUT=
> >>
> >> 13:30 $ make TARGETS=arm64 kselftest-clean
> >> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >> rm -f -r /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
> >> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >>
> >> ✔ ~/ARM/dev/src/pdsw/linux [arm64_for_next_core|…8⚑ 23]
> >>
> >> 13:30 $ make TARGETS=arm64 kselftest
> >> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
> >> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
> >>         ARCH=arm64 -C ../../.. headers_install
> >>   HOSTCC  scripts/basic/fixdep
> >>   HOSTCC  scripts/unifdef
> >> ...
> >> ...
> >>   HDRINST usr/include/asm/msgbuf.h
> >>   HDRINST usr/include/asm/shmbuf.h
> >>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/usr/include
> >> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc     tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
> >> tags_test.c: In function ‘main’:
> >> tags_test.c:21:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
> >>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>             ^~~~~~~~~~~~~~~~~~~~~~~
> >>             PR_GET_TID_ADDRESS
> >> tags_test.c:21:12: note: each undeclared identifier is reported only once for each function it appears in
> >> tags_test.c:21:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
> >>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>                                      ^~~~~~~~~~~~~~~~~~~~~
> >>                                      PR_GET_DUMPABLE
> >> ../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test' failed
> >> make[3]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test] Error 1
> >> Makefile:136: recipe for target 'all' failed
> >> make[2]: *** [all] Error 2
> >> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1237: recipe for target 'kselftest' failed
> >> make[1]: *** [kselftest] Error 2
> >> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >> Makefile:179: recipe for target 'sub-make' failed
> >> make: *** [sub-make] Error 2
> >>
> >> Despite seeing KSFT installing Kernel Headers, they cannot be found.
> >>
> >> Fixing this patch like this make it work for me:
> >>
> >> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> >> index a61b2e743e99..f9f79fb272f0 100644
> >> --- a/tools/testing/selftests/arm64/Makefile
> >> +++ b/tools/testing/selftests/arm64/Makefile
> >> @@ -4,6 +4,7 @@
> >>  ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> >>
> >>  ifneq (,$(filter $(ARCH),aarch64 arm64))
> >> +CFLAGS += -I../../../../usr/include/
> >>  TEST_GEN_PROGS := tags_test
> >>  TEST_PROGS := run_tags_test.sh
> >>  endif
> >>
> >> but is not really a proper fix since it does NOT account for case in which you have
> >> installed the Kernel Headers in a non standard location like when you use KBUILD_OUTPUT.
> >>
> >> Am I missing something ?
> >
> > Hm, PR_SET_TAGGED_ADDR_CTRL is defined in include/uapi/linux/prctl.h,
> > and the test has #include <sys/prctl.h> so as long as you've updated
> > your kernel headers this should work.
> >
> > (I'm OOO next week, I'll see if I can reproduce this once I'm back).
>
> Ok. Thanks for the reply.
>
> I think I've got it in my local tree having cloned arm64/for-next-core:
>
> 18:32 $ egrep -A 10 PR_SET_TAG ./include/uapi/linux/prctl.h
> #define PR_SET_TAGGED_ADDR_CTRL         55
> #define PR_GET_TAGGED_ADDR_CTRL         56
> # define PR_TAGGED_ADDR_ENABLE          (1UL << 0)
>
> #endif /* _LINUX_PRCTL_H */
>
> and Kernel header are locally installed in my kernel src dir (by KSFT indeed)
>
> 18:34 $ egrep -RA 10 PR_SET_TAG usr/include/
> usr/include/linux/prctl.h:#define PR_SET_TAGGED_ADDR_CTRL               55
> usr/include/linux/prctl.h-#define PR_GET_TAGGED_ADDR_CTRL               56
> usr/include/linux/prctl.h-# define PR_TAGGED_ADDR_ENABLE                (1UL << 0)
> usr/include/linux/prctl.h-
> usr/include/linux/prctl.h-#endif /* _LINUX_PRCTL_H */
>
> but how are they supposed to be found if nor the test Makefile
> neither the KSFT Makefile who installs them pass any -I options to the
> compiler ?
> I suppose <sys/prctl.h> tries to include arch specific headers from the regular system path,
> but when you are cross-compiling ?
>
> 18:34 $ make TARGETS=arm64 kselftest
> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
>         ARCH=arm64 -C ../../.. headers_install
>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/usr/include
> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc -Wall -O2 -g    tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test
> tags_test.c: In function ‘main’:
> tags_test.c:20:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>             ^~~~~~~~~~~~~~~~~~~~~~~
>             PR_GET_TID_ADDRESS
> tags_test.c:20:12: note: each undeclared identifier is reported only once for each function it appears in
> tags_test.c:20:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>                                      ^~~~~~~~~~~~~~~~~~~~~
>                                      PR_GET_DUMPABLE
> ../../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test' failed
> make[4]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test] Error 1
> Makefile:19: recipe for target 'all' failed
> make[3]: *** [all] Error 2
> Makefile:137: recipe for target 'all' failed
> make[2]: *** [all] Error 2
> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1236: recipe for target 'kselftest' failed
> make[1]: *** [kselftest] Error 2
> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> Makefile:179: recipe for target 'sub-make' failed
> make: *** [sub-make] Error 2
>
>
> In fact many KSFT testcases seems to brutally add default headers path:
>
> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/uapi/
> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/
> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../usr/include/
> tools/testing/selftests/net/Makefile:CFLAGS += -I../../../../usr/include/
> tools/testing/selftests/membarrier/Makefile:CFLAGS += -g -I../../../../usr/include/
> ...

Hi Cristian!

Indeed, I can reproduce the issue. I don't know what's the proper way
to resolve this. Adding "CFLAGS += -I../../../../usr/include/" looks
good to me. AFAICS your series resolves this issue in a similar way,
but I think we should fix this before the current rc is released. Do
you want to submit a patch that adds this simple fix or should I do
that?

Thanks!

>
> Cheers
>
> Cristian
> >
> >
> >
> >>
> >> Thanks
> >>
> >> Cristian
> >>
> >>>
> >>> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
> >>> new file mode 100644
> >>> index 000000000000..e8fae8d61ed6
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/arm64/.gitignore
> >>> @@ -0,0 +1 @@
> >>> +tags_test
> >>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> >>> new file mode 100644
> >>> index 000000000000..a61b2e743e99
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/arm64/Makefile
> >>> @@ -0,0 +1,11 @@
> >>> +# SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +# ARCH can be overridden by the user for cross compiling
> >>> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> >>> +
> >>> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> >>> +TEST_GEN_PROGS := tags_test
> >>> +TEST_PROGS := run_tags_test.sh
> >>> +endif
> >>> +
> >>> +include ../lib.mk
> >>> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
> >>> new file mode 100755
> >>> index 000000000000..745f11379930
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> >>> @@ -0,0 +1,12 @@
> >>> +#!/bin/sh
> >>> +# SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +echo "--------------------"
> >>> +echo "running tags test"
> >>> +echo "--------------------"
> >>> +./tags_test
> >>> +if [ $? -ne 0 ]; then
> >>> +     echo "[FAIL]"
> >>> +else
> >>> +     echo "[PASS]"
> >>> +fi
> >>> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
> >>> new file mode 100644
> >>> index 000000000000..22a1b266e373
> >>> --- /dev/null
> >>> +++ b/tools/testing/selftests/arm64/tags_test.c
> >>> @@ -0,0 +1,29 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +#include <stdio.h>
> >>> +#include <stdlib.h>
> >>> +#include <unistd.h>
> >>> +#include <stdint.h>
> >>> +#include <sys/prctl.h>
> >>> +#include <sys/utsname.h>
> >>> +
> >>> +#define SHIFT_TAG(tag)               ((uint64_t)(tag) << 56)
> >>> +#define SET_TAG(ptr, tag)    (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> >>> +                                     SHIFT_TAG(tag))
> >>> +
> >>> +int main(void)
> >>> +{
> >>> +     static int tbi_enabled = 0;
> >>> +     struct utsname *ptr, *tagged_ptr;
> >>> +     int err;
> >>> +
> >>> +     if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>> +             tbi_enabled = 1;
> >>> +     ptr = (struct utsname *)malloc(sizeof(*ptr));
> >>> +     if (tbi_enabled)
> >>> +             tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
> >>> +     err = uname(tagged_ptr);
> >>> +     free(ptr);
> >>> +
> >>> +     return err;
> >>> +}
> >>>
> >>
>
Cristian Marussi Sept. 4, 2019, 4:22 p.m. UTC | #7
Hi Andrey !

On 04/09/2019 15:52, Andrey Konovalov wrote:
> On Fri, Aug 23, 2019 at 7:49 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
>>
>>
>> Hi
>>
>> On 23/08/2019 18:16, Andrey Konovalov wrote:
>>> On Fri, Aug 23, 2019 at 3:56 PM Cristian Marussi
>>> <cristian.marussi@arm.com> wrote:
>>>>
>>>> Hi Andrey
>>>>
>>>> On 24/06/2019 15:33, Andrey Konovalov wrote:
>>>>> This patch is a part of a series that extends kernel ABI to allow to pass
>>>>> tagged user pointers (with the top byte set to something else other than
>>>>> 0x00) as syscall arguments.
>>>>>
>>>>> This patch adds a simple test, that calls the uname syscall with a
>>>>> tagged user pointer as an argument. Without the kernel accepting tagged
>>>>> user pointers the test fails with EFAULT.
>>>>>
>>>>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>>>>> ---
>>>>>  tools/testing/selftests/arm64/.gitignore      |  1 +
>>>>>  tools/testing/selftests/arm64/Makefile        | 11 +++++++
>>>>>  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
>>>>>  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
>>>>>  4 files changed, 53 insertions(+)
>>>>>  create mode 100644 tools/testing/selftests/arm64/.gitignore
>>>>>  create mode 100644 tools/testing/selftests/arm64/Makefile
>>>>>  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
>>>>>  create mode 100644 tools/testing/selftests/arm64/tags_test.c
>>>>
>>>> After building a fresh Kernel from arm64/for-next-core from scratch at:
>>>>
>>>> commit 239ab658bea3b387424501e7c416640d6752dc0c
>>>> Merge: 6bfa3134bd3a 42d038c4fb00 1243cb6a676f d55c5f28afaf d06fa5a118f1 34b5560db40d
>>>> Author: Will Deacon <will@kernel.org>
>>>> Date:   Thu Aug 22 18:23:53 2019 +0100
>>>>
>>>>     Merge branches 'for-next/error-injection', 'for-next/tbi', 'for-next/psci-cpuidle', 'for-next/cpu-topology' and 'for-next/52-bit-kva' into for-next/core
>>>>
>>>>
>>>> KSFT arm64 tests build is broken for me, both setting or not KBUILD_OUTPUT=
>>>>
>>>> 13:30 $ make TARGETS=arm64 kselftest-clean
>>>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>>>> rm -f -r /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
>>>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>>>>
>>>> ✔ ~/ARM/dev/src/pdsw/linux [arm64_for_next_core|…8⚑ 23]
>>>>
>>>> 13:30 $ make TARGETS=arm64 kselftest
>>>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>>>> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
>>>> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
>>>>         ARCH=arm64 -C ../../.. headers_install
>>>>   HOSTCC  scripts/basic/fixdep
>>>>   HOSTCC  scripts/unifdef
>>>> ...
>>>> ...
>>>>   HDRINST usr/include/asm/msgbuf.h
>>>>   HDRINST usr/include/asm/shmbuf.h
>>>>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/usr/include
>>>> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc     tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
>>>> tags_test.c: In function ‘main’:
>>>> tags_test.c:21:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
>>>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>>>             ^~~~~~~~~~~~~~~~~~~~~~~
>>>>             PR_GET_TID_ADDRESS
>>>> tags_test.c:21:12: note: each undeclared identifier is reported only once for each function it appears in
>>>> tags_test.c:21:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
>>>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>>>                                      ^~~~~~~~~~~~~~~~~~~~~
>>>>                                      PR_GET_DUMPABLE
>>>> ../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test' failed
>>>> make[3]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test] Error 1
>>>> Makefile:136: recipe for target 'all' failed
>>>> make[2]: *** [all] Error 2
>>>> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1237: recipe for target 'kselftest' failed
>>>> make[1]: *** [kselftest] Error 2
>>>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>>>> Makefile:179: recipe for target 'sub-make' failed
>>>> make: *** [sub-make] Error 2
>>>>
>>>> Despite seeing KSFT installing Kernel Headers, they cannot be found.
>>>>
>>>> Fixing this patch like this make it work for me:
>>>>
>>>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>>>> index a61b2e743e99..f9f79fb272f0 100644
>>>> --- a/tools/testing/selftests/arm64/Makefile
>>>> +++ b/tools/testing/selftests/arm64/Makefile
>>>> @@ -4,6 +4,7 @@
>>>>  ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>>>
>>>>  ifneq (,$(filter $(ARCH),aarch64 arm64))
>>>> +CFLAGS += -I../../../../usr/include/
>>>>  TEST_GEN_PROGS := tags_test
>>>>  TEST_PROGS := run_tags_test.sh
>>>>  endif
>>>>
>>>> but is not really a proper fix since it does NOT account for case in which you have
>>>> installed the Kernel Headers in a non standard location like when you use KBUILD_OUTPUT.
>>>>
>>>> Am I missing something ?
>>>
>>> Hm, PR_SET_TAGGED_ADDR_CTRL is defined in include/uapi/linux/prctl.h,
>>> and the test has #include <sys/prctl.h> so as long as you've updated
>>> your kernel headers this should work.
>>>
>>> (I'm OOO next week, I'll see if I can reproduce this once I'm back).
>>
>> Ok. Thanks for the reply.
>>
>> I think I've got it in my local tree having cloned arm64/for-next-core:
>>
>> 18:32 $ egrep -A 10 PR_SET_TAG ./include/uapi/linux/prctl.h
>> #define PR_SET_TAGGED_ADDR_CTRL         55
>> #define PR_GET_TAGGED_ADDR_CTRL         56
>> # define PR_TAGGED_ADDR_ENABLE          (1UL << 0)
>>
>> #endif /* _LINUX_PRCTL_H */
>>
>> and Kernel header are locally installed in my kernel src dir (by KSFT indeed)
>>
>> 18:34 $ egrep -RA 10 PR_SET_TAG usr/include/
>> usr/include/linux/prctl.h:#define PR_SET_TAGGED_ADDR_CTRL               55
>> usr/include/linux/prctl.h-#define PR_GET_TAGGED_ADDR_CTRL               56
>> usr/include/linux/prctl.h-# define PR_TAGGED_ADDR_ENABLE                (1UL << 0)
>> usr/include/linux/prctl.h-
>> usr/include/linux/prctl.h-#endif /* _LINUX_PRCTL_H */
>>
>> but how are they supposed to be found if nor the test Makefile
>> neither the KSFT Makefile who installs them pass any -I options to the
>> compiler ?
>> I suppose <sys/prctl.h> tries to include arch specific headers from the regular system path,
>> but when you are cross-compiling ?
>>
>> 18:34 $ make TARGETS=arm64 kselftest
>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
>> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
>>         ARCH=arm64 -C ../../.. headers_install
>>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/usr/include
>> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc -Wall -O2 -g    tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test
>> tags_test.c: In function ‘main’:
>> tags_test.c:20:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>             ^~~~~~~~~~~~~~~~~~~~~~~
>>             PR_GET_TID_ADDRESS
>> tags_test.c:20:12: note: each undeclared identifier is reported only once for each function it appears in
>> tags_test.c:20:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>                                      ^~~~~~~~~~~~~~~~~~~~~
>>                                      PR_GET_DUMPABLE
>> ../../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test' failed
>> make[4]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test] Error 1
>> Makefile:19: recipe for target 'all' failed
>> make[3]: *** [all] Error 2
>> Makefile:137: recipe for target 'all' failed
>> make[2]: *** [all] Error 2
>> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1236: recipe for target 'kselftest' failed
>> make[1]: *** [kselftest] Error 2
>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
>> Makefile:179: recipe for target 'sub-make' failed
>> make: *** [sub-make] Error 2
>>
>>
>> In fact many KSFT testcases seems to brutally add default headers path:
>>
>> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/uapi/
>> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/
>> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../usr/include/
>> tools/testing/selftests/net/Makefile:CFLAGS += -I../../../../usr/include/
>> tools/testing/selftests/membarrier/Makefile:CFLAGS += -g -I../../../../usr/include/
>> ...
> 
> Hi Cristian!
> 
> Indeed, I can reproduce the issue. I don't know what's the proper way
> to resolve this. Adding "CFLAGS += -I../../../../usr/include/" looks
> good to me. AFAICS your series resolves this issue in a similar way,
> but I think we should fix this before the current rc is released. Do
> you want to submit a patch that adds this simple fix or should I do
> that?
> 

Please feel free to post the single line patch above to quickly fix this before
release, so we don't have a broken build straight away. (our CI is already beating me...:D)

On my side (01/11) in the meantime I'll fix the top level KSFT arm64 makefile so as to calculate 
and propagate once for all the headers search path down to all KSFT arm64/ in one go,
trying to guess where they are; this is needed because the above fix works fine as long
as you don't have KBUILD_OUTPUT set, once you set it, KSFT installs kheaders in a different
place and the  above -I fix is fooled again....but this is a general problem also in other
KSFT tests as I can see now so I think this fix is good enough for now
(and the fix on my side, even if trivial, is not going to go into this release for sure)

Thanks !

Cheers

Cristian

> Thanks!
> 
>>
>> Cheers
>>
>> Cristian
>>>
>>>
>>>
>>>>
>>>> Thanks
>>>>
>>>> Cristian
>>>>
>>>>>
>>>>> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
>>>>> new file mode 100644
>>>>> index 000000000000..e8fae8d61ed6
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/arm64/.gitignore
>>>>> @@ -0,0 +1 @@
>>>>> +tags_test
>>>>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>>>>> new file mode 100644
>>>>> index 000000000000..a61b2e743e99
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/arm64/Makefile
>>>>> @@ -0,0 +1,11 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +# ARCH can be overridden by the user for cross compiling
>>>>> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>>>> +
>>>>> +ifneq (,$(filter $(ARCH),aarch64 arm64))
>>>>> +TEST_GEN_PROGS := tags_test
>>>>> +TEST_PROGS := run_tags_test.sh
>>>>> +endif
>>>>> +
>>>>> +include ../lib.mk
>>>>> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
>>>>> new file mode 100755
>>>>> index 000000000000..745f11379930
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
>>>>> @@ -0,0 +1,12 @@
>>>>> +#!/bin/sh
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +echo "--------------------"
>>>>> +echo "running tags test"
>>>>> +echo "--------------------"
>>>>> +./tags_test
>>>>> +if [ $? -ne 0 ]; then
>>>>> +     echo "[FAIL]"
>>>>> +else
>>>>> +     echo "[PASS]"
>>>>> +fi
>>>>> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
>>>>> new file mode 100644
>>>>> index 000000000000..22a1b266e373
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/arm64/tags_test.c
>>>>> @@ -0,0 +1,29 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +#include <stdio.h>
>>>>> +#include <stdlib.h>
>>>>> +#include <unistd.h>
>>>>> +#include <stdint.h>
>>>>> +#include <sys/prctl.h>
>>>>> +#include <sys/utsname.h>
>>>>> +
>>>>> +#define SHIFT_TAG(tag)               ((uint64_t)(tag) << 56)
>>>>> +#define SET_TAG(ptr, tag)    (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
>>>>> +                                     SHIFT_TAG(tag))
>>>>> +
>>>>> +int main(void)
>>>>> +{
>>>>> +     static int tbi_enabled = 0;
>>>>> +     struct utsname *ptr, *tagged_ptr;
>>>>> +     int err;
>>>>> +
>>>>> +     if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
>>>>> +             tbi_enabled = 1;
>>>>> +     ptr = (struct utsname *)malloc(sizeof(*ptr));
>>>>> +     if (tbi_enabled)
>>>>> +             tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
>>>>> +     err = uname(tagged_ptr);
>>>>> +     free(ptr);
>>>>> +
>>>>> +     return err;
>>>>> +}
>>>>>
>>>>
>>
Andrey Konovalov Sept. 4, 2019, 4:42 p.m. UTC | #8
On Wed, Sep 4, 2019 at 6:22 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> Hi Andrey !
>
> On 04/09/2019 15:52, Andrey Konovalov wrote:
> > On Fri, Aug 23, 2019 at 7:49 PM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> >>
> >>
> >> Hi
> >>
> >> On 23/08/2019 18:16, Andrey Konovalov wrote:
> >>> On Fri, Aug 23, 2019 at 3:56 PM Cristian Marussi
> >>> <cristian.marussi@arm.com> wrote:
> >>>>
> >>>> Hi Andrey
> >>>>
> >>>> On 24/06/2019 15:33, Andrey Konovalov wrote:
> >>>>> This patch is a part of a series that extends kernel ABI to allow to pass
> >>>>> tagged user pointers (with the top byte set to something else other than
> >>>>> 0x00) as syscall arguments.
> >>>>>
> >>>>> This patch adds a simple test, that calls the uname syscall with a
> >>>>> tagged user pointer as an argument. Without the kernel accepting tagged
> >>>>> user pointers the test fails with EFAULT.
> >>>>>
> >>>>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >>>>> ---
> >>>>>  tools/testing/selftests/arm64/.gitignore      |  1 +
> >>>>>  tools/testing/selftests/arm64/Makefile        | 11 +++++++
> >>>>>  .../testing/selftests/arm64/run_tags_test.sh  | 12 ++++++++
> >>>>>  tools/testing/selftests/arm64/tags_test.c     | 29 +++++++++++++++++++
> >>>>>  4 files changed, 53 insertions(+)
> >>>>>  create mode 100644 tools/testing/selftests/arm64/.gitignore
> >>>>>  create mode 100644 tools/testing/selftests/arm64/Makefile
> >>>>>  create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
> >>>>>  create mode 100644 tools/testing/selftests/arm64/tags_test.c
> >>>>
> >>>> After building a fresh Kernel from arm64/for-next-core from scratch at:
> >>>>
> >>>> commit 239ab658bea3b387424501e7c416640d6752dc0c
> >>>> Merge: 6bfa3134bd3a 42d038c4fb00 1243cb6a676f d55c5f28afaf d06fa5a118f1 34b5560db40d
> >>>> Author: Will Deacon <will@kernel.org>
> >>>> Date:   Thu Aug 22 18:23:53 2019 +0100
> >>>>
> >>>>     Merge branches 'for-next/error-injection', 'for-next/tbi', 'for-next/psci-cpuidle', 'for-next/cpu-topology' and 'for-next/52-bit-kva' into for-next/core
> >>>>
> >>>>
> >>>> KSFT arm64 tests build is broken for me, both setting or not KBUILD_OUTPUT=
> >>>>
> >>>> 13:30 $ make TARGETS=arm64 kselftest-clean
> >>>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >>>> rm -f -r /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
> >>>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >>>>
> >>>> ✔ ~/ARM/dev/src/pdsw/linux [arm64_for_next_core|…8⚑ 23]
> >>>>
> >>>> 13:30 $ make TARGETS=arm64 kselftest
> >>>> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >>>> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
> >>>> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
> >>>>         ARCH=arm64 -C ../../.. headers_install
> >>>>   HOSTCC  scripts/basic/fixdep
> >>>>   HOSTCC  scripts/unifdef
> >>>> ...
> >>>> ...
> >>>>   HDRINST usr/include/asm/msgbuf.h
> >>>>   HDRINST usr/include/asm/shmbuf.h
> >>>>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/usr/include
> >>>> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc     tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test
> >>>> tags_test.c: In function ‘main’:
> >>>> tags_test.c:21:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
> >>>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>>>             ^~~~~~~~~~~~~~~~~~~~~~~
> >>>>             PR_GET_TID_ADDRESS
> >>>> tags_test.c:21:12: note: each undeclared identifier is reported only once for each function it appears in
> >>>> tags_test.c:21:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
> >>>>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>>>                                      ^~~~~~~~~~~~~~~~~~~~~
> >>>>                                      PR_GET_DUMPABLE
> >>>> ../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test' failed
> >>>> make[3]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux//kselftest/arm64/tags_test] Error 1
> >>>> Makefile:136: recipe for target 'all' failed
> >>>> make[2]: *** [all] Error 2
> >>>> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1237: recipe for target 'kselftest' failed
> >>>> make[1]: *** [kselftest] Error 2
> >>>> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >>>> Makefile:179: recipe for target 'sub-make' failed
> >>>> make: *** [sub-make] Error 2
> >>>>
> >>>> Despite seeing KSFT installing Kernel Headers, they cannot be found.
> >>>>
> >>>> Fixing this patch like this make it work for me:
> >>>>
> >>>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> >>>> index a61b2e743e99..f9f79fb272f0 100644
> >>>> --- a/tools/testing/selftests/arm64/Makefile
> >>>> +++ b/tools/testing/selftests/arm64/Makefile
> >>>> @@ -4,6 +4,7 @@
> >>>>  ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> >>>>
> >>>>  ifneq (,$(filter $(ARCH),aarch64 arm64))
> >>>> +CFLAGS += -I../../../../usr/include/
> >>>>  TEST_GEN_PROGS := tags_test
> >>>>  TEST_PROGS := run_tags_test.sh
> >>>>  endif
> >>>>
> >>>> but is not really a proper fix since it does NOT account for case in which you have
> >>>> installed the Kernel Headers in a non standard location like when you use KBUILD_OUTPUT.
> >>>>
> >>>> Am I missing something ?
> >>>
> >>> Hm, PR_SET_TAGGED_ADDR_CTRL is defined in include/uapi/linux/prctl.h,
> >>> and the test has #include <sys/prctl.h> so as long as you've updated
> >>> your kernel headers this should work.
> >>>
> >>> (I'm OOO next week, I'll see if I can reproduce this once I'm back).
> >>
> >> Ok. Thanks for the reply.
> >>
> >> I think I've got it in my local tree having cloned arm64/for-next-core:
> >>
> >> 18:32 $ egrep -A 10 PR_SET_TAG ./include/uapi/linux/prctl.h
> >> #define PR_SET_TAGGED_ADDR_CTRL         55
> >> #define PR_GET_TAGGED_ADDR_CTRL         56
> >> # define PR_TAGGED_ADDR_ENABLE          (1UL << 0)
> >>
> >> #endif /* _LINUX_PRCTL_H */
> >>
> >> and Kernel header are locally installed in my kernel src dir (by KSFT indeed)
> >>
> >> 18:34 $ egrep -RA 10 PR_SET_TAG usr/include/
> >> usr/include/linux/prctl.h:#define PR_SET_TAGGED_ADDR_CTRL               55
> >> usr/include/linux/prctl.h-#define PR_GET_TAGGED_ADDR_CTRL               56
> >> usr/include/linux/prctl.h-# define PR_TAGGED_ADDR_ENABLE                (1UL << 0)
> >> usr/include/linux/prctl.h-
> >> usr/include/linux/prctl.h-#endif /* _LINUX_PRCTL_H */
> >>
> >> but how are they supposed to be found if nor the test Makefile
> >> neither the KSFT Makefile who installs them pass any -I options to the
> >> compiler ?
> >> I suppose <sys/prctl.h> tries to include arch specific headers from the regular system path,
> >> but when you are cross-compiling ?
> >>
> >> 18:34 $ make TARGETS=arm64 kselftest
> >> make[1]: Entering directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >> arch/arm64/Makefile:56: CROSS_COMPILE_COMPAT not defined or empty, the compat vDSO will not be built
> >> make --no-builtin-rules INSTALL_HDR_PATH=$BUILD/usr \
> >>         ARCH=arm64 -C ../../.. headers_install
> >>   INSTALL /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/usr/include
> >> /opt/toolchains/gcc-arm-8.3-2019.03-x86_64-aarch64-linux-gnu/bin/aarch64-linux-gnu-gcc -Wall -O2 -g    tags_test.c  -o /home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test
> >> tags_test.c: In function ‘main’:
> >> tags_test.c:20:12: error: ‘PR_SET_TAGGED_ADDR_CTRL’ undeclared (first use in this function); did you mean ‘PR_GET_TID_ADDRESS’?
> >>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>             ^~~~~~~~~~~~~~~~~~~~~~~
> >>             PR_GET_TID_ADDRESS
> >> tags_test.c:20:12: note: each undeclared identifier is reported only once for each function it appears in
> >> tags_test.c:20:37: error: ‘PR_TAGGED_ADDR_ENABLE’ undeclared (first use in this function); did you mean ‘PR_GET_DUMPABLE’?
> >>   if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>                                      ^~~~~~~~~~~~~~~~~~~~~
> >>                                      PR_GET_DUMPABLE
> >> ../../lib.mk:138: recipe for target '/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test' failed
> >> make[4]: *** [/home/crimar01/ARM/dev/src/pdsw/out_linux/kselftest/arm64/tags/tags_test] Error 1
> >> Makefile:19: recipe for target 'all' failed
> >> make[3]: *** [all] Error 2
> >> Makefile:137: recipe for target 'all' failed
> >> make[2]: *** [all] Error 2
> >> /home/crimar01/ARM/dev/src/pdsw/linux/Makefile:1236: recipe for target 'kselftest' failed
> >> make[1]: *** [kselftest] Error 2
> >> make[1]: Leaving directory '/home/crimar01/ARM/dev/src/pdsw/out_linux'
> >> Makefile:179: recipe for target 'sub-make' failed
> >> make: *** [sub-make] Error 2
> >>
> >>
> >> In fact many KSFT testcases seems to brutally add default headers path:
> >>
> >> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/uapi/
> >> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../include/
> >> tools/testing/selftests/memfd/Makefile:CFLAGS += -I../../../../usr/include/
> >> tools/testing/selftests/net/Makefile:CFLAGS += -I../../../../usr/include/
> >> tools/testing/selftests/membarrier/Makefile:CFLAGS += -g -I../../../../usr/include/
> >> ...
> >
> > Hi Cristian!
> >
> > Indeed, I can reproduce the issue. I don't know what's the proper way
> > to resolve this. Adding "CFLAGS += -I../../../../usr/include/" looks
> > good to me. AFAICS your series resolves this issue in a similar way,
> > but I think we should fix this before the current rc is released. Do
> > you want to submit a patch that adds this simple fix or should I do
> > that?
> >
>
> Please feel free to post the single line patch above to quickly fix this before
> release, so we don't have a broken build straight away. (our CI is already beating me...:D)

Done.

>
> On my side (01/11) in the meantime I'll fix the top level KSFT arm64 makefile so as to calculate
> and propagate once for all the headers search path down to all KSFT arm64/ in one go,
> trying to guess where they are; this is needed because the above fix works fine as long
> as you don't have KBUILD_OUTPUT set, once you set it, KSFT installs kheaders in a different
> place and the  above -I fix is fooled again....but this is a general problem also in other
> KSFT tests as I can see now so I think this fix is good enough for now
> (and the fix on my side, even if trivial, is not going to go into this release for sure)

Ah, I see.

Thanks for the report!

>
> Thanks !
>
> Cheers
>
> Cristian
>
> > Thanks!
> >
> >>
> >> Cheers
> >>
> >> Cristian
> >>>
> >>>
> >>>
> >>>>
> >>>> Thanks
> >>>>
> >>>> Cristian
> >>>>
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
> >>>>> new file mode 100644
> >>>>> index 000000000000..e8fae8d61ed6
> >>>>> --- /dev/null
> >>>>> +++ b/tools/testing/selftests/arm64/.gitignore
> >>>>> @@ -0,0 +1 @@
> >>>>> +tags_test
> >>>>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> >>>>> new file mode 100644
> >>>>> index 000000000000..a61b2e743e99
> >>>>> --- /dev/null
> >>>>> +++ b/tools/testing/selftests/arm64/Makefile
> >>>>> @@ -0,0 +1,11 @@
> >>>>> +# SPDX-License-Identifier: GPL-2.0
> >>>>> +
> >>>>> +# ARCH can be overridden by the user for cross compiling
> >>>>> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> >>>>> +
> >>>>> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> >>>>> +TEST_GEN_PROGS := tags_test
> >>>>> +TEST_PROGS := run_tags_test.sh
> >>>>> +endif
> >>>>> +
> >>>>> +include ../lib.mk
> >>>>> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
> >>>>> new file mode 100755
> >>>>> index 000000000000..745f11379930
> >>>>> --- /dev/null
> >>>>> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> >>>>> @@ -0,0 +1,12 @@
> >>>>> +#!/bin/sh
> >>>>> +# SPDX-License-Identifier: GPL-2.0
> >>>>> +
> >>>>> +echo "--------------------"
> >>>>> +echo "running tags test"
> >>>>> +echo "--------------------"
> >>>>> +./tags_test
> >>>>> +if [ $? -ne 0 ]; then
> >>>>> +     echo "[FAIL]"
> >>>>> +else
> >>>>> +     echo "[PASS]"
> >>>>> +fi
> >>>>> diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
> >>>>> new file mode 100644
> >>>>> index 000000000000..22a1b266e373
> >>>>> --- /dev/null
> >>>>> +++ b/tools/testing/selftests/arm64/tags_test.c
> >>>>> @@ -0,0 +1,29 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>> +
> >>>>> +#include <stdio.h>
> >>>>> +#include <stdlib.h>
> >>>>> +#include <unistd.h>
> >>>>> +#include <stdint.h>
> >>>>> +#include <sys/prctl.h>
> >>>>> +#include <sys/utsname.h>
> >>>>> +
> >>>>> +#define SHIFT_TAG(tag)               ((uint64_t)(tag) << 56)
> >>>>> +#define SET_TAG(ptr, tag)    (((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> >>>>> +                                     SHIFT_TAG(tag))
> >>>>> +
> >>>>> +int main(void)
> >>>>> +{
> >>>>> +     static int tbi_enabled = 0;
> >>>>> +     struct utsname *ptr, *tagged_ptr;
> >>>>> +     int err;
> >>>>> +
> >>>>> +     if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> >>>>> +             tbi_enabled = 1;
> >>>>> +     ptr = (struct utsname *)malloc(sizeof(*ptr));
> >>>>> +     if (tbi_enabled)
> >>>>> +             tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
> >>>>> +     err = uname(tagged_ptr);
> >>>>> +     free(ptr);
> >>>>> +
> >>>>> +     return err;
> >>>>> +}
> >>>>>
> >>>>
> >>
>

Patch
diff mbox series

diff --git a/tools/testing/selftests/arm64/.gitignore b/tools/testing/selftests/arm64/.gitignore
new file mode 100644
index 000000000000..e8fae8d61ed6
--- /dev/null
+++ b/tools/testing/selftests/arm64/.gitignore
@@ -0,0 +1 @@ 
+tags_test
diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
new file mode 100644
index 000000000000..a61b2e743e99
--- /dev/null
+++ b/tools/testing/selftests/arm64/Makefile
@@ -0,0 +1,11 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+# ARCH can be overridden by the user for cross compiling
+ARCH ?= $(shell uname -m 2>/dev/null || echo not)
+
+ifneq (,$(filter $(ARCH),aarch64 arm64))
+TEST_GEN_PROGS := tags_test
+TEST_PROGS := run_tags_test.sh
+endif
+
+include ../lib.mk
diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/run_tags_test.sh
new file mode 100755
index 000000000000..745f11379930
--- /dev/null
+++ b/tools/testing/selftests/arm64/run_tags_test.sh
@@ -0,0 +1,12 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+echo "--------------------"
+echo "running tags test"
+echo "--------------------"
+./tags_test
+if [ $? -ne 0 ]; then
+	echo "[FAIL]"
+else
+	echo "[PASS]"
+fi
diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags_test.c
new file mode 100644
index 000000000000..22a1b266e373
--- /dev/null
+++ b/tools/testing/selftests/arm64/tags_test.c
@@ -0,0 +1,29 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <sys/prctl.h>
+#include <sys/utsname.h>
+
+#define SHIFT_TAG(tag)		((uint64_t)(tag) << 56)
+#define SET_TAG(ptr, tag)	(((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
+					SHIFT_TAG(tag))
+
+int main(void)
+{
+	static int tbi_enabled = 0;
+	struct utsname *ptr, *tagged_ptr;
+	int err;
+
+	if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
+		tbi_enabled = 1;
+	ptr = (struct utsname *)malloc(sizeof(*ptr));
+	if (tbi_enabled)
+		tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
+	err = uname(tagged_ptr);
+	free(ptr);
+
+	return err;
+}