On Tue, Jan 29, 2019 at 1:02 PM Daniel Jurgens <danielj@xxxxxxxxxxxx> wrote: > On 1/29/2019 11:48 AM, Daniel Jurgens wrote: > > On 1/29/2019 11:30 AM, Paul Moore wrote: > >> On Mon, Jan 28, 2019 at 9:57 PM Don Dutile <ddutile@xxxxxxxxxx> wrote: > >>> On 01/28/2019 06:03 PM, Paul Moore wrote: > >>>> On Mon, Jan 28, 2019 at 11:57 AM Daniel Jurgens <danielj@xxxxxxxxxxxx> wrote: > >>>>> On 1/28/2019 10:37 AM, Paul Moore wrote: > >>>>>> On Sun, Jan 27, 2019 at 3:10 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote: > >>>>>>> From: Daniel Jurgens <danielj@xxxxxxxxxxxx> > >>>>>>> > >>>>>>> --- > >>>>>>> drivers/infiniband/core/security.c | 34 ++++-------------------------- > >>>>>>> include/rdma/ib_mad.h | 3 --- > >>>>>>> 2 files changed, 4 insertions(+), 33 deletions(-) > >>>> I'm trying to quickly understand the MAD agent lifecycle, and it looks > >>>> like you have your own register/unregister routines, with locking, so > >>>> is it reasonable to assume that it would be possible to iterate over > >>>> the MAD agents in the IB code? I wonder if it would be possible to > >>>> group MAD agents (per-port grouping, does that make sense?) such that > >>>> several agents would share a single LSM notifier registration? > >>>> > >>> one can have numerous MAD agents. it isn't just one. > >> Clearly. Daniel already mentioned hundreds of thousands of MAD agents. > > Just to be clear, it was hundreds to thousands, not hundreds of thousands. Ah, sorry about that, I guess my mind multiplied it by a few orders of magnitude. Regardless, even a few hundred is a lot for a full list traversal. > >> From what I've gathered from this thread the issue stems from the > >> ib_mad_agent_security_setup() where we register the > >> ib_mad_agent_security_change() callback with the LSM notification > >> mechanism. Looking at the ib_mad_agent_security_change() function, > >> all it seems to do is perform a LSM permission check > >> (security_ib_endport_manage_subnet()) and store the result in > >> ib_mad_agent->smp_allowed; quickly grep'ing through the code, the only > >> place where I see that value used is in ib_mad_enforce_security(). > >> > >> Looking at ib_mad_enforce_security(), it seems to have access to the > >> necessary MAD agent information, via ib_mad_agent_private->agent, to > >> perform the LSM permission check directly in this function. Could we > >> not get rid of the MAD agent LSM notification callback and simply do > >> the LSM access check directly in ib_mad_enforce_security()? What am I > >> missing? > >> > >> I do see that the SELinux implementation of the > >> security_ib_endport_manage_subnet() LSM hook doesn't seem to cache the > >> IB endport labels. If that proves to be a performance issue we can > >> create a cache for that (like we do for the pkeys and a few other > >> objects). > >> > >> I barely understand IB, so it is very possible there is a fundamental > >> flaw in the logic above, but let's try to work together and figure > >> something out. The above may not be it, but I'm not ready to throw in > >> the towel. > > I think that's a reasonable idea, also holding agents lists in the IB layer and only registering for one notifier could work as well (it's already registered for the PKeys). Jason had suggested that internally, but the problem is I wasn't really able to reproduce the reported issue, so we're kind of flying blind either way in terms of knowing if either approach addresses the perf problem. > > I guess I should clarify. Obviously if we only register for the notifier once and hold the list(s) in the IB layer we won't have issues with register/unregister, but there may still be a problem lurking during a notification event. So this is less bad, at least it wouldn't affect non SElinux users. Okay, so let's attempt the change above where we just do the access check directly. Although I'm a little concerned that without a reproducer we might not end up fixing the problem we're trying to fix. Is anyone in touch with the person who originally reported the problem? It would be great if we could get that person to verify the change ... -- paul moore www.paul-moore.com
