| Message ID | 20230412205429.389382-1-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: ensure av_permissions.h is built when needed | expand |
On Wed, Apr 12, 2023 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > The Makefile rule responsible for building flask.h and > av_permissions.h only lists flask.h as a target which means that > av_permissions.h is only generated when flash.h needs to be Typo: flash.h -> flask.h > generated. This patch fixes this by adding av_permissions.h as a > target to the rule. > > Fixes: 8753f6bec352 ("selinux: generate flask headers during kernel build") > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > index 103c2776478a..df35d4ec46f0 100644 > --- a/security/selinux/Makefile > +++ b/security/selinux/Makefile > @@ -26,5 +26,5 @@ quiet_cmd_flask = GEN $(obj)/flask.h $(obj)/av_permissions.h > cmd_flask = $< $(obj)/flask.h $(obj)/av_permissions.h > > targets += flask.h av_permissions.h > -$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE > +$(obj)/flask.h $(obj)/av_permissions.h: scripts/selinux/genheaders/genheaders FORCE I had something like this in my patch originally, but then I couldn't come up with a scenario where it would matter, so I dropped it... Are you sure it's really needed? (See also the "$(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h" rule above.) If it is, then I think you want to use "grouped targets" instead: $(obj)/flask.h $(obj)/av_permissions.h &: [...] See: https://www.gnu.org/software/make/manual/html_node/Multiple-Targets.html > $(call if_changed,flask) > -- > 2.40.0 >
On Wed, Apr 12, 2023 at 6:00 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Wed, Apr 12, 2023 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > The Makefile rule responsible for building flask.h and > > av_permissions.h only lists flask.h as a target which means that > > av_permissions.h is only generated when flash.h needs to be > > Typo: flash.h -> flask.h Thanks. Spell checkers don't work very well when you typo one word into another wrong, but correctly spelled, word :) > > generated. This patch fixes this by adding av_permissions.h as a > > target to the rule. > > > > Fixes: 8753f6bec352 ("selinux: generate flask headers during kernel build") > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > security/selinux/Makefile | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > > index 103c2776478a..df35d4ec46f0 100644 > > --- a/security/selinux/Makefile > > +++ b/security/selinux/Makefile > > @@ -26,5 +26,5 @@ quiet_cmd_flask = GEN $(obj)/flask.h $(obj)/av_permissions.h > > cmd_flask = $< $(obj)/flask.h $(obj)/av_permissions.h > > > > targets += flask.h av_permissions.h > > -$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE > > +$(obj)/flask.h $(obj)/av_permissions.h: scripts/selinux/genheaders/genheaders FORCE > > I had something like this in my patch originally, but then I couldn't > come up with a scenario where it would matter, so I dropped it... Are > you sure it's really needed? Yep. I don't hit this very often, but it does happen. Here is a forced example: % rm security/selinux/av_permissions.h % make security/selinux/ CALL scripts/checksyscalls.sh DESCEND objtool make[3]: 'install_headers' is up to date. CC security/selinux/avc.o In file included from security/selinux/avc.c:30: ./security/selinux/include/avc.h:20:10: fatal error: av_permissions.h: No such f ile or directory 20 | #include "av_permissions.h" | ^~~~~~~~~~~~~~~~~~ compilation terminated. make[3]: *** [scripts/Makefile.build:252: security/selinux/avc.o] Error 1 make[2]: *** [scripts/Makefile.build:494: security/selinux] Error 2 make[1]: *** [scripts/Makefile.build:494: security] Error 2 make: *** [Makefile:2028: .] Error 2 > (See also the "$(addprefix > $(obj)/,$(selinux-y)): $(obj)/flask.h" rule above.) Yes, I know, but since there are only two files/targets I felt it was clearer to add the file without the function-mangling. If people feel strongly about it I can change it, but I'd just assume leave it as-is. If there were a number of files, yes, using 'addprefix' would be the way to go. > If it is, then I think you want to use "grouped targets" instead: > > $(obj)/flask.h $(obj)/av_permissions.h &: [...] > > See: > https://www.gnu.org/software/make/manual/html_node/Multiple-Targets.html Thanks, I wasn't aware of that.
On Thu, Apr 13, 2023 at 1:46 AM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Apr 12, 2023 at 6:00 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, Apr 12, 2023 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > The Makefile rule responsible for building flask.h and > > > av_permissions.h only lists flask.h as a target which means that > > > av_permissions.h is only generated when flash.h needs to be > > > > Typo: flash.h -> flask.h > > Thanks. Spell checkers don't work very well when you typo one word > into another wrong, but correctly spelled, word :) > > > > generated. This patch fixes this by adding av_permissions.h as a > > > target to the rule. > > > > > > Fixes: 8753f6bec352 ("selinux: generate flask headers during kernel build") > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > --- > > > security/selinux/Makefile | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > > > index 103c2776478a..df35d4ec46f0 100644 > > > --- a/security/selinux/Makefile > > > +++ b/security/selinux/Makefile > > > @@ -26,5 +26,5 @@ quiet_cmd_flask = GEN $(obj)/flask.h $(obj)/av_permissions.h > > > cmd_flask = $< $(obj)/flask.h $(obj)/av_permissions.h > > > > > > targets += flask.h av_permissions.h > > > -$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE > > > +$(obj)/flask.h $(obj)/av_permissions.h: scripts/selinux/genheaders/genheaders FORCE > > > > I had something like this in my patch originally, but then I couldn't > > come up with a scenario where it would matter, so I dropped it... Are > > you sure it's really needed? > > Yep. I don't hit this very often, but it does happen. Here is a > forced example: > > % rm security/selinux/av_permissions.h > % make security/selinux/ > CALL scripts/checksyscalls.sh > DESCEND objtool > make[3]: 'install_headers' is up to date. > CC security/selinux/avc.o > In file included from security/selinux/avc.c:30: > ./security/selinux/include/avc.h:20:10: fatal error: av_permissions.h: No such f > ile or directory > 20 | #include "av_permissions.h" > | ^~~~~~~~~~~~~~~~~~ > compilation terminated. > make[3]: *** [scripts/Makefile.build:252: security/selinux/avc.o] Error 1 > make[2]: *** [scripts/Makefile.build:494: security/selinux] Error 2 > make[1]: *** [scripts/Makefile.build:494: security] Error 2 > make: *** [Makefile:2028: .] Error 2 > > > (See also the "$(addprefix > > $(obj)/,$(selinux-y)): $(obj)/flask.h" rule above.) > > Yes, I know, but since there are only two files/targets I felt it was > clearer to add the file without the function-mangling. If people feel > strongly about it I can change it, but I'd just assume leave it as-is. > If there were a number of files, yes, using 'addprefix' would be the > way to go. Sorry, that's not what I meant. My point was that that line makes the whole SELinux code dependent on (only) flask.h, and that flask.h is being used to establish the link between SELinux code and the genheaders invocation. I don't quite understand how you would end up with fresh flask.h, but missing/outdated av_permissions.h (other than forcibly removing it as in your example). Nonetheless, I agree it's better to list both as a grouped target to avoid confusion and broken corner cases. However, we should then also adjust the "addprefix" rule to depend on both flask.h and av_permissions.h to keep the symmetry and completeness. Since you already merged the v2 without it - do you want to send another patch to close the gap or should I? > > If it is, then I think you want to use "grouped targets" instead: > > > > $(obj)/flask.h $(obj)/av_permissions.h &: [...] > > > > See: > > https://www.gnu.org/software/make/manual/html_node/Multiple-Targets.html > > Thanks, I wasn't aware of that. > > -- > paul-moore.com >
On Thu, Apr 20, 2023 at 9:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Thu, Apr 13, 2023 at 1:46 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Apr 12, 2023 at 6:00 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Wed, Apr 12, 2023 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote: > > > > The Makefile rule responsible for building flask.h and > > > > av_permissions.h only lists flask.h as a target which means that > > > > av_permissions.h is only generated when flash.h needs to be > > > > > > Typo: flash.h -> flask.h > > > > Thanks. Spell checkers don't work very well when you typo one word > > into another wrong, but correctly spelled, word :) > > > > > > generated. This patch fixes this by adding av_permissions.h as a > > > > target to the rule. > > > > > > > > Fixes: 8753f6bec352 ("selinux: generate flask headers during kernel build") > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > > > --- > > > > security/selinux/Makefile | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/security/selinux/Makefile b/security/selinux/Makefile > > > > index 103c2776478a..df35d4ec46f0 100644 > > > > --- a/security/selinux/Makefile > > > > +++ b/security/selinux/Makefile > > > > @@ -26,5 +26,5 @@ quiet_cmd_flask = GEN $(obj)/flask.h $(obj)/av_permissions.h > > > > cmd_flask = $< $(obj)/flask.h $(obj)/av_permissions.h > > > > > > > > targets += flask.h av_permissions.h > > > > -$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE > > > > +$(obj)/flask.h $(obj)/av_permissions.h: scripts/selinux/genheaders/genheaders FORCE > > > > > > I had something like this in my patch originally, but then I couldn't > > > come up with a scenario where it would matter, so I dropped it... Are > > > you sure it's really needed? > > > > Yep. I don't hit this very often, but it does happen. Here is a > > forced example: > > > > % rm security/selinux/av_permissions.h > > % make security/selinux/ > > CALL scripts/checksyscalls.sh > > DESCEND objtool > > make[3]: 'install_headers' is up to date. > > CC security/selinux/avc.o > > In file included from security/selinux/avc.c:30: > > ./security/selinux/include/avc.h:20:10: fatal error: av_permissions.h: No such f > > ile or directory > > 20 | #include "av_permissions.h" > > | ^~~~~~~~~~~~~~~~~~ > > compilation terminated. > > make[3]: *** [scripts/Makefile.build:252: security/selinux/avc.o] Error 1 > > make[2]: *** [scripts/Makefile.build:494: security/selinux] Error 2 > > make[1]: *** [scripts/Makefile.build:494: security] Error 2 > > make: *** [Makefile:2028: .] Error 2 > > > > > (See also the "$(addprefix > > > $(obj)/,$(selinux-y)): $(obj)/flask.h" rule above.) > > > > Yes, I know, but since there are only two files/targets I felt it was > > clearer to add the file without the function-mangling. If people feel > > strongly about it I can change it, but I'd just assume leave it as-is. > > If there were a number of files, yes, using 'addprefix' would be the > > way to go. > > Sorry, that's not what I meant. My point was that that line makes the > whole SELinux code dependent on (only) flask.h, and that flask.h is > being used to establish the link between SELinux code and the > genheaders invocation. I don't quite understand how you would end up > with fresh flask.h, but missing/outdated av_permissions.h (other than > forcibly removing it as in your example). Sometimes strange things happen during development and code review :) It happens so rarely, and not recently, that I can't say what triggered it - it may have been me manually cleaning up some files due to odd directory state - but it *has* happened, and we might as well make the Makefile do the right thing. > Nonetheless, I agree it's better to list both as a grouped target to > avoid confusion and broken corner cases. However, we should then also > adjust the "addprefix" rule to depend on both flask.h and > av_permissions.h to keep the symmetry and completeness. Since you > already merged the v2 without it - do you want to send another patch > to close the gap or should I? Agreed. If you had replied sooner I would have been okay with merging a patch before the merge window opens, but with the merge window opening in a few days I'm not going to merge anything that isn't critical. Yes, the risk is low, but the advantage gained is even lower. I can send a patch after the merge window closes, or you can do it; I don't care much either way.
diff --git a/security/selinux/Makefile b/security/selinux/Makefile index 103c2776478a..df35d4ec46f0 100644 --- a/security/selinux/Makefile +++ b/security/selinux/Makefile @@ -26,5 +26,5 @@ quiet_cmd_flask = GEN $(obj)/flask.h $(obj)/av_permissions.h cmd_flask = $< $(obj)/flask.h $(obj)/av_permissions.h targets += flask.h av_permissions.h -$(obj)/flask.h: scripts/selinux/genheaders/genheaders FORCE +$(obj)/flask.h $(obj)/av_permissions.h: scripts/selinux/genheaders/genheaders FORCE $(call if_changed,flask)
The Makefile rule responsible for building flask.h and av_permissions.h only lists flask.h as a target which means that av_permissions.h is only generated when flash.h needs to be generated. This patch fixes this by adding av_permissions.h as a target to the rule. Fixes: 8753f6bec352 ("selinux: generate flask headers during kernel build") Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)