linux-kernel - Re: [PATCH] selftests/mount_setattr: fix redefine struct mount

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y+vzB1OTXr+zTCV7@do-x1extreme>
Date:   Tue, 14 Feb 2023 14:45:59 -0600
From:   Seth Forshee <sforshee@...nel.org>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     shuah@...nel.org, brauner@...nel.org,
        linux-fsdevel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selftests/mount_setattr: fix redefine struct mount_attr
 build error

On Tue, Feb 14, 2023 at 10:10:00AM -0700, Shuah Khan wrote:
> On 2/13/23 16:50, Seth Forshee wrote:
> > On Mon, Feb 13, 2023 at 11:31:49AM -0700, Shuah Khan wrote:
> > > Fix the following build error due to redefining struct mount_attr by
> > > removing duplicate define from mount_setattr_test.c
> > > 
> > > gcc -g -isystem .../tools/testing/selftests/../../../usr/include -Wall -O2 -pthread     mount_setattr_test.c  -o .../tools/testing/selftests/mount_setattr/mount_setattr_test
> > > mount_setattr_test.c:107:8: error: redefinition of ‘struct mount_attr’
> > >    107 | struct mount_attr {
> > >        |        ^~~~~~~~~~
> > > In file included from /usr/include/x86_64-linux-gnu/sys/mount.h:32,
> > >                   from mount_setattr_test.c:10:
> > > .../usr/include/linux/mount.h:129:8: note: originally defined here
> > >    129 | struct mount_attr {
> > >        |        ^~~~~~~~~~
> > > make: *** [../lib.mk:145: .../tools/testing/selftests/mount_setattr/mount_setattr_test] Error 1
> > > 
> > > Signed-off-by: Shuah Khan <skhan@...uxfoundation.org>
> > > ---
> > >   tools/testing/selftests/mount_setattr/mount_setattr_test.c | 7 -------
> > >   1 file changed, 7 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
> > > index 8c5fea68ae67..582669ca38e9 100644
> > > --- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
> > > +++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
> > > @@ -103,13 +103,6 @@
> > >   	#else
> > >   		#define __NR_mount_setattr 442
> > >   	#endif
> > > -
> > > -struct mount_attr {
> > > -	__u64 attr_set;
> > > -	__u64 attr_clr;
> > > -	__u64 propagation;
> > > -	__u64 userns_fd;
> > > -};
> > >   #endif
> > 
> > The difficulty with this is that whether or not you need this definition
> > depends on your system headers. My laptop doesn't have definitions for
> > either __NR_mount_setattr or struct mount_attr, so for me the build
> > works without this patch but fails with it.
> > 
> 
> The header search looks up system headers followed by installed headers in
> the repo (both in-tree and out-of-tree builds). kselftest builds do depend
> on headers_install. Did you building after running headers_install?

I wasn't aware they depend on headers_install. Why doesn't
Documentation/dev-tools/kselftest.rst mention this in the section that
describes how to run tests?

It seems what I really need to fix the build is to include
linux/mount.h, which works for me with or without headers_install,
because I have the struct in /usr/include/linux/mount.h. And I suppose
the makefile should use KHDR_INCLUDES. So maybe the changes below should
also be included.

However I know Christian has said that there are challenges with
including the mount headers. He wrote this test, so I'd like to hear his
thoughts about adding the include. He's on vacation this week though.

> > I suppose we could fix this universally by using a different name for
> > the struct in the test, e.g.:
> > 
> 
> This is not a good way to for a couple of reasons. This masks any problems
> due to incompatibility between these defines.

Agreed that this isn't ideal.

Thanks,
Seth


diff --git a/tools/testing/selftests/mount_setattr/Makefile b/tools/testing/selftests/mount_setattr/Makefile
index 2250f7dcb81e..fde72df01b11 100644
--- a/tools/testing/selftests/mount_setattr/Makefile
+++ b/tools/testing/selftests/mount_setattr/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for mount selftests.
-CFLAGS = -g -I../../../../usr/include/ -Wall -O2 -pthread
+CFLAGS = -g $(KHDR_INCLUDES) -Wall -O2 -pthread
 
 TEST_GEN_FILES += mount_setattr_test
 
diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index 8c5fea68ae67..969647228817 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -18,6 +18,7 @@
 #include <grp.h>
 #include <stdbool.h>
 #include <stdarg.h>
+#include <linux/mount.h>
 
 #include "../kselftest_harness.h"