Re: [PATCH v18 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel

From: Andrey Konovalov
Date: Wed Sep 04 2019 - 12:43:00 EST

Next message: Michael Kelley: "RE: [PATCH v4 02/12] x86/hyper-v: Implement hv_is_hibernation_supported()"
Previous message: Eric W. Biederman: "Re: [PATCH AUTOSEL 4.19 111/167] signal/arc: Use force_sig_fault where appropriate"
In reply to: Cristian Marussi: "Re: [PATCH v18 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Sep 4, 2019 at 6:22 PM Cristian Marussi
<cristian.marussi@xxxxxxx> 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@xxxxxxx> wrote:
> >>
> >>
> >> Hi
> >>
> >> On 23/08/2019 18:16, Andrey Konovalov wrote:
> >>> On Fri, Aug 23, 2019 at 3:56 PM Cristian Marussi
> >>> <cristian.marussi@xxxxxxx> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>> 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@xxxxxxxxxx>
> >>>> 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;
> >>>>> +}
> >>>>>
> >>>>
> >>
>

Next message: Michael Kelley: "RE: [PATCH v4 02/12] x86/hyper-v: Implement hv_is_hibernation_supported()"
Previous message: Eric W. Biederman: "Re: [PATCH AUTOSEL 4.19 111/167] signal/arc: Use force_sig_fault where appropriate"
In reply to: Cristian Marussi: "Re: [PATCH v18 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]