| Message ID | 20240808145639.GA20510@asgard.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests/alsa/Makefile: fix relative rpath usage | expand |
On 8/8/24 08:57, Eugene Syromiatnikov wrote: > The relative RPATH ("./") supplied to linker options in CFLAGS is resolved > relative to current working directory and not the executable directory, > which will lead in incorrect resolution when the test executables are run > from elsewhere. Changing it to $ORIGIN makes it resolve relative > to the directory in which the executables reside, which is supposedly > the desired behaviour. > > Discovered by the check-rpaths script[1][2] that checks for insecure > RPATH/RUNPATH[3], such as relative directories, during an attempt > to package BPF selftests for later use in CI: > > ERROR 0004: file '/usr/libexec/kselftests/bpf/urandom_read' contains an insecure runpath '.' in [.] > > [1] https://github.com/rpm-software-management/rpm/blob/master/scripts/check-rpaths > [2] https://github.com/rpm-software-management/rpm/blob/master/scripts/check-rpaths-worker > [3] https://cwe.mitre.org/data/definitions/426.html > > Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> > --- > tools/testing/selftests/alsa/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/alsa/Makefile b/tools/testing/selftests/alsa/Makefile > index c1ce39874e2b..0f204da9ea8e 100644 > --- a/tools/testing/selftests/alsa/Makefile > +++ b/tools/testing/selftests/alsa/Makefile > @@ -6,7 +6,7 @@ LDLIBS += $(shell pkg-config --libs alsa) > ifeq ($(LDLIBS),) > LDLIBS += -lasound > endif > -CFLAGS += -L$(OUTPUT) -Wl,-rpath=./ > +CFLAGS += -L$(OUTPUT) -Wl,-rpath=\$$ORIGIN/ > > LDLIBS+=-lpthread > Wouldn't make sense to fix fix this in selftests main Makefile instead of changing the all the test makefiles Same comment on all other files. It would be easier to send these as series - please mentioned the tests run as well after this change. thanks, -- Shuah
On Thu, Aug 08, 2024 at 02:20:21PM -0600, Shuah Khan wrote: > Wouldn't make sense to fix fix this in selftests main Makefile > instead of changing the all the test makefiles As of now, the usage of rpath is localised, so it is relatively easy to evaluate the effect/prudence of such a change; I am not so confident in imposing rpath on all of the selftests (and, if doing so, I would rather opt for runpath, to leave out an ability to override the search path via LD_LIBRARY_PATH, if such need arises); in that case it is possibly also worth to add -L$(OUTPUT) to the CFLAGS as well, as the compile-time counterpart. But, again, I was trying to avoid the task of evaluating the possible side effects of such a change, considering the variability in environments and setups selftests are run. > Same comment on all other files. > It would be easier to send these as series I hesitated to do so due to the fact that different selftests are seemingly maintained by different people. > please mentioned the tests run as well after this change. I have checked the ldd output after the change remained the same (and that ldd is able to find the libraries used when run outside the directory the tests reside in) and did a cursory check of the results of the run of the affected tests (but not so sure about the BPF selftests, as they don't compile as-is due to numerous "incompatible pointer types" warnings that are forced into errors by -Werror and the fact that it hanged the machine I tried to run them on). > thanks, > -- Shuah > >
On 8/8/24 19:00, Eugene Syromiatnikov wrote: > On Thu, Aug 08, 2024 at 02:20:21PM -0600, Shuah Khan wrote: >> Wouldn't make sense to fix fix this in selftests main Makefile >> instead of changing the all the test makefiles > > As of now, the usage of rpath is localised, so it is relatively easy > to evaluate the effect/prudence of such a change; I am not so confident > in imposing rpath on all of the selftests (and, if doing so, I would > rather opt for runpath, to leave out an ability to override the search > path via LD_LIBRARY_PATH, if such need arises); in that case it is possibly > also worth to add -L$(OUTPUT) to the CFLAGS as well, as the compile-time > counterpart. But, again, I was trying to avoid the task of evaluating > the possible side effects of such a change, considering the variability > in environments and setups selftests are run. Okay. > >> Same comment on all other files. > >> It would be easier to send these as series > > I hesitated to do so due to the fact that different selftests are seemingly > maintained by different people. You can cc everybody on the cover-letter explaining the change and the individual patches can be sent selectively. This is a kind of change it would be good to go as a series so it will be easier for reviewers. I had to comment on all 3 patches you sent - instead I could have sent one reply to the cover letter. It makes it so much easier for people to follow the discussion and add to it. > >> please mentioned the tests run as well after this change. > > I have checked the ldd output after the change remained the same (and that ldd > is able to find the libraries used when run outside the directory the tests > reside in) and did a cursory check of the results of the run of the affected > tests Please mention that then in the change log. I applied this patch and ran alsa test without any issues. You could do the same with: make kselftest TARGETS=alsa (but not so sure about the BPF selftests, as they don't compile as-is > due to numerous "incompatible pointer types" warnings that are forced > into errors by -Werror and the fact that it hanged the machine I tried > to run them on). > I see a bpf patch from you in the inbox - if you mention the issues bpf people might be able to help you. I am not replying to your other patches. Take these as comments on others as well. thanks, -- Shuah
On Fri, 9 Aug 2024 at 19:01, Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 8/8/24 19:00, Eugene Syromiatnikov wrote: > > On Thu, Aug 08, 2024 at 02:20:21PM -0600, Shuah Khan wrote: > >> Wouldn't make sense to fix fix this in selftests main Makefile > >> instead of changing the all the test makefiles > > > > As of now, the usage of rpath is localised, so it is relatively easy > > to evaluate the effect/prudence of such a change; I am not so confident > > in imposing rpath on all of the selftests (and, if doing so, I would > > rather opt for runpath, to leave out an ability to override the search > > path via LD_LIBRARY_PATH, if such need arises); in that case it is possibly > > also worth to add -L$(OUTPUT) to the CFLAGS as well, as the compile-time > > counterpart. But, again, I was trying to avoid the task of evaluating > > the possible side effects of such a change, considering the variability > > in environments and setups selftests are run. > > Okay. > > > > >> Same comment on all other files. > > > >> It would be easier to send these as series > > > > I hesitated to do so due to the fact that different selftests are seemingly > > maintained by different people. > > You can cc everybody on the cover-letter explaining the change > and the individual patches can be sent selectively. > > This is a kind of change it would be good to go as a series so > it will be easier for reviewers. I see, thank you for the explanation. Right now I am working on the variant of the patch that consolidates the -L/-rpath flags in lib.mk, do you think it will be of use to have some opt-in/opt-out mechanism, or just impose them unconditionally, similarly to -D_GNU_SOURCE? So far I don't see any issues with either building or running the tests, but I can imagine it might be necessary to avoid such flags in some cases. > I had to comment on all 3 patches you sent - instead I could have > sent one reply to the cover letter. It makes it so much easier for > people to follow the discussion and add to it. My apologies. > >> please mentioned the tests run as well after this change. > > > > I have checked the ldd output after the change remained the same (and that ldd > > is able to find the libraries used when run outside the directory the tests > > reside in) and did a cursory check of the results of the run of the affected > > tests > > Please mention that then in the change log. > > I applied this patch and ran alsa test without any issues. You > could do the same with: > > make kselftest TARGETS=alsa Thanks, will do. > (but not so sure about the BPF selftests, as they don't compile as-is > > due to numerous "incompatible pointer types" warnings that are forced > > into errors by -Werror and the fact that it hanged the machine I tried > > to run them on). > > > > I see a bpf patch from you in the inbox - if you mention the issues bpf > people might be able to help you. Right, I am in the process of condensing my issues into patches or at least useful bug reports. > I am not replying to your other patches. Take these as comments on others > as well. > > thanks, > -- Shuah
diff --git a/tools/testing/selftests/alsa/Makefile b/tools/testing/selftests/alsa/Makefile index c1ce39874e2b..0f204da9ea8e 100644 --- a/tools/testing/selftests/alsa/Makefile +++ b/tools/testing/selftests/alsa/Makefile @@ -6,7 +6,7 @@ LDLIBS += $(shell pkg-config --libs alsa) ifeq ($(LDLIBS),) LDLIBS += -lasound endif -CFLAGS += -L$(OUTPUT) -Wl,-rpath=./ +CFLAGS += -L$(OUTPUT) -Wl,-rpath=\$$ORIGIN/ LDLIBS+=-lpthread
The relative RPATH ("./") supplied to linker options in CFLAGS is resolved relative to current working directory and not the executable directory, which will lead in incorrect resolution when the test executables are run from elsewhere. Changing it to $ORIGIN makes it resolve relative to the directory in which the executables reside, which is supposedly the desired behaviour. Discovered by the check-rpaths script[1][2] that checks for insecure RPATH/RUNPATH[3], such as relative directories, during an attempt to package BPF selftests for later use in CI: ERROR 0004: file '/usr/libexec/kselftests/bpf/urandom_read' contains an insecure runpath '.' in [.] [1] https://github.com/rpm-software-management/rpm/blob/master/scripts/check-rpaths [2] https://github.com/rpm-software-management/rpm/blob/master/scripts/check-rpaths-worker [3] https://cwe.mitre.org/data/definitions/426.html Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com> --- tools/testing/selftests/alsa/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)