[PATCH 0/2] selinux: Do not include <linux/*.h> from host programs (+ extra clean-up)

[PATCH 0/2] selinux: Do not include <linux/*.h> from host programs (+ extra clean-up)

[Masahiro Yamada]

This is a small patch set to refactor selinux Makefile code.
(1/2 is the main motivation here)

1/2 is intended to replace the following shenonigans:
  - [PATCH 02/12] kbuild: add header_install dependency to scripts
  - [PATCH 06/12] selinux/genheaders: include bitsperlong and posix_types headers
  - [PATCH 07/12] selinux/mdp: include bitsperlong and posix_types headers
    https://lore.kernel.org/linux-kbuild/20240807-macos-build-support-v1-7-4cd1ded85694@samsung.com/T/#m1231a27dc83f86c283c4abf480c3d3312955fbb7

2/2 is just an extra work while I am here.



Masahiro Yamada (2):
  selinux: do not include <linux/*.h> headers from host programs
  selinux: move genheaders to security/selinux/

 scripts/remove-stale-files                    |  3 +++
 scripts/selinux/Makefile                      |  2 +-
 scripts/selinux/genheaders/.gitignore         |  2 --
 scripts/selinux/genheaders/Makefile           |  5 -----
 scripts/selinux/mdp/Makefile                  |  2 +-
 scripts/selinux/mdp/mdp.c                     |  4 ----
 security/selinux/.gitignore                   |  1 +
 security/selinux/Makefile                     |  7 +++++--
 .../selinux}/genheaders.c                     |  3 ---
 security/selinux/include/classmap.h           | 19 ++++++++++++-------
 .../selinux/include/initial_sid_to_string.h   |  2 --
 11 files changed, 23 insertions(+), 27 deletions(-)
 delete mode 100644 scripts/selinux/genheaders/.gitignore
 delete mode 100644 scripts/selinux/genheaders/Makefile
 rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (97%)

-- 
2.43.0

[PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs

[Masahiro Yamada]

Commit bfc5e3a6af39 ("selinux: use the kernel headers when building
scripts/selinux") is not the right thing to do.

It is clear from the warning in include/uapi/linux/types.h:

  #ifndef __EXPORTED_HEADERS__
  #warning "Attempt to use kernel headers from user space, see https://kernelnewbies.org/KernelHeaders"
  #endif /* __EXPORTED_HEADERS__ */

If you are inclined to define __EXPORTED_HEADERS__, you are likely doing
wrong.

Adding the comment:

  /* NOTE: we really do want to use the kernel headers here */

does not justify the hack in any way.

Currently, <linux/*.h> headers are included for the following purposes:

 - <linux/capability.h> is included to check CAP_LAST_CAP
 - <linux/socket.h> in included to check PF_MAX

We can skip these checks when building host programs, as they will
be eventually tested when building the kernel space.

I got rid of <linux/stddef.h> from initial_sid_to_string.h because
it is likely that NULL is already defined. If you insist on making
it self-contained, you can add the following:

  #ifdef __KERNEL__
  #include <linux/stddef.h>
  #else
  #include <stddef.h>
  #endif

scripts/selinux/mdp/mdp.c still includes <linux/kconfig.h>, which is
also discouraged and should be fixed by a follow-up refactoring.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/selinux/genheaders/Makefile           |  4 +---
 scripts/selinux/genheaders/genheaders.c       |  3 ---
 scripts/selinux/mdp/Makefile                  |  2 +-
 scripts/selinux/mdp/mdp.c                     |  4 ----
 security/selinux/include/classmap.h           | 19 ++++++++++++-------
 .../selinux/include/initial_sid_to_string.h   |  2 --
 6 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/scripts/selinux/genheaders/Makefile b/scripts/selinux/genheaders/Makefile
index 1faf7f07e8db..866f60e78882 100644
--- a/scripts/selinux/genheaders/Makefile
+++ b/scripts/selinux/genheaders/Makefile
@@ -1,5 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 hostprogs-always-y += genheaders
-HOST_EXTRACFLAGS += \
-	-I$(srctree)/include/uapi -I$(srctree)/include \
-	-I$(srctree)/security/selinux/include
+HOST_EXTRACFLAGS += -I$(srctree)/security/selinux/include
diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
index 15520806889e..3834d7eb0af6 100644
--- a/scripts/selinux/genheaders/genheaders.c
+++ b/scripts/selinux/genheaders/genheaders.c
@@ -1,8 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
 
-/* NOTE: we really do want to use the kernel headers here */
-#define __EXPORTED_HEADERS__
-
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
diff --git a/scripts/selinux/mdp/Makefile b/scripts/selinux/mdp/Makefile
index d61058ddd15c..673782e3212f 100644
--- a/scripts/selinux/mdp/Makefile
+++ b/scripts/selinux/mdp/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 hostprogs-always-y += mdp
 HOST_EXTRACFLAGS += \
