Re: [PATCH] selftests: clone3: Use the capget and capset syscall directly

[zhouyuhang]

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@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >   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