Re: [PATCH rdma-next] IB/core: Don't register MAD agents for LSM notifications

[Paul Moore <paul@paul-moore.com>]

On Fri, Feb 1, 2019 at 9:21 AM Don Dutile <ddutile@redhat.com> wrote:
> On 01/29/2019 12:30 PM, Paul Moore wrote:
> > On Mon, Jan 28, 2019 at 9:57 PM Don Dutile <ddutile@redhat.com> wrote:
> >> On 01/28/2019 06:03 PM, Paul Moore wrote:
> >>> On Mon, Jan 28, 2019 at 11:57 AM Daniel Jurgens <danielj@mellanox.com> wrote:
> >>>> On 1/28/2019 10:37 AM, Paul Moore wrote:
> >>>>> On Sun, Jan 27, 2019 at 3:10 AM Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>> From: Daniel Jurgens <danielj@mellanox.com>
> >>>>>>
> >>>>>> ---
> >>>>>>    drivers/infiniband/core/security.c | 34 ++++--------------------------
> >>>>>>    include/rdma/ib_mad.h              |  3 ---
> >>>>>>    2 files changed, 4 insertions(+), 33 deletions(-)
> >>>>> Perhaps predictably, I'm not very excited about this change.  Have you
> >>>>> looked closer into the slowdown to see where the cycles are being
> >>>>> spent?  I'm wondering if the issue is that a large number of notifiers
> >>>>> are being registered with the same priority causing the while loop in
> >>>>> notifier_chain_register() to take a significant amount of time.
> >>>>
> >>>> That's what's happening, each MAD agent is registering it's own notifier. The bug reporter was creating hundreds or thousands of  short lived MAD agents. With IRQs disabled too long it resulted in timeouts.
> >>>>
> >>>> When I initially added the notifier mechanism I thought it was you that said it wasn't really needed, since access wasn't generally revoked in these types of scenarios. Given that I didn't think this would be especially controversial. It was nice to have, unfortunately it causes problems even for users that don't enable SELinux.
> >>>
> >>> Revoking permission is difficult, and in some cases likely impossible,
> >>> but that doesn't mean we shouldn't try to make it possible when we
> >>> can.  I'd like to see if we can sort this out before we give up and
> >>> rip it out.
> >>
> >> Agree that it'd be nice to see it work, but since few (no other?) subsys registered
> >> with selinux/security is doing this, can we 'release' the IB mad agents from this
> >> 'imprisonment' and figure out a general solution that resolves this issue, and
> >> then it can be implemented not only for ib mad packet access, but for other
> >> subsys using selinux/security, and wanting the same revocation support.
> >
> > If you scroll up, ever so slightly to my last reply (it's just a few
> > lines, I'll wait), you'll see where I said "I'd like to see if we can
> > sort this out before we give up and rip it out."
> >
> > I don't recall at the moment why the notifier approach was taken with
> > the IB hooks back when Daniel wrote the code, and I'm definitely not
> > sure what other subsystems you are using for your comparisons here,
> > but I can tell you that I am beyond sick and tired of hearing people
> > immediately jump to "rip out this security code" because of some
> > performance glitch.  I care about performance too, okay?  We've got a
>
> I didn't advocate for ripping out the security code; I advocated to eliminating
> the notifier callback, not setup by many other subsystem selinux consumers,
> so the notifier performance issue could be resolved.
> I see this as a notifier perf issue, not a security issue.

In the original patch Daniel posted, removing the notifier callback
resulted in the removing the security_ib_endport_manage_subnet() call,
which is removing security code.  I described one way to remove the
MAD LSM notifier entirely (basically move the LSM hook down into
ib_mad_enforce_security()) as well as one way to reduce it's usage
(group MAD agents together); it looks like Daniel is taking the latter
approach.  Either option should mitigate the locking issue we're
currently seeing while preserving the access controls of the current
code.

For the record, the IB code has a LSM notifier callback here because
it is caching an access decision (ib_mad_agent.smp_allowed) so the IB
code needs to be notified when the LSM's security policy changes so it
can update it's cached access control decision.  You don't see the
notifier used in other kernel subsystems because they don't cache
access decisions like IB does.  This is why the claim of IB being
unfairly singled out with the LSM notifier is misleading, and not
correct IMHO.

> > bunch of clever people here, let's spend some time and see if we can
> > improve what we've got before we make some knee jerk reaction and just
> > rip out the access controls.
>
> It's not knee jerk.  I spent hours hacking away at the RHEL implementation
> to skip notification registration if selinux is disabled... the short-term 'hack'
> for the impacted user(s), since the users weren't enabling selinux. :(
> *but*, I'm looking for a long-term, selinux-enabled solution.

Removing the LSM hook without making some effort at preserving the
access control made it look like a knee jerk to me, but perhaps I've
seen this stuff too often and it strikes a nerve.  Regardless, it
looks like we have some options now for a long(er) term solution which
should address both our concerns.

-- 
paul moore
www.paul-moore.com