-	-I$(srctree)/include/uapi -I$(srctree)/include \
+	-I$(srctree)/include \
 	-I$(srctree)/security/selinux/include -I$(objtree)/include
 
 clean-files	:= policy.* file_contexts
diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c
index 1415604c3d24..52365921c043 100644
--- a/scripts/selinux/mdp/mdp.c
+++ b/scripts/selinux/mdp/mdp.c
@@ -11,10 +11,6 @@
  * Authors: Serge E. Hallyn <serue@us.ibm.com>
  */
 
-
-/* NOTE: we really do want to use the kernel headers here */
-#define __EXPORTED_HEADERS__
-
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 7229c9bf6c27..518209e1beb0 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -1,8 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
-#include <linux/capability.h>
-#include <linux/socket.h>
-
 #define COMMON_FILE_SOCK_PERMS                                            \
 	"ioctl", "read", "write", "create", "getattr", "setattr", "lock", \
 		"relabelfrom", "relabelto", "append", "map"
@@ -36,10 +33,6 @@
 	"mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend", \
 		"audit_read", "perfmon", "bpf", "checkpoint_restore"
 
-#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
-#error New capability defined, please update COMMON_CAP2_PERMS.
-#endif
-
 /*
  * Note: The name for any socket class should be suffixed by "socket",
  *	 and doesn't contain more than one substr of "socket".
@@ -181,6 +174,18 @@ const struct security_class_mapping secclass_map[] = {
 	{ NULL }
 };
 
+#ifdef __KERNEL__ /* avoid this check when building host programs */
+
+#include <linux/capability.h>
+
+#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
+#error New capability defined, please update COMMON_CAP2_PERMS.
+#endif
+
+#include <linux/socket.h>
+
 #if PF_MAX > 46
 #error New address family defined, please update secclass_map.
 #endif
+
+#endif /* __KERNEL__ */
diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
index 99b353b2abb4..f683a78b21fd 100644
--- a/security/selinux/include/initial_sid_to_string.h
+++ b/security/selinux/include/initial_sid_to_string.h
@@ -1,7 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
-#include <linux/stddef.h>
-
 static const char *const initial_sid_to_string[] = {
 	NULL, /* zero placeholder, not used */
 	"kernel", /* kernel / SECINITSID_KERNEL */
-- 
2.43.0

[PATCH 2/2] selinux: move genheaders to security/selinux/

[Masahiro Yamada]

This tool is only used in security/selinux/Makefile.

There is no reason to keep it under scripts/.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/remove-stale-files                                 | 3 +++
 scripts/selinux/Makefile                                   | 2 +-
 scripts/selinux/genheaders/.gitignore                      | 2 --
 scripts/selinux/genheaders/Makefile                        | 3 ---
 security/selinux/.gitignore                                | 1 +
 security/selinux/Makefile                                  | 7 +++++--
 .../selinux/genheaders => security/selinux}/genheaders.c   | 0
 7 files changed, 10 insertions(+), 8 deletions(-)
 delete mode 100644 scripts/selinux/genheaders/.gitignore
 delete mode 100644 scripts/selinux/genheaders/Makefile
 rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)

