On Thu, Apr 13, 2023 at 1:46 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Wed, Apr 12, 2023 at 6:00 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > On Wed, Apr 12, 2023 at 10:56 PM Paul Moore <paul@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > ---
> > > 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
>
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
