On Thu, Apr 20, 2023 at 9:36 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> 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).
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.
--
paul-moore.com