diff --git a/scripts/remove-stale-files b/scripts/remove-stale-files
index f38d26b78c2a..4e7d25668a98 100755
--- a/scripts/remove-stale-files
+++ b/scripts/remove-stale-files
@@ -20,4 +20,7 @@ set -e
 # yard. Stale files stay in this file for a while (for some release cycles?),
 # then will be really dead and removed from the code base entirely.
 
+# moved to security/selinux/genheaders
+rm -f scripts/selinux/genheaders/genheaders
+
 rm -f *.spec
diff --git a/scripts/selinux/Makefile b/scripts/selinux/Makefile
index 59494e14989b..4b1308fa5732 100644
--- a/scripts/selinux/Makefile
+++ b/scripts/selinux/Makefile
@@ -1,2 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-subdir-y := mdp genheaders
+subdir-y := mdp
diff --git a/scripts/selinux/genheaders/.gitignore b/scripts/selinux/genheaders/.gitignore
deleted file mode 100644
index 5fcadd307908..000000000000
--- a/scripts/selinux/genheaders/.gitignore
+++ /dev/null
@@ -1,2 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-only
-genheaders
diff --git a/scripts/selinux/genheaders/Makefile b/scripts/selinux/genheaders/Makefile
deleted file mode 100644
index 866f60e78882..000000000000
--- a/scripts/selinux/genheaders/Makefile
+++ /dev/null
@@ -1,3 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0
-hostprogs-always-y += genheaders
-HOST_EXTRACFLAGS += -I$(srctree)/security/selinux/include
diff --git a/security/selinux/.gitignore b/security/selinux/.gitignore
index 168fae13ca5a..01c0df8ab009 100644
--- a/security/selinux/.gitignore
+++ b/security/selinux/.gitignore
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 av_permissions.h
 flask.h
+/genheaders
diff --git a/security/selinux/Makefile b/security/selinux/Makefile
index c47519ed8156..86f0575f670d 100644
--- a/security/selinux/Makefile
+++ b/security/selinux/Makefile
@@ -36,7 +36,10 @@ quiet_cmd_genhdrs = GEN     $(addprefix $(obj)/,$(genhdrs))
 # see the note above, replace the $targets and 'flask.h' rule with the lines
 # below:
 #  targets += $(genhdrs)
-#  $(addprefix $(obj)/,$(genhdrs)) &: scripts/selinux/...
+#  $(addprefix $(obj)/,$(genhdrs)) &: $(obj)/genheaders FORCE
 targets += flask.h
-$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE
+$(obj)/flask.h: $(obj)/genheaders FORCE
 	$(call if_changed,genhdrs)
+
+hostprogs := genheaders
+HOST_EXTRACFLAGS += -I$(srctree)/security/selinux/include
diff --git a/scripts/selinux/genheaders/genheaders.c b/security/selinux/genheaders.c
similarity index 100%
rename from scripts/selinux/genheaders/genheaders.c
rename to security/selinux/genheaders.c
-- 
2.43.0

