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. >>> Think: you can have any number of apps scanning the fabric for various stats >>> to draw node/graph pics, gather per-port packet counts, gather bw numbers, >>> etc. For IB, scanning the fabric involves 'mad pkts'. >>> In addition to scanning, one could do 'less then nice things' to switches, >>> not just read stats but set parameters... like routing table entries. >>> and thus, the security level addition. >>> One is highly unlikely to grant access to an app to do mad pkt generation/use, >>> then change your mind (semi-?)randomly to enable/disable it.... generally always enabled, >>> or always disabled. >>> >>> Let's take this case as a catalyst to resolve a previously unknown perf issue, >>> and not hold it up to a higher functional requirement then other apps that want to be secured. >> See my comment above. I don't even know what you are talking about >> regarding "higher functional arguments", what other subsystems? >> Seriously Don, language like this causes an extreme allergic reaction. >> >> Anyway, let's move on to something a bit more constructive, shall we? >> >> 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.
