| Message ID | 20230607153600.15816-1-osmtendev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] selftests: prctl: Add new prctl test for PR_SET_NAME | expand |
On 6/7/23 09:36, Osama Muhammad wrote: > This patch will add the new test, which covers the prctl call > PR_SET_NAME command. The test tries to give a name using the PR_SET_NAME > call and then confirm it that it changed correctly by using PR_GET_NAME. > It also tries to rename it with empty name.In the test PR_GET_NAME is > tested by passing null pointer to it and check its behaviour. > > Signed-off-by: Osama Muhammad <osmtendev@gmail.com> > > --- > changes since v1: > -Used TASK_COMM_LEN instead of using numerical value 16. Please add linux/sched.h here as an include to pull this. It is good to add an explicit include as opposed taking a chance on it being included from another include. thanks, -- Shuah
Hi all, I looked into it and tried to use TASK_COMM_LEN in the test. Even though I included "linux/sched.h '', I was not able to compile the test because it couldn't find it in the header file. I dived deep into the issue and turns out header file mapped in /usr/include/linux/sched.h is actually mapped to /include/uapi/linux/sched.h[1] in linux source, where TASK_COMM_LEN is not even defined. Instead TASK_COMM_LEN is defined in /include/linux/sched.h which is not mapped to any header files in userspace(/(/usr/include/linux). I also tried to find the TASK_COM_LEN in /usr/include/linux/ but I couldn't find it. Following are the search results. grep -rnw '/usr/include/linux/' -e 'TASK_COMM_LEN" RESULTS OF COMMAND :- /usr/include/linux/taskstats.h:38:#define TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN Based on this information, I have two questions: 1. Would this require a fix to move 'TASK_COMM_LEN' macro from /include/linux/sched.h to UAPI headers /include/uapi/linux/sched.h. 2. Is there any other way to access TASK_COMM_LEN in the selftest that I'm not aware of? [1]:-https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h Thanks, Osama On Sat, 10 Jun 2023 at 02:43, Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 6/7/23 09:36, Osama Muhammad wrote: > > This patch will add the new test, which covers the prctl call > > PR_SET_NAME command. The test tries to give a name using the PR_SET_NAME > > call and then confirm it that it changed correctly by using PR_GET_NAME. > > It also tries to rename it with empty name.In the test PR_GET_NAME is > > tested by passing null pointer to it and check its behaviour. > > > > Signed-off-by: Osama Muhammad <osmtendev@gmail.com> > > > > --- > > changes since v1: > > -Used TASK_COMM_LEN instead of using numerical value 16. > > Please add linux/sched.h here as an include to pull this. > It is good to add an explicit include as opposed taking > a chance on it being included from another include. > > thanks, > -- Shuah
On 6/10/23 07:01, Osama Muhammad wrote: > Hi all, > > I looked into it and tried to use TASK_COMM_LEN in the test. Even > though I included "linux/sched.h '', I was not able to compile the > test because it couldn't find it in the header file. > I dived deep into the issue and turns out header file mapped in > /usr/include/linux/sched.h is actually mapped to > /include/uapi/linux/sched.h[1] in linux source, > where TASK_COMM_LEN is not even defined. Instead TASK_COMM_LEN is > defined in /include/linux/sched.h which is not mapped to any header > files in > userspace(/(/usr/include/linux). > I also tried to find the TASK_COM_LEN in /usr/include/linux/ but I > couldn't find it. Following are the search results. > grep -rnw '/usr/include/linux/' -e 'TASK_COMM_LEN" > RESULTS OF COMMAND :- /usr/include/linux/taskstats.h:38:#define > TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN > Based on this information, I have two questions: > 1. Would this require a fix to move 'TASK_COMM_LEN' macro from > /include/linux/sched.h to UAPI headers /include/uapi/linux/sched.h. > 2. Is there any other way to access TASK_COMM_LEN in the selftest that > I'm not aware of? > > [1]:-https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h > The best source is Linux mainline. Take a look at test files that include linux/sched.h arm64/abi/tpidr2.c is one of them. Did you install headers before compiling the test? make headers_install thanks, -- Shuah
Hi, Yes, I did install the latest kernel headers and TASK_COMM_LEN is not accessible in userspace. I looked into the test which uses TASK_COMM_LEN but the test defines it in its own header file. Example: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/progs/pyperf.h#L13 TASK_COMM_LEN is defined in include/linux/sched.h, but this header file is not exposed to userspace. TASK_COMM_LEN is not defined in /include/uapi/linux/sched.h which is exposed to userspace kernel headers. Please find the link to the header file exposed to user space :- -https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h As for arm64/abi/tpidr2.c It includes linux/sched.h which will be /include/uapi/linux/sched.h because the user space program is including it. So it also cannot use TASK_COMM_LEN directly. Regards, Osama On Tue, 13 Jun 2023 at 02:56, Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 6/10/23 07:01, Osama Muhammad wrote: > > Hi all, > > > > I looked into it and tried to use TASK_COMM_LEN in the test. Even > > though I included "linux/sched.h '', I was not able to compile the > > test because it couldn't find it in the header file. > > I dived deep into the issue and turns out header file mapped in > > /usr/include/linux/sched.h is actually mapped to > > /include/uapi/linux/sched.h[1] in linux source, > > where TASK_COMM_LEN is not even defined. Instead TASK_COMM_LEN is > > defined in /include/linux/sched.h which is not mapped to any header > > files in > > userspace(/(/usr/include/linux). > > I also tried to find the TASK_COM_LEN in /usr/include/linux/ but I > > couldn't find it. Following are the search results. > > grep -rnw '/usr/include/linux/' -e 'TASK_COMM_LEN" > > RESULTS OF COMMAND :- /usr/include/linux/taskstats.h:38:#define > > TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN > > Based on this information, I have two questions: > > 1. Would this require a fix to move 'TASK_COMM_LEN' macro from > > /include/linux/sched.h to UAPI headers /include/uapi/linux/sched.h. > > 2. Is there any other way to access TASK_COMM_LEN in the selftest that > > I'm not aware of? > > > > [1]:-https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h > > > > The best source is Linux mainline. > > Take a look at test files that include linux/sched.h > > arm64/abi/tpidr2.c is one of them. > > Did you install headers before compiling the test? > make headers_install > > thanks, > -- Shuah > >
Hi Shuah, Any feedback on this patch?. Thanks, Osama On Sat, 17 Jun 2023 at 18:01, Osama Muhammad <osmtendev@gmail.com> wrote: > > Hi, > > Yes, I did install the latest kernel headers and TASK_COMM_LEN is not > accessible in userspace. > > I looked into the test which uses TASK_COMM_LEN but the test defines > it in its own header file. > > Example: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/progs/pyperf.h#L13 > > TASK_COMM_LEN is defined in include/linux/sched.h, but this header > file is not exposed to userspace. > > TASK_COMM_LEN is not defined in /include/uapi/linux/sched.h which is > exposed to userspace kernel headers. > Please find the link to the header file exposed to user space :- > -https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h > > As for arm64/abi/tpidr2.c It includes linux/sched.h which will be > /include/uapi/linux/sched.h because the user space program is > including it. > So it also cannot use TASK_COMM_LEN directly. > > Regards, > Osama > > On Tue, 13 Jun 2023 at 02:56, Shuah Khan <skhan@linuxfoundation.org> wrote: > > > > On 6/10/23 07:01, Osama Muhammad wrote: > > > Hi all, > > > > > > I looked into it and tried to use TASK_COMM_LEN in the test. Even > > > though I included "linux/sched.h '', I was not able to compile the > > > test because it couldn't find it in the header file. > > > I dived deep into the issue and turns out header file mapped in > > > /usr/include/linux/sched.h is actually mapped to > > > /include/uapi/linux/sched.h[1] in linux source, > > > where TASK_COMM_LEN is not even defined. Instead TASK_COMM_LEN is > > > defined in /include/linux/sched.h which is not mapped to any header > > > files in > > > userspace(/(/usr/include/linux). > > > I also tried to find the TASK_COM_LEN in /usr/include/linux/ but I > > > couldn't find it. Following are the search results. > > > grep -rnw '/usr/include/linux/' -e 'TASK_COMM_LEN" > > > RESULTS OF COMMAND :- /usr/include/linux/taskstats.h:38:#define > > > TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN > > > Based on this information, I have two questions: > > > 1. Would this require a fix to move 'TASK_COMM_LEN' macro from > > > /include/linux/sched.h to UAPI headers /include/uapi/linux/sched.h. > > > 2. Is there any other way to access TASK_COMM_LEN in the selftest that > > > I'm not aware of? > > > > > > [1]:-https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h > > > > > > > The best source is Linux mainline. > > > > Take a look at test files that include linux/sched.h > > > > arm64/abi/tpidr2.c is one of them. > > > > Did you install headers before compiling the test? > > make headers_install > > > > thanks, > > -- Shuah > > > >
On 6/10/23 2:43 AM, Shuah Khan wrote: > On 6/7/23 09:36, Osama Muhammad wrote: >> This patch will add the new test, which covers the prctl call >> PR_SET_NAME command. The test tries to give a name using the PR_SET_NAME >> call and then confirm it that it changed correctly by using PR_GET_NAME. >> It also tries to rename it with empty name.In the test PR_GET_NAME is >> tested by passing null pointer to it and check its behaviour. >> >> Signed-off-by: Osama Muhammad <osmtendev@gmail.com> >> >> --- >> changes since v1: >> -Used TASK_COMM_LEN instead of using numerical value 16. > > Please add linux/sched.h here as an include to pull this. > It is good to add an explicit include as opposed taking > a chance on it being included from another include. TASK_COMM_LEN is defined in include/linux/sched.h. It is only to be used by the kernel. If we look at include/uapi/linux/sched.h, TASK_COMM_LEN isn't defined. So this test looks good to me. > > thanks, > -- Shuah
Hi, Thank you for the patch. This patch cleanly applies on next-20230627. Unrelated to this patch: I'm not sure if this patch was written against linux next. Always try to send patches against latest next tag from following repo: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git On 6/26/23 11:36 PM, Osama Muhammad wrote: > Hi Shuah, > > Any feedback on this patch?. > > Thanks, > Osama > > > On Sat, 17 Jun 2023 at 18:01, Osama Muhammad <osmtendev@gmail.com> wrote: >> >> Hi, >> >> Yes, I did install the latest kernel headers and TASK_COMM_LEN is not >> accessible in userspace. >> >> I looked into the test which uses TASK_COMM_LEN but the test defines >> it in its own header file. >> >> Example: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/progs/pyperf.h#L13 >> >> TASK_COMM_LEN is defined in include/linux/sched.h, but this header >> file is not exposed to userspace. >> >> TASK_COMM_LEN is not defined in /include/uapi/linux/sched.h which is >> exposed to userspace kernel headers. >> Please find the link to the header file exposed to user space :- >> -https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h >> >> As for arm64/abi/tpidr2.c It includes linux/sched.h which will be >> /include/uapi/linux/sched.h because the user space program is >> including it. >> So it also cannot use TASK_COMM_LEN directly. >> >> Regards, >> Osama >> >> On Tue, 13 Jun 2023 at 02:56, Shuah Khan <skhan@linuxfoundation.org> wrote: >>> >>> On 6/10/23 07:01, Osama Muhammad wrote: >>>> Hi all, >>>> >>>> I looked into it and tried to use TASK_COMM_LEN in the test. Even >>>> though I included "linux/sched.h '', I was not able to compile the >>>> test because it couldn't find it in the header file. >>>> I dived deep into the issue and turns out header file mapped in >>>> /usr/include/linux/sched.h is actually mapped to >>>> /include/uapi/linux/sched.h[1] in linux source, >>>> where TASK_COMM_LEN is not even defined. Instead TASK_COMM_LEN is >>>> defined in /include/linux/sched.h which is not mapped to any header >>>> files in >>>> userspace(/(/usr/include/linux). >>>> I also tried to find the TASK_COM_LEN in /usr/include/linux/ but I >>>> couldn't find it. Following are the search results. >>>> grep -rnw '/usr/include/linux/' -e 'TASK_COMM_LEN" >>>> RESULTS OF COMMAND :- /usr/include/linux/taskstats.h:38:#define >>>> TS_COMM_LEN 32 /* should be >= TASK_COMM_LEN >>>> Based on this information, I have two questions: >>>> 1. Would this require a fix to move 'TASK_COMM_LEN' macro from >>>> /include/linux/sched.h to UAPI headers /include/uapi/linux/sched.h. >>>> 2. Is there any other way to access TASK_COMM_LEN in the selftest that >>>> I'm not aware of? >>>> >>>> [1]:-https://elixir.bootlin.com/linux/v5.15.116/source/include/uapi/linux/sched.h >>>> >>> >>> The best source is Linux mainline. >>> >>> Take a look at test files that include linux/sched.h >>> >>> arm64/abi/tpidr2.c is one of them. >>> >>> Did you install headers before compiling the test? >>> make headers_install >>> >>> thanks, >>> -- Shuah >>> >>>
Unrelated to this patch: You can build the user header files by running `make headers` in a kernel repository to process the kernel header files. They are generated in <kernel_source>/include/uapi Then run `make -C tools/testing/selftests/prctl` to build the test etc. On 6/7/23 8:36 PM, Osama Muhammad wrote: > This patch will add the new test, which covers the prctl call > PR_SET_NAME command. The test tries to give a name using the PR_SET_NAME > call and then confirm it that it changed correctly by using PR_GET_NAME. > It also tries to rename it with empty name.In the test PR_GET_NAME is > tested by passing null pointer to it and check its behaviour. > > Signed-off-by: Osama Muhammad <osmtendev@gmail.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > --- > changes since v1: > -Used TASK_COMM_LEN instead of using numerical value 16. > > --- > tools/testing/selftests/prctl/Makefile | 2 +- > .../selftests/prctl/set-process-name.c | 62 +++++++++++++++++++ > 2 files changed, 63 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/prctl/set-process-name.c > > diff --git a/tools/testing/selftests/prctl/Makefile b/tools/testing/selftests/prctl/Makefile > index c058b81ee..cfc35d29f 100644 > --- a/tools/testing/selftests/prctl/Makefile > +++ b/tools/testing/selftests/prctl/Makefile > @@ -5,7 +5,7 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/) > > ifeq ($(ARCH),x86) > TEST_PROGS := disable-tsc-ctxt-sw-stress-test disable-tsc-on-off-stress-test \ > - disable-tsc-test set-anon-vma-name-test > + disable-tsc-test set-anon-vma-name-test set-process-name > all: $(TEST_PROGS) > > include ../lib.mk > diff --git a/tools/testing/selftests/prctl/set-process-name.c b/tools/testing/selftests/prctl/set-process-name.c > new file mode 100644 > index 000000000..3bc5e0e09 > --- /dev/null > +++ b/tools/testing/selftests/prctl/set-process-name.c > @@ -0,0 +1,62 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This test covers the PR_SET_NAME functionality of prctl calls > + */ > + > +#include <errno.h> > +#include <sys/prctl.h> > +#include <string.h> > + > +#include "../kselftest_harness.h" > + > +#define CHANGE_NAME "changename" > +#define EMPTY_NAME "" > +#define TASK_COMM_LEN 16 > + > +int set_name(char *name) > +{ > + int res; > + > + res = prctl(PR_SET_NAME, name, NULL, NULL, NULL); > + > + if (res < 0) > + return -errno; > + return res; > +} > + > +int check_is_name_correct(char *check_name) > +{ > + char name[TASK_COMM_LEN]; > + int res; > + > + res = prctl(PR_GET_NAME, name, NULL, NULL, NULL); > + > + if (res < 0) > + return -errno; > + > + return !strcmp(name, check_name); > +} > + > +int check_null_pointer(char *check_name) > +{ > + char *name = NULL; > + int res; > + > + res = prctl(PR_GET_NAME, name, NULL, NULL, NULL); > + > + return res; > +} > + > +TEST(rename_process) { > + > + EXPECT_GE(set_name(CHANGE_NAME), 0); > + EXPECT_TRUE(check_is_name_correct(CHANGE_NAME)); > + > + EXPECT_GE(set_name(EMPTY_NAME), 0); > + EXPECT_TRUE(check_is_name_correct(EMPTY_NAME)); > + > + EXPECT_GE(set_name(CHANGE_NAME), 0); > + EXPECT_LT(check_null_pointer(CHANGE_NAME), 0); > +} > + > +TEST_HARNESS_MAIN
On 6/26/23 12:36, Osama Muhammad wrote: > Hi Shuah, > > Any feedback on this patch?. > > Thanks, > Osama > > Please don't top post when you are responding on kernel mailing lists. It gets very difficult to follow the comments in the email thread. > On Sat, 17 Jun 2023 at 18:01, Osama Muhammad <osmtendev@gmail.com> wrote: >> >> Hi, >> >> Yes, I did install the latest kernel headers and TASK_COMM_LEN is not >> accessible in userspace. >> >> I looked into the test which uses TASK_COMM_LEN but the test defines >> it in its own header file. >> >> Example: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/progs/pyperf.h#L13 bfp test does things differently because its dependencies on run-time environment. >> >> TASK_COMM_LEN is defined in include/linux/sched.h, but this header >> file is not exposed to userspace. Correct. you can include linux/sched.h like other tests do Take a look at tools/testing/selftests/clone3 thanks, -- Shuah
On 7/14/23 10:21, Shuah Khan wrote: > On 6/26/23 12:36, Osama Muhammad wrote: >> Hi Shuah, >> >> Any feedback on this patch?. >> >> Thanks, >> Osama >> >> > > Please don't top post when you are responding on kernel > mailing lists. It gets very difficult to follow the > comments in the email thread. > >> On Sat, 17 Jun 2023 at 18:01, Osama Muhammad <osmtendev@gmail.com> wrote: >>> >>> Hi, >>> >>> Yes, I did install the latest kernel headers and TASK_COMM_LEN is not >>> accessible in userspace. >>> >>> I looked into the test which uses TASK_COMM_LEN but the test defines >>> it in its own header file. >>> >>> Example: https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/progs/pyperf.h#L13 > > bfp test does things differently because its dependencies > on run-time environment. > >>> >>> TASK_COMM_LEN is defined in include/linux/sched.h, but this header >>> file is not exposed to userspace. > > Correct. you can include linux/sched.h like other tests do > Take a look at tools/testing/selftests/clone3 > You are right about TASK_COMM_LEN not visible to user-space. This patch has been applied to linux-kselftest next for 6.6-rc1 thanks, -- Shuah
diff --git a/tools/testing/selftests/prctl/Makefile b/tools/testing/selftests/prctl/Makefile index c058b81ee..cfc35d29f 100644 --- a/tools/testing/selftests/prctl/Makefile +++ b/tools/testing/selftests/prctl/Makefile @@ -5,7 +5,7 @@ ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/) ifeq ($(ARCH),x86) TEST_PROGS := disable-tsc-ctxt-sw-stress-test disable-tsc-on-off-stress-test \ - disable-tsc-test set-anon-vma-name-test + disable-tsc-test set-anon-vma-name-test set-process-name all: $(TEST_PROGS) include ../lib.mk diff --git a/tools/testing/selftests/prctl/set-process-name.c b/tools/testing/selftests/prctl/set-process-name.c new file mode 100644 index 000000000..3bc5e0e09 --- /dev/null +++ b/tools/testing/selftests/prctl/set-process-name.c @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * This test covers the PR_SET_NAME functionality of prctl calls + */ + +#include <errno.h> +#include <sys/prctl.h> +#include <string.h> + +#include "../kselftest_harness.h" + +#define CHANGE_NAME "changename" +#define EMPTY_NAME "" +#define TASK_COMM_LEN 16 + +int set_name(char *name) +{ + int res; + + res = prctl(PR_SET_NAME, name, NULL, NULL, NULL); + + if (res < 0) + return -errno; + return res; +} + +int check_is_name_correct(char *check_name) +{ + char name[TASK_COMM_LEN]; + int res; + + res = prctl(PR_GET_NAME, name, NULL, NULL, NULL); + + if (res < 0) + return -errno; + + return !strcmp(name, check_name); +} + +int check_null_pointer(char *check_name) +{ + char *name = NULL; + int res; + + res = prctl(PR_GET_NAME, name, NULL, NULL, NULL); + + return res; +} + +TEST(rename_process) { + + EXPECT_GE(set_name(CHANGE_NAME), 0); + EXPECT_TRUE(check_is_name_correct(CHANGE_NAME)); + + EXPECT_GE(set_name(EMPTY_NAME), 0); + EXPECT_TRUE(check_is_name_correct(EMPTY_NAME)); + + EXPECT_GE(set_name(CHANGE_NAME), 0); + EXPECT_LT(check_null_pointer(CHANGE_NAME), 0); +} + +TEST_HARNESS_MAIN
This patch will add the new test, which covers the prctl call PR_SET_NAME command. The test tries to give a name using the PR_SET_NAME call and then confirm it that it changed correctly by using PR_GET_NAME. It also tries to rename it with empty name.In the test PR_GET_NAME is tested by passing null pointer to it and check its behaviour. Signed-off-by: Osama Muhammad <osmtendev@gmail.com> --- changes since v1: -Used TASK_COMM_LEN instead of using numerical value 16. --- tools/testing/selftests/prctl/Makefile | 2 +- .../selftests/prctl/set-process-name.c | 62 +++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/prctl/set-process-name.c