Re: [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs

[Paul Moore]

On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> 
> Commit bfc5e3a6af39 ("selinux: use the kernel headers when building
> scripts/selinux") is not the right thing to do.
> 
> It is clear from the warning in include/uapi/linux/types.h:
> 
>   #ifndef __EXPORTED_HEADERS__
>   #warning "Attempt to use kernel headers from user space, see https://kernelnewbies.org/KernelHeaders"
>   #endif /* __EXPORTED_HEADERS__ */
> 
> If you are inclined to define __EXPORTED_HEADERS__, you are likely doing
> wrong.
> 
> Adding the comment:
> 
>   /* NOTE: we really do want to use the kernel headers here */
> 
> does not justify the hack in any way.
> 
> Currently, <linux/*.h> headers are included for the following purposes:
> 
>  - <linux/capability.h> is included to check CAP_LAST_CAP
>  - <linux/socket.h> in included to check PF_MAX
> 
> We can skip these checks when building host programs, as they will
> be eventually tested when building the kernel space.
> 
> I got rid of <linux/stddef.h> from initial_sid_to_string.h because
> it is likely that NULL is already defined. If you insist on making
> it self-contained, you can add the following:
> 
>   #ifdef __KERNEL__
>   #include <linux/stddef.h>
>   #else
>   #include <stddef.h>
>   #endif
> 
> scripts/selinux/mdp/mdp.c still includes <linux/kconfig.h>, which is
> also discouraged and should be fixed by a follow-up refactoring.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>  scripts/selinux/genheaders/Makefile           |  4 +---
>  scripts/selinux/genheaders/genheaders.c       |  3 ---
>  scripts/selinux/mdp/Makefile                  |  2 +-
>  scripts/selinux/mdp/mdp.c                     |  4 ----
>  security/selinux/include/classmap.h           | 19 ++++++++++++-------
>  .../selinux/include/initial_sid_to_string.h   |  2 --
>  6 files changed, 14 insertions(+), 20 deletions(-)

I'm not going to argue with the general idea behind this commit, but I
am going to complain about the writing style in that commit description,
it's not something I'm going to pull via the SELinux tree and I'm going
to object to it going in via any other tree.

Please rewrite the commit description to simply state the problem (e.g.
"we do not need to include the kernel headers when building the userspace
SELinux scripts") and the fix (e.g "we resolve this by making the kernel
specific portions protected by a __KERNEL__ macro check").  If you want
to preserve the snarky commentary you can do so in the cover letter.

> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 7229c9bf6c27..518209e1beb0 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -1,8 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  
> -#include <linux/capability.h>
> -#include <linux/socket.h>
> -
>  #define COMMON_FILE_SOCK_PERMS                                            \
>  	"ioctl", "read", "write", "create", "getattr", "setattr", "lock", \
>  		"relabelfrom", "relabelto", "append", "map"
> @@ -36,10 +33,6 @@
>  	"mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend", \
>  		"audit_read", "perfmon", "bpf", "checkpoint_restore"
>  
> -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
> -#error New capability defined, please update COMMON_CAP2_PERMS.
> -#endif
> -
>  /*
>   * Note: The name for any socket class should be suffixed by "socket",
>   *	 and doesn't contain more than one substr of "socket".
> @@ -181,6 +174,18 @@ const struct security_class_mapping secclass_map[] = {
>  	{ NULL }
>  };
>  
> +#ifdef __KERNEL__ /* avoid this check when building host programs */
> +
> +#include <linux/capability.h>
> +
> +#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
> +#error New capability defined, please update COMMON_CAP2_PERMS.
> +#endif
> +
> +#include <linux/socket.h>
> +
>  #if PF_MAX > 46
>  #error New address family defined, please update secclass_map.
>  #endif
> +
> +#endif /* __KERNEL__ */

Please keep both the CAP_LAST_CAP and PF_MAX checks where they are now,
closer to the affected code.

> diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h
> index 99b353b2abb4..f683a78b21fd 100644
> --- a/security/selinux/include/initial_sid_to_string.h
> +++ b/security/selinux/include/initial_sid_to_string.h
> @@ -1,7 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  
> -#include <linux/stddef.h>

Yes, we don't need the kernel's stddef.h here, but I would like us to
have some type of stdddef.h included here so we have NULL defined.

>  static const char *const initial_sid_to_string[] = {
>  	NULL, /* zero placeholder, not used */
>  	"kernel", /* kernel / SECINITSID_KERNEL */
> -- 
> 2.43.0

--
paul-moore.com

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Paul Moore]

On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> 
> This tool is only used in security/selinux/Makefile.
> 
> There is no reason to keep it under scripts/.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>  scripts/remove-stale-files                                 | 3 +++
>  scripts/selinux/Makefile                                   | 2 +-
>  scripts/selinux/genheaders/.gitignore                      | 2 --
>  scripts/selinux/genheaders/Makefile                        | 3 ---
>  security/selinux/.gitignore                                | 1 +
>  security/selinux/Makefile                                  | 7 +++++--
>  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
>  7 files changed, 10 insertions(+), 8 deletions(-)
>  delete mode 100644 scripts/selinux/genheaders/.gitignore
>  delete mode 100644 scripts/selinux/genheaders/Makefile
>  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)

As long as there is no harm in keeping genheaders under scripts/selinux,
and based on your cover letter it would appear that there is no problem
with the current location, I would prefer to keep it where it currently
lives.

--
paul-moore.com

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Masahiro Yamada]

