* [PATCH 0/2] selinux: Do not include <linux/*.h> from host programs (+ extra clean-up)
@ 2024-08-09 12:19 Masahiro Yamada
2024-08-09 12:19 ` [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs Masahiro Yamada
2024-08-09 12:19 ` [PATCH 2/2] selinux: move genheaders to security/selinux/ Masahiro Yamada
0 siblings, 2 replies; 11+ messages in thread
From: Masahiro Yamada @ 2024-08-09 12:19 UTC (permalink / raw)
To: Paul Moore, linux-security-module
Cc: linux-kbuild, linux-kernel, Daniel Gomez, Masahiro Yamada,
Ondrej Mosnacek, Stephen Smalley, selinux
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs 2024-08-09 12:19 [PATCH 0/2] selinux: Do not include <linux/*.h> from host programs (+ extra clean-up) Masahiro Yamada @ 2024-08-09 12:19 ` Masahiro Yamada 2024-08-26 21:14 ` Paul Moore 2024-08-09 12:19 ` [PATCH 2/2] selinux: move genheaders to security/selinux/ Masahiro Yamada 1 sibling, 1 reply; 11+ messages in thread From: Masahiro Yamada @ 2024-08-09 12:19 UTC (permalink / raw) To: Paul Moore, linux-security-module Cc: linux-kbuild, linux-kernel, Daniel Gomez, Masahiro Yamada, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs 2024-08-09 12:19 ` [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs Masahiro Yamada @ 2024-08-26 21:14 ` Paul Moore 0 siblings, 0 replies; 11+ messages in thread From: Paul Moore @ 2024-08-26 21:14 UTC (permalink / raw) To: Masahiro Yamada, linux-security-module Cc: linux-kbuild, linux-kernel, Daniel Gomez, Masahiro Yamada, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-08-09 12:19 [PATCH 0/2] selinux: Do not include <linux/*.h> from host programs (+ extra clean-up) Masahiro Yamada 2024-08-09 12:19 ` [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs Masahiro Yamada @ 2024-08-09 12:19 ` Masahiro Yamada 2024-08-26 21:14 ` Paul Moore 1 sibling, 1 reply; 11+ messages in thread From: Masahiro Yamada @ 2024-08-09 12:19 UTC (permalink / raw) To: Paul Moore, linux-security-module Cc: linux-kbuild, linux-kernel, Daniel Gomez, Masahiro Yamada, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-08-09 12:19 ` [PATCH 2/2] selinux: move genheaders to security/selinux/ Masahiro Yamada @ 2024-08-26 21:14 ` Paul Moore 2024-09-06 15:19 ` Masahiro Yamada 0 siblings, 1 reply; 11+ messages in thread From: Paul Moore @ 2024-08-26 21:14 UTC (permalink / raw) To: Masahiro Yamada, linux-security-module Cc: linux-kbuild, linux-kernel, Daniel Gomez, Masahiro Yamada, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-08-26 21:14 ` Paul Moore @ 2024-09-06 15:19 ` Masahiro Yamada 2024-09-06 15:37 ` Paul Moore 0 siblings, 1 reply; 11+ messages in thread From: Masahiro Yamada @ 2024-09-06 15:19 UTC (permalink / raw) To: Paul Moore Cc: linux-security-module, linux-kbuild, linux-kernel, Daniel Gomez, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-09-06 15:19 ` Masahiro Yamada @ 2024-09-06 15:37 ` Paul Moore 2024-09-06 16:06 ` Masahiro Yamada 0 siblings, 1 reply; 11+ messages in thread From: Paul Moore @ 2024-09-06 15:37 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-security-module, linux-kbuild, linux-kernel, Daniel Gomez, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-09-06 15:37 ` Paul Moore @ 2024-09-06 16:06 ` Masahiro Yamada 2024-09-06 16:22 ` Paul Moore 0 siblings, 1 reply; 11+ messages in thread From: Masahiro Yamada @ 2024-09-06 16:06 UTC (permalink / raw) To: Paul Moore Cc: linux-security-module, linux-kbuild, linux-kernel, Daniel Gomez, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-09-06 16:06 ` Masahiro Yamada @ 2024-09-06 16:22 ` Paul Moore 2024-09-06 16:37 ` Masahiro Yamada 0 siblings, 1 reply; 11+ messages in thread From: Paul Moore @ 2024-09-06 16:22 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-security-module, linux-kbuild, linux-kernel, Daniel Gomez, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-09-06 16:22 ` Paul Moore @ 2024-09-06 16:37 ` Masahiro Yamada 2024-09-06 17:33 ` Paul Moore 0 siblings, 1 reply; 11+ messages in thread From: Masahiro Yamada @ 2024-09-06 16:37 UTC (permalink / raw) To: Paul Moore Cc: linux-security-module, linux-kbuild, linux-kernel, Daniel Gomez, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selinux: move genheaders to security/selinux/ 2024-09-06 16:37 ` Masahiro Yamada @ 2024-09-06 17:33 ` Paul Moore 0 siblings, 0 replies; 11+ messages in thread From: Paul Moore @ 2024-09-06 17:33 UTC (permalink / raw) To: Masahiro Yamada Cc: linux-security-module, linux-kbuild, linux-kernel, Daniel Gomez, Ondrej Mosnacek, Stephen Smalley, selinux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-09-06 17:34 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-09 12:19 [PATCH 0/2] selinux: Do not include <linux/*.h> from host programs (+ extra clean-up) Masahiro Yamada 2024-08-09 12:19 ` [PATCH 1/2] selinux: do not include <linux/*.h> headers from host programs Masahiro Yamada 2024-08-26 21:14 ` Paul Moore 2024-08-09 12:19 ` [PATCH 2/2] selinux: move genheaders to security/selinux/ Masahiro Yamada 2024-08-26 21:14 ` Paul Moore 2024-09-06 15:19 ` Masahiro Yamada 2024-09-06 15:37 ` Paul Moore 2024-09-06 16:06 ` Masahiro Yamada 2024-09-06 16:22 ` Paul Moore 2024-09-06 16:37 ` Masahiro Yamada 2024-09-06 17:33 ` Paul Moore
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).