On Thu, Sep 23, 2021 at 10:13 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> It's become clear you *really* don't like passing the cred pointer
> here, presumably based on a very specific security model for lockdown.
Not just that. I'm sensitive about the cred pointer because of the
timing, but I'm also sensitive about just looking at code and saying
"that makes no sense".
Having callers do pointless things that make no sense to the callers
is a bad thing. Having a patch where 59 out of 63 callers just
mindlessly pass in "current_cred()", and then remaining three callers
pass in NULL for NO DISCERNIBLE REASON is a sign of just a bad
interface.
And I realize that to you, these interfaces are what you do.
To everybody else, it's completely mindless noise that makes no sense
at all, because there's no _logic_ to it. It's random LSM calls in
random contexts that have no clear reason for them, because the
"reason" is hidden in some odd SELinux rule that no kernel developer
even knows about.
You can't even grep for the use of it, because there is none. It's
literally randomness. Which means that it _has_ to make conceptual
sense to be at all maintainable.
So when I see a diffstat where basically 90% of the patch has
ABSOLUTELY NOTHING to do with SElinux or anything you maintain, and
that 90% makes absolutely no sense to begin with, guess what? I think
that patch is bad.
When I then look closer, and see that the _reason_ for the patch is
that SElinux - yet again - incorrectly accessed process credentials in
random contexts, I just do "this is not just a bad patch, this is
actually a fundamental problem with SELinux".
See where I'm coming from? The "specific security model for lockdown"
is just the high-level view of what made sense. You violated that view
for bad reasons, with a bad patch, and with bad timing.
This isn't _just_ a SElinux problem. I think it's a LSM problem in
general. Lots of those random callbacks are completely
incomprehensible, have odd rules, and the LSM people aren't even
trying to have them make sense.
As long as the oddity is contained, that's one thing. But when it is
made this obvious and affects random code in strange places in the
kernel, and it's for a feature that had a lot of discussion even when
it got merged, I'm putting my foot down.
Linus