On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > This tool is only used in security/selinux/Makefile.
> >
> > There is no reason to keep it under scripts/.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >  scripts/remove-stale-files                                 | 3 +++
> >  scripts/selinux/Makefile                                   | 2 +-
> >  scripts/selinux/genheaders/.gitignore                      | 2 --
> >  scripts/selinux/genheaders/Makefile                        | 3 ---
> >  security/selinux/.gitignore                                | 1 +
> >  security/selinux/Makefile                                  | 7 +++++--
> >  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
> >  7 files changed, 10 insertions(+), 8 deletions(-)
> >  delete mode 100644 scripts/selinux/genheaders/.gitignore
> >  delete mode 100644 scripts/selinux/genheaders/Makefile
> >  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)
>
> As long as there is no harm in keeping genheaders under scripts/selinux,
> and based on your cover letter it would appear that there is no problem
> with the current location, I would prefer to keep it where it currently
> lives.


'make clean' is meant to clean up the tree, but keep
build artifacts necessary for building external modules.


See the help message:


  clean           - Remove most generated files but keep the config and
                    enough build support to build external modules




'make clean' does not clean up under scripts/
because tools located scripts/ are used in tree-wide
and often used for external modules as well.

So, scripts/selinux/genheaders/genheaders is left over.


genheaders is locally used in security/selinux/.

'make clean' will properly clean up security/selinux/genheaders.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Paul Moore]

On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > This tool is only used in security/selinux/Makefile.
> > >
> > > There is no reason to keep it under scripts/.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >  scripts/remove-stale-files                                 | 3 +++
> > >  scripts/selinux/Makefile                                   | 2 +-
> > >  scripts/selinux/genheaders/.gitignore                      | 2 --
> > >  scripts/selinux/genheaders/Makefile                        | 3 ---
> > >  security/selinux/.gitignore                                | 1 +
> > >  security/selinux/Makefile                                  | 7 +++++--
> > >  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
> > >  7 files changed, 10 insertions(+), 8 deletions(-)
> > >  delete mode 100644 scripts/selinux/genheaders/.gitignore
> > >  delete mode 100644 scripts/selinux/genheaders/Makefile
> > >  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)
> >
> > As long as there is no harm in keeping genheaders under scripts/selinux,
> > and based on your cover letter it would appear that there is no problem
> > with the current location, I would prefer to keep it where it currently
> > lives.
>
> 'make clean' is meant to clean up the tree, but keep
> build artifacts necessary for building external modules.
>
> See the help message:
>
>   clean           - Remove most generated files but keep the config and
>                     enough build support to build external modules
>
> 'make clean' does not clean up under scripts/
> because tools located scripts/ are used in tree-wide
> and often used for external modules as well.
>
> So, scripts/selinux/genheaders/genheaders is left over.
>
> genheaders is locally used in security/selinux/.
>
> 'make clean' will properly clean up security/selinux/genheaders.

Your last sentence is confusing and doesn't align with the rest of
your email, please clarify.

