| Message ID | 20241010121612.2601444-1-zhouyuhang1010@163.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: clone3: Use the capget and capset syscall directly | expand |
On 10/10/24 06:16, zhouyuhang wrote: > From: zhouyuhang <zhouyuhang@kylinos.cn> > > The libcap commit aca076443591 ("Make cap_t operations thread safe.") added a > __u8 mutex at the beginning of the struct _cap_struct,it changes the offset of > the members in the structure that breaks the assumption made in the "struct libcap" > definition in clone3_cap_checkpoint_restore.c.So use the capget and capset syscall > directly and remove the libcap library dependency like the commit 663af70aabb7 > ("bpf: selftests: Add helpers to directly use the capget and capset syscall") does. > NIT: grammar and comma spacing. Please fix those for readability. e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it" Fix others as well. > Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn> > --- > tools/testing/selftests/clone3/Makefile | 1 - > .../clone3/clone3_cap_checkpoint_restore.c | 60 +++++++++---------- > 2 files changed, 28 insertions(+), 33 deletions(-) > > diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile > index 84832c369a2e..59d26e8da8d2 100644 > --- a/tools/testing/selftests/clone3/Makefile > +++ b/tools/testing/selftests/clone3/Makefile > @@ -1,6 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) > -LDLIBS += -lcap > > TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ > clone3_cap_checkpoint_restore > diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c > index 3c196fa86c99..111912e2aead 100644 > --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c > +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c > @@ -15,7 +15,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <stdbool.h> > -#include <sys/capability.h> > +#include <linux/capability.h> > #include <sys/prctl.h> > #include <sys/syscall.h> > #include <sys/types.h> > @@ -27,6 +27,13 @@ > #include "../kselftest_harness.h" > #include "clone3_selftests.h" > > +#ifndef CAP_CHECKPOINT_RESTORE > +#define CAP_CHECKPOINT_RESTORE 40 > +#endif > + Why is this necessary? This is defined in linux/capability.h. > +int capget(cap_user_header_t header, cap_user_data_t data); > +int capset(cap_user_header_t header, const cap_user_data_t data); In general prototypes such as these should be defined in header file. Why are we defining these here? These are defined in sys/capability.h I don't understand this change. You are removing sys/capability.h which requires you to add these defines here. This doesn't sound like a correct solution to me. > + > static void child_exit(int ret) > { > fflush(stdout); > @@ -87,47 +94,36 @@ static int test_clone3_set_tid(struct __test_metadata *_metadata, > return ret; > } > > -struct libcap { > - struct __user_cap_header_struct hdr; > - struct __user_cap_data_struct data[2]; > -}; > - > static int set_capability(void) > { > - cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID }; > - struct libcap *cap; > - int ret = -1; > - cap_t caps; > - > - caps = cap_get_proc(); > - if (!caps) { > - perror("cap_get_proc"); > + struct __user_cap_data_struct data[2]; > + struct __user_cap_header_struct hdr = { > + .version = _LINUX_CAPABILITY_VERSION_3, > + }; > + __u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID; > + __u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32); > + int ret; > + > + ret = capget(&hdr, data); > + if (ret) { > + perror("capget"); > return -1; > } > > /* Drop all capabilities */ > - if (cap_clear(caps)) { > - perror("cap_clear"); > - goto out; > - } > + memset(&data, 0, sizeof(data)); > > - cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET); > - cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET); > + data[0].effective |= cap0; > + data[0].permitted |= cap0; > > - cap = (struct libcap *) caps; > + data[1].effective |= cap1; > + data[1].permitted |= cap1; > > - /* 40 -> CAP_CHECKPOINT_RESTORE */ > - cap->data[1].effective |= 1 << (40 - 32); > - cap->data[1].permitted |= 1 << (40 - 32); > - > - if (cap_set_proc(caps)) { > - perror("cap_set_proc"); > - goto out; > + ret = capset(&hdr, data); > + if (ret) { > + perror("capset"); > + return -1; > } > - ret = 0; > -out: > - if (cap_free(caps)) > - perror("cap_free"); > return ret; > } > thanks, -- Shuah
On 2024/10/10 23:50, Shuah Khan wrote: > On 10/10/24 06:16, zhouyuhang wrote: >> From: zhouyuhang <zhouyuhang@kylinos.cn> >> >> The libcap commit aca076443591 ("Make cap_t operations thread safe.") >> added a >> __u8 mutex at the beginning of the struct _cap_struct,it changes the >> offset of >> the members in the structure that breaks the assumption made in the >> "struct libcap" >> definition in clone3_cap_checkpoint_restore.c.So use the capget and >> capset syscall >> directly and remove the libcap library dependency like the commit >> 663af70aabb7 >> ("bpf: selftests: Add helpers to directly use the capget and capset >> syscall") does. >> > > NIT: grammar and comma spacing. Please fix those for readability. > e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it" > Fix others as well. > Thanks, I'll fix it in V2 >> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn> >> --- >> tools/testing/selftests/clone3/Makefile | 1 - >> .../clone3/clone3_cap_checkpoint_restore.c | 60 +++++++++---------- >> 2 files changed, 28 insertions(+), 33 deletions(-) >> >> diff --git a/tools/testing/selftests/clone3/Makefile >> b/tools/testing/selftests/clone3/Makefile >> index 84832c369a2e..59d26e8da8d2 100644 >> --- a/tools/testing/selftests/clone3/Makefile >> +++ b/tools/testing/selftests/clone3/Makefile >> @@ -1,6 +1,5 @@ >> # SPDX-License-Identifier: GPL-2.0 >> CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) >> -LDLIBS += -lcap >> TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ >> clone3_cap_checkpoint_restore >> diff --git >> a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >> b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >> index 3c196fa86c99..111912e2aead 100644 >> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >> @@ -15,7 +15,7 @@ >> #include <stdio.h> >> #include <stdlib.h> >> #include <stdbool.h> >> -#include <sys/capability.h> >> +#include <linux/capability.h> >> #include <sys/prctl.h> >> #include <sys/syscall.h> >> #include <sys/types.h> >> @@ -27,6 +27,13 @@ >> #include "../kselftest_harness.h" >> #include "clone3_selftests.h" >> +#ifndef CAP_CHECKPOINT_RESTORE >> +#define CAP_CHECKPOINT_RESTORE 40 >> +#endif >> + > > Why is this necessary? This is defined in linux/capability.h. > >> +int capget(cap_user_header_t header, cap_user_data_t data); >> +int capset(cap_user_header_t header, const cap_user_data_t data); > > In general prototypes such as these should be defined in header > file. Why are we defining these here? > > These are defined in sys/capability.h > > I don't understand this change. You are removing sys/capability.h > which requires you to add these defines here. This doesn't > sound like a correct solution to me. > I tested it on my machine without libcap-dev installed, the /usr/include/linux/capability.h is on this machine by default. Successfully compiled using #include <linux/capability.h> but not with #include <sys/capability.h>. This patch removes libcap library dependencies. And we don't use any part of sys/capability.h other than these two syscalls. So I think that's why it's necessary. >> + >> static void child_exit(int ret) >> { >> fflush(stdout); >> @@ -87,47 +94,36 @@ static int test_clone3_set_tid(struct >> __test_metadata *_metadata, >> return ret; >> } >> -struct libcap { >> - struct __user_cap_header_struct hdr; >> - struct __user_cap_data_struct data[2]; >> -}; >> - >> static int set_capability(void) >> { >> - cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID }; >> - struct libcap *cap; >> - int ret = -1; >> - cap_t caps; >> - >> - caps = cap_get_proc(); >> - if (!caps) { >> - perror("cap_get_proc"); >> + struct __user_cap_data_struct data[2]; >> + struct __user_cap_header_struct hdr = { >> + .version = _LINUX_CAPABILITY_VERSION_3, >> + }; >> + __u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID; >> + __u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32); >> + int ret; >> + >> + ret = capget(&hdr, data); >> + if (ret) { >> + perror("capget"); >> return -1; >> } >> /* Drop all capabilities */ >> - if (cap_clear(caps)) { >> - perror("cap_clear"); >> - goto out; >> - } >> + memset(&data, 0, sizeof(data)); >> - cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET); >> - cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET); >> + data[0].effective |= cap0; >> + data[0].permitted |= cap0; >> - cap = (struct libcap *) caps; >> + data[1].effective |= cap1; >> + data[1].permitted |= cap1; >> - /* 40 -> CAP_CHECKPOINT_RESTORE */ >> - cap->data[1].effective |= 1 << (40 - 32); >> - cap->data[1].permitted |= 1 << (40 - 32); >> - >> - if (cap_set_proc(caps)) { >> - perror("cap_set_proc"); >> - goto out; >> + ret = capset(&hdr, data); >> + if (ret) { >> + perror("capset"); >> + return -1; >> } >> - ret = 0; >> -out: >> - if (cap_free(caps)) >> - perror("cap_free"); >> return ret; >> } > > thanks, > -- Shuah
On 10/11/24 00:59, zhouyuhang wrote: > > On 2024/10/10 23:50, Shuah Khan wrote: >> On 10/10/24 06:16, zhouyuhang wrote: >>> From: zhouyuhang <zhouyuhang@kylinos.cn> >>> >>> The libcap commit aca076443591 ("Make cap_t operations thread safe.") added a >>> __u8 mutex at the beginning of the struct _cap_struct,it changes the offset of >>> the members in the structure that breaks the assumption made in the "struct libcap" >>> definition in clone3_cap_checkpoint_restore.c.So use the capget and capset syscall >>> directly and remove the libcap library dependency like the commit 663af70aabb7 >>> ("bpf: selftests: Add helpers to directly use the capget and capset syscall") does. >>> >> >> NIT: grammar and comma spacing. Please fix those for readability. >> e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it" >> Fix others as well. >> > > Thanks, I'll fix it in V2 > > >>> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn> >>> --- >>> tools/testing/selftests/clone3/Makefile | 1 - >>> .../clone3/clone3_cap_checkpoint_restore.c | 60 +++++++++---------- >>> 2 files changed, 28 insertions(+), 33 deletions(-) >>> >>> diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile >>> index 84832c369a2e..59d26e8da8d2 100644 >>> --- a/tools/testing/selftests/clone3/Makefile >>> +++ b/tools/testing/selftests/clone3/Makefile >>> @@ -1,6 +1,5 @@ >>> # SPDX-License-Identifier: GPL-2.0 >>> CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) >>> -LDLIBS += -lcap >>> TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ >>> clone3_cap_checkpoint_restore >>> diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>> index 3c196fa86c99..111912e2aead 100644 >>> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>> @@ -15,7 +15,7 @@ >>> #include <stdio.h> >>> #include <stdlib.h> >>> #include <stdbool.h> >>> -#include <sys/capability.h> >>> +#include <linux/capability.h> >>> #include <sys/prctl.h> >>> #include <sys/syscall.h> >>> #include <sys/types.h> >>> @@ -27,6 +27,13 @@ >>> #include "../kselftest_harness.h" >>> #include "clone3_selftests.h" >>> +#ifndef CAP_CHECKPOINT_RESTORE >>> +#define CAP_CHECKPOINT_RESTORE 40 >>> +#endif >>> + >> >> Why is this necessary? This is defined in linux/capability.h. >> >>> +int capget(cap_user_header_t header, cap_user_data_t data); >>> +int capset(cap_user_header_t header, const cap_user_data_t data); >> >> In general prototypes such as these should be defined in header >> file. Why are we defining these here? >> >> These are defined in sys/capability.h >> >> I don't understand this change. You are removing sys/capability.h >> which requires you to add these defines here. This doesn't >> sound like a correct solution to me. >> > > I tested it on my machine without libcap-dev installed, the /usr/include/linux/capability.h > > is on this machine by default. Successfully compiled using #include <linux/capability.h> > > but not with #include <sys/capability.h>. This patch removes libcap library dependencies. > > And we don't use any part of sys/capability.h other than these two syscalls. So I think that's why it's necessary. You are changing the code to not include sys/capability.h What happens if sys/capability.h along with linux/capability.h Do you see problems? thanks, -- Shuah
On 2024/10/11 22:21, Shuah Khan wrote: > On 10/11/24 00:59, zhouyuhang wrote: >> >> On 2024/10/10 23:50, Shuah Khan wrote: >>> On 10/10/24 06:16, zhouyuhang wrote: >>>> From: zhouyuhang <zhouyuhang@kylinos.cn> >>>> >>>> The libcap commit aca076443591 ("Make cap_t operations thread >>>> safe.") added a >>>> __u8 mutex at the beginning of the struct _cap_struct,it changes >>>> the offset of >>>> the members in the structure that breaks the assumption made in the >>>> "struct libcap" >>>> definition in clone3_cap_checkpoint_restore.c.So use the capget and >>>> capset syscall >>>> directly and remove the libcap library dependency like the commit >>>> 663af70aabb7 >>>> ("bpf: selftests: Add helpers to directly use the capget and capset >>>> syscall") does. >>>> >>> >>> NIT: grammar and comma spacing. Please fix those for readability. >>> e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it" >>> Fix others as well. >>> >> >> Thanks, I'll fix it in V2 >> >> >>>> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn> >>>> --- >>>> tools/testing/selftests/clone3/Makefile | 1 - >>>> .../clone3/clone3_cap_checkpoint_restore.c | 60 >>>> +++++++++---------- >>>> 2 files changed, 28 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/clone3/Makefile >>>> b/tools/testing/selftests/clone3/Makefile >>>> index 84832c369a2e..59d26e8da8d2 100644 >>>> --- a/tools/testing/selftests/clone3/Makefile >>>> +++ b/tools/testing/selftests/clone3/Makefile >>>> @@ -1,6 +1,5 @@ >>>> # SPDX-License-Identifier: GPL-2.0 >>>> CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) >>>> -LDLIBS += -lcap >>>> TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ >>>> clone3_cap_checkpoint_restore >>>> diff --git >>>> a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>> b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>> index 3c196fa86c99..111912e2aead 100644 >>>> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>> @@ -15,7 +15,7 @@ >>>> #include <stdio.h> >>>> #include <stdlib.h> >>>> #include <stdbool.h> >>>> -#include <sys/capability.h> >>>> +#include <linux/capability.h> >>>> #include <sys/prctl.h> >>>> #include <sys/syscall.h> >>>> #include <sys/types.h> >>>> @@ -27,6 +27,13 @@ >>>> #include "../kselftest_harness.h" >>>> #include "clone3_selftests.h" >>>> +#ifndef CAP_CHECKPOINT_RESTORE >>>> +#define CAP_CHECKPOINT_RESTORE 40 >>>> +#endif >>>> + >>> >>> Why is this necessary? This is defined in linux/capability.h. >>> >>>> +int capget(cap_user_header_t header, cap_user_data_t data); >>>> +int capset(cap_user_header_t header, const cap_user_data_t data); >>> >>> In general prototypes such as these should be defined in header >>> file. Why are we defining these here? >>> >>> These are defined in sys/capability.h >>> >>> I don't understand this change. You are removing sys/capability.h >>> which requires you to add these defines here. This doesn't >>> sound like a correct solution to me. >>> >> >> I tested it on my machine without libcap-dev installed, the >> /usr/include/linux/capability.h >> >> is on this machine by default. Successfully compiled using #include >> <linux/capability.h> >> >> but not with #include <sys/capability.h>. This patch removes libcap >> library dependencies. >> >> And we don't use any part of sys/capability.h other than these two >> syscalls. So I think that's why it's necessary. > > You are changing the code to not include sys/capability.h > What happens if sys/capability.h along with linux/capability.h > > Do you see problems? > I'm sorry, maybe I wasn't clear enough. When we install the libcap library it will have the following output: test@test:~/work/libcap$ sudo make install | grep capability install -m 0644 include/sys/capability.h /usr/include/sys install -m 0644 include/sys/capability.h /usr/include/sys /usr/share/man/man5 capability.conf.5 \ It installs sys/capability.h in the correct location, but does not install linux/capability.h, so sys/capability.h is bound to the libcap library and they will either exist or disappear together. Now I want to remove the dependency of the test on libcap library so I changed the code that it does not contain sys/capability.h but instead linux/capability.h, so that the test can compile successfully without libcap being installed, these two syscalls are not declared in linux/capability.h(It is sufficient for test use except for these two syscalls) so we need to declare them here. I think that's why the commit 663af70aabb7 ("bpf: selftests: Add helpers to directly use the capget and capset syscall") I refered to does the same thing. As for your question "What happens if sys/capability.h along with linux/capability.h", I haven't found the problem yet, I sincerely hope you can help me improve this code. Thank you very much. > thanks, > -- Shuah
On 10/12/24 02:28, zhouyuhang wrote: > > On 2024/10/11 22:21, Shuah Khan wrote: >> On 10/11/24 00:59, zhouyuhang wrote: >>> >>> On 2024/10/10 23:50, Shuah Khan wrote: >>>> On 10/10/24 06:16, zhouyuhang wrote: >>>>> From: zhouyuhang <zhouyuhang@kylinos.cn> >>>>> >>>>> The libcap commit aca076443591 ("Make cap_t operations thread safe.") added a >>>>> __u8 mutex at the beginning of the struct _cap_struct,it changes the offset of >>>>> the members in the structure that breaks the assumption made in the "struct libcap" >>>>> definition in clone3_cap_checkpoint_restore.c.So use the capget and capset syscall >>>>> directly and remove the libcap library dependency like the commit 663af70aabb7 >>>>> ("bpf: selftests: Add helpers to directly use the capget and capset syscall") does. >>>>> >>>> >>>> NIT: grammar and comma spacing. Please fix those for readability. >>>> e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it" >>>> Fix others as well. >>>> >>> >>> Thanks, I'll fix it in V2 >>> >>> >>>>> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn> >>>>> --- >>>>> tools/testing/selftests/clone3/Makefile | 1 - >>>>> .../clone3/clone3_cap_checkpoint_restore.c | 60 +++++++++---------- >>>>> 2 files changed, 28 insertions(+), 33 deletions(-) >>>>> >>>>> diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile >>>>> index 84832c369a2e..59d26e8da8d2 100644 >>>>> --- a/tools/testing/selftests/clone3/Makefile >>>>> +++ b/tools/testing/selftests/clone3/Makefile >>>>> @@ -1,6 +1,5 @@ >>>>> # SPDX-License-Identifier: GPL-2.0 >>>>> CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) >>>>> -LDLIBS += -lcap >>>>> TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ >>>>> clone3_cap_checkpoint_restore >>>>> diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>> index 3c196fa86c99..111912e2aead 100644 >>>>> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>> @@ -15,7 +15,7 @@ >>>>> #include <stdio.h> >>>>> #include <stdlib.h> >>>>> #include <stdbool.h> >>>>> -#include <sys/capability.h> >>>>> +#include <linux/capability.h> >>>>> #include <sys/prctl.h> >>>>> #include <sys/syscall.h> >>>>> #include <sys/types.h> >>>>> @@ -27,6 +27,13 @@ >>>>> #include "../kselftest_harness.h" >>>>> #include "clone3_selftests.h" >>>>> +#ifndef CAP_CHECKPOINT_RESTORE >>>>> +#define CAP_CHECKPOINT_RESTORE 40 >>>>> +#endif >>>>> + >>>> >>>> Why is this necessary? This is defined in linux/capability.h. >>>> >>>>> +int capget(cap_user_header_t header, cap_user_data_t data); >>>>> +int capset(cap_user_header_t header, const cap_user_data_t data); >>>> >>>> In general prototypes such as these should be defined in header >>>> file. Why are we defining these here? >>>> >>>> These are defined in sys/capability.h >>>> >>>> I don't understand this change. You are removing sys/capability.h >>>> which requires you to add these defines here. This doesn't >>>> sound like a correct solution to me. >>>> >>> >>> I tested it on my machine without libcap-dev installed, the /usr/include/linux/capability.h >>> >>> is on this machine by default. Successfully compiled using #include <linux/capability.h> >>> >>> but not with #include <sys/capability.h>. This patch removes libcap library dependencies. >>> >>> And we don't use any part of sys/capability.h other than these two syscalls. So I think that's why it's necessary. >> >> You are changing the code to not include sys/capability.h >> What happens if sys/capability.h along with linux/capability.h >> >> Do you see problems? >> > > I'm sorry, maybe I wasn't clear enough. > When we install the libcap library it will have the following output: > > test@test:~/work/libcap$ sudo make install | grep capability > install -m 0644 include/sys/capability.h /usr/include/sys > install -m 0644 include/sys/capability.h /usr/include/sys > /usr/share/man/man5 capability.conf.5 \ > > It installs sys/capability.h in the correct location, but does not > > install linux/capability.h, so sys/capability.h is bound to the libcap library It won't install inux/capability.h unless you run "make headers" in the kernel repo. > > and they will either exist or disappear together. Now I want to remove > > the dependency of the test on libcap library so I changed the code that it > > does not contain sys/capability.h but instead linux/capability.h, > > so that the test can compile successfully without libcap being installed, > > these two syscalls are not declared in linux/capability.h(It is sufficient for test use except for these two syscalls) > > so we need to declare them here. I think that's why the commit 663af70aabb7 > > ("bpf: selftests: Add helpers to directly use the capget and capset syscall") I refered to > > does the same thing. As for your question "What happens if sys/capability.h along > > with linux/capability.h", I haven't found the problem yet, I sincerely hope you can > > help me improve this code. Thank you very much. Try this: Run make headers in the kernel repo. Build without making any changes. Then add you changes and add linux/capability.h to include files Tell me what happens. The change you are making isn't correct. Because you don't want to define system calls locally in your source file. thanks, -- Shuah
在 2024/10/15 06:38, Shuah Khan 写道: > On 10/12/24 02:28, zhouyuhang wrote: >> >> On 2024/10/11 22:21, Shuah Khan wrote: >>> On 10/11/24 00:59, zhouyuhang wrote: >>>> >>>> On 2024/10/10 23:50, Shuah Khan wrote: >>>>> On 10/10/24 06:16, zhouyuhang wrote: >>>>>> From: zhouyuhang <zhouyuhang@kylinos.cn> >>>>>> >>>>>> The libcap commit aca076443591 ("Make cap_t operations thread >>>>>> safe.") added a >>>>>> __u8 mutex at the beginning of the struct _cap_struct,it changes >>>>>> the offset of >>>>>> the members in the structure that breaks the assumption made in >>>>>> the "struct libcap" >>>>>> definition in clone3_cap_checkpoint_restore.c.So use the capget >>>>>> and capset syscall >>>>>> directly and remove the libcap library dependency like the commit >>>>>> 663af70aabb7 >>>>>> ("bpf: selftests: Add helpers to directly use the capget and >>>>>> capset syscall") does. >>>>>> >>>>> >>>>> NIT: grammar and comma spacing. Please fix those for readability. >>>>> e.g: Change "struct _cap_struct,it" to "struct _cap_struct, it" >>>>> Fix others as well. >>>>> >>>> >>>> Thanks, I'll fix it in V2 >>>> >>>> >>>>>> Signed-off-by: zhouyuhang <zhouyuhang@kylinos.cn> >>>>>> --- >>>>>> tools/testing/selftests/clone3/Makefile | 1 - >>>>>> .../clone3/clone3_cap_checkpoint_restore.c | 60 >>>>>> +++++++++---------- >>>>>> 2 files changed, 28 insertions(+), 33 deletions(-) >>>>>> >>>>>> diff --git a/tools/testing/selftests/clone3/Makefile >>>>>> b/tools/testing/selftests/clone3/Makefile >>>>>> index 84832c369a2e..59d26e8da8d2 100644 >>>>>> --- a/tools/testing/selftests/clone3/Makefile >>>>>> +++ b/tools/testing/selftests/clone3/Makefile >>>>>> @@ -1,6 +1,5 @@ >>>>>> # SPDX-License-Identifier: GPL-2.0 >>>>>> CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) >>>>>> -LDLIBS += -lcap >>>>>> TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ >>>>>> clone3_cap_checkpoint_restore >>>>>> diff --git >>>>>> a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>>> b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>>> index 3c196fa86c99..111912e2aead 100644 >>>>>> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>>> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c >>>>>> @@ -15,7 +15,7 @@ >>>>>> #include <stdio.h> >>>>>> #include <stdlib.h> >>>>>> #include <stdbool.h> >>>>>> -#include <sys/capability.h> >>>>>> +#include <linux/capability.h> >>>>>> #include <sys/prctl.h> >>>>>> #include <sys/syscall.h> >>>>>> #include <sys/types.h> >>>>>> @@ -27,6 +27,13 @@ >>>>>> #include "../kselftest_harness.h" >>>>>> #include "clone3_selftests.h" >>>>>> +#ifndef CAP_CHECKPOINT_RESTORE >>>>>> +#define CAP_CHECKPOINT_RESTORE 40 >>>>>> +#endif >>>>>> + >>>>> >>>>> Why is this necessary? This is defined in linux/capability.h. Sorry for not noticing this before. This is to be compatible with some older versions of linux/capability.h that do not define this macro. >>>>> >>>>>> +int capget(cap_user_header_t header, cap_user_data_t data); >>>>>> +int capset(cap_user_header_t header, const cap_user_data_t data); >>>>> >>>>> In general prototypes such as these should be defined in header >>>>> file. Why are we defining these here? >>>>> >>>>> These are defined in sys/capability.h >>>>> >>>>> I don't understand this change. You are removing sys/capability.h >>>>> which requires you to add these defines here. This doesn't >>>>> sound like a correct solution to me. >>>>> >>>> >>>> I tested it on my machine without libcap-dev installed, the >>>> /usr/include/linux/capability.h >>>> >>>> is on this machine by default. Successfully compiled using #include >>>> <linux/capability.h> >>>> >>>> but not with #include <sys/capability.h>. This patch removes libcap >>>> library dependencies. >>>> >>>> And we don't use any part of sys/capability.h other than these two >>>> syscalls. So I think that's why it's necessary. >>> >>> You are changing the code to not include sys/capability.h >>> What happens if sys/capability.h along with linux/capability.h >>> >>> Do you see problems? >>> >> >> I'm sorry, maybe I wasn't clear enough. >> When we install the libcap library it will have the following output: >> >> test@test:~/work/libcap$ sudo make install | grep capability >> install -m 0644 include/sys/capability.h /usr/include/sys >> install -m 0644 include/sys/capability.h /usr/include/sys >> /usr/share/man/man5 capability.conf.5 \ >> >> It installs sys/capability.h in the correct location, but does not >> >> install linux/capability.h, so sys/capability.h is bound to the >> libcap library > > It won't install inux/capability.h unless you run "make headers" in > the kernel repo. > >> >> and they will either exist or disappear together. Now I want to remove >> >> the dependency of the test on libcap library so I changed the code >> that it >> >> does not contain sys/capability.h but instead linux/capability.h, >> >> so that the test can compile successfully without libcap being >> installed, >> >> these two syscalls are not declared in linux/capability.h(It is >> sufficient for test use except for these two syscalls) >> >> so we need to declare them here. I think that's why the commit >> 663af70aabb7 >> >> ("bpf: selftests: Add helpers to directly use the capget and capset >> syscall") I refered to >> >> does the same thing. As for your question "What happens if >> sys/capability.h along >> >> with linux/capability.h", I haven't found the problem yet, I >> sincerely hope you can >> >> help me improve this code. Thank you very much. > > Try this: > > Run make headers in the kernel repo. > Build without making any changes. > Then add you changes and add linux/capability.h to include files > > Tell me what happens. > > The change you are making isn't correct. Because you don't want to > define system calls locally in your source file. > Thanks, I see. Maybe I should move them to a new header file and resend a patch. > thanks, > -- Shuah
On 10/15/24 03:00, zhouyuhang wrote: > [snip] for clarity. >>>>>> Why is this necessary? This is defined in linux/capability.h. > > Sorry for not noticing this before. > This is to be compatible with some older versions of linux/capability.h that do not define this macro. > >>>>>> >>>>>>> +int capget(cap_user_header_t header, cap_user_data_t data); >>>>>>> +int capset(cap_user_header_t header, const cap_user_data_t data); >>>>>> >>>>>> In general prototypes such as these should be defined in header >>>>>> file. Why are we defining these here? >>>>>> >>>>>> These are defined in sys/capability.h >>>>>> >>>>>> I don't understand this change. You are removing sys/capability.h >>>>>> which requires you to add these defines here. This doesn't >>>>>> sound like a correct solution to me. >>>>>> >>>>> >>>>> I tested it on my machine without libcap-dev installed, the /usr/include/linux/capability.h >>>>> >>>>> is on this machine by default. Successfully compiled using #include <linux/capability.h> >>>>> >>>>> but not with #include <sys/capability.h>. This patch removes libcap library dependencies. >>>>> >>>>> And we don't use any part of sys/capability.h other than these two syscalls. So I think that's why it's necessary. >>>> >>>> You are changing the code to not include sys/capability.h >>>> What happens if sys/capability.h along with linux/capability.h >>>> >>>> Do you see problems? >>>> >>> >>> I'm sorry, maybe I wasn't clear enough. >>> When we install the libcap library it will have the following output: >>> >>> test@test:~/work/libcap$ sudo make install | grep capability >>> install -m 0644 include/sys/capability.h /usr/include/sys >>> install -m 0644 include/sys/capability.h /usr/include/sys >>> /usr/share/man/man5 capability.conf.5 \ >>> >>> It installs sys/capability.h in the correct location, but does not >>> >>> install linux/capability.h, so sys/capability.h is bound to the libcap library >> >> It won't install inux/capability.h unless you run "make headers" in >> the kernel repo. >> >>> >>> and they will either exist or disappear together. Now I want to remove >>> >>> the dependency of the test on libcap library so I changed the code that it >>> >>> does not contain sys/capability.h but instead linux/capability.h, >>> >>> so that the test can compile successfully without libcap being installed, >>> >>> these two syscalls are not declared in linux/capability.h(It is sufficient for test use except for these two syscalls) >>> >>> so we need to declare them here. I think that's why the commit 663af70aabb7 >>> >>> ("bpf: selftests: Add helpers to directly use the capget and capset syscall") I refered to >>> >>> does the same thing. As for your question "What happens if sys/capability.h along >>> >>> with linux/capability.h", I haven't found the problem yet, I sincerely hope you can >>> >>> help me improve this code. Thank you very much. >> >> Try this: >> >> Run make headers in the kernel repo. >> Build without making any changes. >> Then add you changes and add linux/capability.h to include files >> >> Tell me what happens. Try the above first. >> >> The change you are making isn't correct. Because you don't want to >> define system calls locally in your source file. >> > > Thanks, I see. > Maybe I should move them to a new header file and resend a patch. No. Please see above. I would rather not see these defined at all locally. thanks, -- Shuah
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile index 84832c369a2e..59d26e8da8d2 100644 --- a/tools/testing/selftests/clone3/Makefile +++ b/tools/testing/selftests/clone3/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) -LDLIBS += -lcap TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \ clone3_cap_checkpoint_restore diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c index 3c196fa86c99..111912e2aead 100644 --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c @@ -15,7 +15,7 @@ #include <stdio.h> #include <stdlib.h> #include <stdbool.h> -#include <sys/capability.h> +#include <linux/capability.h> #include <sys/prctl.h> #include <sys/syscall.h> #include <sys/types.h> @@ -27,6 +27,13 @@ #include "../kselftest_harness.h" #include "clone3_selftests.h" +#ifndef CAP_CHECKPOINT_RESTORE +#define CAP_CHECKPOINT_RESTORE 40 +#endif + +int capget(cap_user_header_t header, cap_user_data_t data); +int capset(cap_user_header_t header, const cap_user_data_t data); + static void child_exit(int ret) { fflush(stdout); @@ -87,47 +94,36 @@ static int test_clone3_set_tid(struct __test_metadata *_metadata, return ret; } -struct libcap { - struct __user_cap_header_struct hdr; - struct __user_cap_data_struct data[2]; -}; - static int set_capability(void) { - cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID }; - struct libcap *cap; - int ret = -1; - cap_t caps; - - caps = cap_get_proc(); - if (!caps) { - perror("cap_get_proc"); + struct __user_cap_data_struct data[2]; + struct __user_cap_header_struct hdr = { + .version = _LINUX_CAPABILITY_VERSION_3, + }; + __u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID; + __u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32); + int ret; + + ret = capget(&hdr, data); + if (ret) { + perror("capget"); return -1; } /* Drop all capabilities */ - if (cap_clear(caps)) { - perror("cap_clear"); - goto out; - } + memset(&data, 0, sizeof(data)); - cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET); - cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET); + data[0].effective |= cap0; + data[0].permitted |= cap0; - cap = (struct libcap *) caps; + data[1].effective |= cap1; + data[1].permitted |= cap1; - /* 40 -> CAP_CHECKPOINT_RESTORE */ - cap->data[1].effective |= 1 << (40 - 32); - cap->data[1].permitted |= 1 << (40 - 32); - - if (cap_set_proc(caps)) { - perror("cap_set_proc"); - goto out; + ret = capset(&hdr, data); + if (ret) { + perror("capset"); + return -1; } - ret = 0; -out: - if (cap_free(caps)) - perror("cap_free"); return ret; }