Re: [PATCH v3] selftests: Fix seccomp to support relocatable build (O=objdir)

[Shuah Khan]

On 3/13/20 5:18 PM, Kees Cook wrote:
> On Fri, Mar 13, 2020 at 03:24:04PM -0600, Shuah Khan wrote:
> > Fix seccomp relocatable builds. This is a simple fix to use the right
> > lib.mk variable TEST_GEN_PROGS with dependency on kselftest_harness.h
> > header, and defining LDFLAGS for pthread lib.
> >
> > Removes custom clean rule which is no longer necessary with the use of
> > TEST_GEN_PROGS.
> >
> > Uses $(OUTPUT) defined in lib.mk to handle build relocation.
> >
> > The following use-cases work with this change:
> >
> > In seccomp directory:
> > make all and make clean
> >
> >  From top level from main Makefile:
> > make kselftest-install O=objdir ARCH=arm64 HOSTCC=gcc \
> >   CROSS_COMPILE=aarch64-linux-gnu- TARGETS=seccomp
> >
> > Signed-off-by: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Changes since v2:
> > -- Using TEST_GEN_PROGS is sufficient to generate objects.
> >     Addresses review comments from Kees Cook.
> >
> >   tools/testing/selftests/seccomp/Makefile | 18 ++++++++----------
> >   1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/seccomp/Makefile b/tools/testing/selftests/seccomp/Makefile
> > index 1760b3e39730..a0388fd2c3f2 100644
> > --- a/tools/testing/selftests/seccomp/Makefile
> > +++ b/tools/testing/selftests/seccomp/Makefile
> > @@ -1,17 +1,15 @@
> >   # SPDX-License-Identifier: GPL-2.0
> > -all:
> > -
> > -include ../lib.mk
> > +CFLAGS += -Wl,-no-as-needed -Wall
> > +LDFLAGS += -lpthread
> >   
> >   .PHONY: all clean
>
> Isn't this line redundant to ../lib.mk's?
>
> >   
> > -BINARIES := seccomp_bpf seccomp_benchmark
> > -CFLAGS += -Wl,-no-as-needed -Wall
> > +include ../lib.mk
> > +
> > +# OUTPUT set by lib.mk
> > +TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf $(OUTPUT)/seccomp_benchmark
> >   
> > -seccomp_bpf: seccomp_bpf.c ../kselftest_harness.h
> > -	$(CC) $(CFLAGS) $(LDFLAGS) $< -lpthread -o $@
> > +$(TEST_GEN_PROGS): ../kselftest_harness.h
> >   
> > -TEST_PROGS += $(BINARIES)
> > -EXTRA_CLEAN := $(BINARIES)
> > +all: $(TEST_GEN_PROGS)
>
> And isn't this one too?

make in seccomp directory won't work. lib.mk won't build it.
One reason why I wanted to clearly call this out as CUSTOM
program.

But it does make sense to reduce additional EXTRA_CLEAN by
just using TEST_GEN_PROGS.
> I think if those are removed it should all still work? Regardless:
diff Makefile
diff --git a/tools/testing/selftests/seccomp/Makefile 
b/tools/testing/selftests/seccomp/Makefile
index a0388fd2c3f2..d3d256654cb1 100644
--- a/tools/testing/selftests/seccomp/Makefile
+++ b/tools/testing/selftests/seccomp/Makefile
@@ -2,7 +2,7 @@
 CFLAGS += -Wl,-no-as-needed -Wall
 LDFLAGS += -lpthread

-.PHONY: all clean
+#.PHONY: all clean

 include ../lib.mk

@@ -11,5 +11,5 @@ TEST_GEN_PROGS := $(OUTPUT)/seccomp_bpf 
$(OUTPUT)/seccomp_benchmark

 $(TEST_GEN_PROGS): ../kselftest_harness.h

-all: $(TEST_GEN_PROGS)
+#$all: $(TEST_GEN_PROGS)

With this:

make
make: Nothing to be done for 'all'.

> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

I am addressing your comment about other tests that de[end on
kselftest_harness.h. I have a few patches ready to be sent.

thanks,
-- Shuah