Regardless, this sort of explanation is what one needs to put in the
commit description, a simple "There is no reason to keep it under
scripts/" isn't sufficient.  Patches like this need to provide a well
defined reason to justify the code churn.

-- 
paul-moore.com

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Masahiro Yamada]

On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > This tool is only used in security/selinux/Makefile.
> > > >
> > > > There is no reason to keep it under scripts/.
> > > >
> > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > ---
> > > >  scripts/remove-stale-files                                 | 3 +++
> > > >  scripts/selinux/Makefile                                   | 2 +-
> > > >  scripts/selinux/genheaders/.gitignore                      | 2 --
> > > >  scripts/selinux/genheaders/Makefile                        | 3 ---
> > > >  security/selinux/.gitignore                                | 1 +
> > > >  security/selinux/Makefile                                  | 7 +++++--
> > > >  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
> > > >  7 files changed, 10 insertions(+), 8 deletions(-)
> > > >  delete mode 100644 scripts/selinux/genheaders/.gitignore
> > > >  delete mode 100644 scripts/selinux/genheaders/Makefile
> > > >  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)
> > >
> > > As long as there is no harm in keeping genheaders under scripts/selinux,
> > > and based on your cover letter it would appear that there is no problem
> > > with the current location, I would prefer to keep it where it currently
> > > lives.
> >
> > 'make clean' is meant to clean up the tree, but keep
> > build artifacts necessary for building external modules.
> >
> > See the help message:
> >
> >   clean           - Remove most generated files but keep the config and
> >                     enough build support to build external modules
> >
> > 'make clean' does not clean up under scripts/
> > because tools located scripts/ are used in tree-wide
> > and often used for external modules as well.
> >
> > So, scripts/selinux/genheaders/genheaders is left over.
> >
> > genheaders is locally used in security/selinux/.
> >
> > 'make clean' will properly clean up security/selinux/genheaders.
>
> Your last sentence is confusing and doesn't align with the rest of
> your email, please clarify.


I do not understand what was unclear.



'make clean' cannot clean the current path,
scripts/selinux/genheaders/genheaders.

'make clean' can clean the proposed path,
security/selinux/genheaders.



genheaders is only used during the kernel build.
When you run 'make clean', there is no reason to keep it
for external modules.
Thus, it should move to the directory path that
'make clean' can handle.


Clearer?






>
> Regardless, this sort of explanation is what one needs to put in the
> commit description, a simple "There is no reason to keep it under
> scripts/" isn't sufficient.  Patches like this need to provide a well
> defined reason to justify the code churn.
>
> --
> paul-moore.com



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Paul Moore]

On Fri, Sep 6, 2024 at 12:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > >
> > > > > This tool is only used in security/selinux/Makefile.
> > > > >
> > > > > There is no reason to keep it under scripts/.
> > > > >
> > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > > ---
> > > > >  scripts/remove-stale-files                                 | 3 +++
> > > > >  scripts/selinux/Makefile                                   | 2 +-
> > > > >  scripts/selinux/genheaders/.gitignore                      | 2 --
> > > > >  scripts/selinux/genheaders/Makefile                        | 3 ---
> > > > >  security/selinux/.gitignore                                | 1 +
> > > > >  security/selinux/Makefile                                  | 7 +++++--
> > > > >  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
> > > > >  7 files changed, 10 insertions(+), 8 deletions(-)
> > > > >  delete mode 100644 scripts/selinux/genheaders/.gitignore
> > > > >  delete mode 100644 scripts/selinux/genheaders/Makefile
> > > > >  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)
> > > >
> > > > As long as there is no harm in keeping genheaders under scripts/selinux,
> > > > and based on your cover letter it would appear that there is no problem
> > > > with the current location, I would prefer to keep it where it currently
> > > > lives.
> > >
> > > 'make clean' is meant to clean up the tree, but keep
> > > build artifacts necessary for building external modules.
> > >
> > > See the help message:
> > >
> > >   clean           - Remove most generated files but keep the config and
> > >                     enough build support to build external modules
> > >
> > > 'make clean' does not clean up under scripts/
> > > because tools located scripts/ are used in tree-wide
> > > and often used for external modules as well.
> > >
> > > So, scripts/selinux/genheaders/genheaders is left over.
> > >
> > > genheaders is locally used in security/selinux/.
> > >
> > > 'make clean' will properly clean up security/selinux/genheaders.
> >
> > Your last sentence is confusing and doesn't align with the rest of
> > your email, please clarify.
>
> I do not understand what was unclear.

Near the start of your email you stated: "'make clean' does not clean
up under scripts/".  However you ended your email with "'make clean'
will properly clean up security/selinux/genheaders" which seems
contradictory to your initial statement; I was guessing that you were
implying that moving the genheaders script will allow `make clean` to
work properly, but you explicitly included the old/existing location
of security/selinux/genheaders directory in your comment which didn't
support that guess.

Your latest reply makes it a bit more clear.

-- 
paul-moore.com

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Masahiro Yamada]

On Sat, Sep 7, 2024 at 1:23 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Sep 6, 2024 at 12:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > >
> > > > > > This tool is only used in security/selinux/Makefile.
> > > > > >
> > > > > > There is no reason to keep it under scripts/.
> > > > > >
> > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > > > ---
> > > > > >  scripts/remove-stale-files                                 | 3 +++
> > > > > >  scripts/selinux/Makefile                                   | 2 +-
> > > > > >  scripts/selinux/genheaders/.gitignore                      | 2 --
> > > > > >  scripts/selinux/genheaders/Makefile                        | 3 ---
> > > > > >  security/selinux/.gitignore                                | 1 +
> > > > > >  security/selinux/Makefile                                  | 7 +++++--
> > > > > >  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
> > > > > >  7 files changed, 10 insertions(+), 8 deletions(-)
> > > > > >  delete mode 100644 scripts/selinux/genheaders/.gitignore
> > > > > >  delete mode 100644 scripts/selinux/genheaders/Makefile
> > > > > >  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)
> > > > >
> > > > > As long as there is no harm in keeping genheaders under scripts/selinux,
> > > > > and based on your cover letter it would appear that there is no problem
> > > > > with the current location, I would prefer to keep it where it currently
> > > > > lives.
> > > >
> > > > 'make clean' is meant to clean up the tree, but keep
> > > > build artifacts necessary for building external modules.
> > > >
> > > > See the help message:
> > > >
> > > >   clean           - Remove most generated files but keep the config and
> > > >                     enough build support to build external modules
> > > >
> > > > 'make clean' does not clean up under scripts/
> > > > because tools located scripts/ are used in tree-wide
> > > > and often used for external modules as well.
> > > >
> > > > So, scripts/selinux/genheaders/genheaders is left over.
> > > >
> > > > genheaders is locally used in security/selinux/.
> > > >
> > > > 'make clean' will properly clean up security/selinux/genheaders.
> > >
> > > Your last sentence is confusing and doesn't align with the rest of
> > > your email, please clarify.
> >
> > I do not understand what was unclear.
>
> Near the start of your email you stated: "'make clean' does not clean
> up under scripts/".  However you ended your email with "'make clean'
> will properly clean up security/selinux/genheaders" which seems
> contradictory to your initial statement; I was guessing that you were
> implying that moving the genheaders script will allow `make clean` to
> work properly, but you explicitly included the old/existing location
> of security/selinux/genheaders directory in your comment which didn't
> support that guess.


OK, now I understand.


I meant this:


  With this patch applied, 'make clean' will properly clean up
  security/selinux/genheaders.



> Your latest reply makes it a bit more clear.



So, are you ok with the following commit description,
which I proposed in another thread?



--------------->8--------------------
selinux: move genheaders to security/selinux/

This tool is only used in security/selinux/Makefile.

Move it to security/selinux/ so that 'make clean' can clean it up.

Please note 'make clean' does not visit scripts/ because tools under
scripts/ are often used for external module builds. Obviously, genheaders
is not the case here.
--------------->8--------------------














--
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] selinux: move genheaders to security/selinux/

[Paul Moore]

On Fri, Sep 6, 2024 at 12:38 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, Sep 7, 2024 at 1:23 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Sep 6, 2024 at 12:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > On Sat, Sep 7, 2024 at 12:37 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Sep 6, 2024 at 11:19 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > On Tue, Aug 27, 2024 at 6:22 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Aug  9, 2024 Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > > > > >
> > > > > > > This tool is only used in security/selinux/Makefile.
> > > > > > >
> > > > > > > There is no reason to keep it under scripts/.
> > > > > > >
> > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > > > > > ---
> > > > > > >  scripts/remove-stale-files                                 | 3 +++
> > > > > > >  scripts/selinux/Makefile                                   | 2 +-
> > > > > > >  scripts/selinux/genheaders/.gitignore                      | 2 --
> > > > > > >  scripts/selinux/genheaders/Makefile                        | 3 ---
> > > > > > >  security/selinux/.gitignore                                | 1 +
> > > > > > >  security/selinux/Makefile                                  | 7 +++++--
> > > > > > >  .../selinux/genheaders => security/selinux}/genheaders.c   | 0
> > > > > > >  7 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > >  delete mode 100644 scripts/selinux/genheaders/.gitignore
> > > > > > >  delete mode 100644 scripts/selinux/genheaders/Makefile
> > > > > > >  rename {scripts/selinux/genheaders => security/selinux}/genheaders.c (100%)
> > > > > >
> > > > > > As long as there is no harm in keeping genheaders under scripts/selinux,
> > > > > > and based on your cover letter it would appear that there is no problem
> > > > > > with the current location, I would prefer to keep it where it currently
> > > > > > lives.
> > > > >
> > > > > 'make clean' is meant to clean up the tree, but keep
> > > > > build artifacts necessary for building external modules.
> > > > >
> > > > > See the help message:
> > > > >
> > > > >   clean           - Remove most generated files but keep the config and
> > > > >                     enough build support to build external modules
> > > > >
> > > > > 'make clean' does not clean up under scripts/
> > > > > because tools located scripts/ are used in tree-wide
> > > > > and often used for external modules as well.
> > > > >
> > > > > So, scripts/selinux/genheaders/genheaders is left over.
> > > > >
> > > > > genheaders is locally used in security/selinux/.
> > > > >
> > > > > 'make clean' will properly clean up security/selinux/genheaders.
> > > >
> > > > Your last sentence is confusing and doesn't align with the rest of
> > > > your email, please clarify.
> > >
> > > I do not understand what was unclear.
> >
> > Near the start of your email you stated: "'make clean' does not clean
> > up under scripts/".  However you ended your email with "'make clean'
> > will properly clean up security/selinux/genheaders" which seems
> > contradictory to your initial statement; I was guessing that you were
> > implying that moving the genheaders script will allow `make clean` to
> > work properly, but you explicitly included the old/existing location
> > of security/selinux/genheaders directory in your comment which didn't
> > support that guess.
>
>
> OK, now I understand.
>
>
> I meant this:
>
>
>   With this patch applied, 'make clean' will properly clean up
>   security/selinux/genheaders.
>
>
>
> > Your latest reply makes it a bit more clear.
>
>
>
> So, are you ok with the following commit description,
> which I proposed in another thread?
>
>
>
> --------------->8--------------------
> selinux: move genheaders to security/selinux/
>
> This tool is only used in security/selinux/Makefile.
>
> Move it to security/selinux/ so that 'make clean' can clean it up.
>
> Please note 'make clean' does not visit scripts/ because tools under
> scripts/ are often used for external module builds. Obviously, genheaders
> is not the case here.
> --------------->8--------------------

Yes, thank you.


-- 
paul-moore.com