On 3/16/2012 11:44 AM, Ira Weiny wrote:
> On Fri, 16 Mar 2012 11:17:16 -0400
> Hal Rosenstock <h...@dev.mellanox.co.il> wrote:
>
>> On 3/15/2012 2:23 PM, Jim Foraker wrote:
>>>
>>> On Tue, 2012-03-13 at 05:29 -0700, Hal Rosenstock wrote:
>>>> On 3/9/2012 2:27 PM, Jim Foraker wrote:
>>>>>
>>>>> On Fri, 2012-03-09 at 05:00 -0800, Hal Rosenstock wrote:
>>>>>> On 3/6/2012 5:18 PM, Jim Foraker wrote:
>>>>>>>
>>>>>>> Support for "m_key=" parameter,
>>>>>>
>>>>>> Shouldn't mkey be on a per port basis rather than a single mkey for all
>>>>>> ports ?
>>>>> In the long run, I agree. However, adding this support brings the
>>>>> diags up to speed with the "one mkey per subnet" model that OpenSM
>>>>> currently (sort of) supports.
>>>>
>>>> OpenSM mkey support is not complete for even that model. IMO the diags
>>>> should not just be in sync with the current OpenSM (a minimum bar for
>>>> this support) but support the general requirement. In the end, I suspect
>>>> there will be several mkey policies supported by OpenSM.
>>> I've got some tentative OpenSM patches which I'll hopefully be
>>> ready to post once we've hashed out the tools side.
>>
>> Does the OpenSM side need to wait for that ?
>
> I don't believe so. This whole thing started a few months back when I was
> looking into using the mkey
> and found that even "one mkey" was not fully and correctly supported.
Yes, there are a number of aspects missing in OpenSM for even that.
> Jim has been kind enough to pick up the torch.
Indeed.
> In my opinion, the first step was to get OpenSM and the diags to correctly
> support a single mkey.
That's fine; I just want to be sure about future extensibility
especially of anything exposed in API or to user.
> This should have been straight forward and in fact I think Jim has spent more
> time discussing this
> on the list than he did "fixing" OpenSM and the diags.
Maybe more than the diags side; not sure about OpenSM. But this is often
the case with patches...
> I still believe a single mkey is better than what we have now and would be a
> valid option for
> someone looking for very basic security such as an accidental change from
> non-management nodes
> or looking to verify that all hardware in a large fabric is indeed responding
> to mkey's correctly.
Agreed.
> If you can guarantee that once we figure out this algorithm it will be the
> only one used for all SM's
> then I can see the point in not supporting a single mkey parameter. (And
> perhaps for tools like
> ibnetdiscover that should be removed because it will only work with the "one
> mkey" approach.)
Maybe that's the case now but IMO it would need to be updated to work
when there an mkey per port model is in use.
> That said, in general for diagnostic purposes, having an mkey option is not a
> bad idea;
> especially on single port utilities.
Sure; but what does that mean in an mkey per port model ?
-- Hal
> Going forward the diags will have to support an "mkeypassword" or the like to
> seed mkey generation.
> But I believe that can be supported moving forward from a single mkey
> approach both in OpenSM and the diags.
>
> Ira
>
>>
>> -- Hal
>>
>>> At the moment,
>>> they're centered around the "one mkey to rule them all" model too, but
>>> should make it easier to support other model(s).
>>>
>>>>
>>>>> We should do better down the road, but I
>>>>> don't think this patch gets in the way of that.
>>>>
>>>> I wouldn't think the approach for a per port MKey would use a single
>>>> MKey parameter and store it in the source port structure.
>>> I was referring specifically to this patch, and in particular the
>>> visible change, adding a "m_key" option to the config file.
>>> Dumping the mkey in the source port structure is absolutely a hack.
>>> The problem is that no real context is kept on the destination port in
>>> the current API, which near as I can tell means supporting mkeys
>>> (particularly multiple mkeys) cleanly requires either bumping the API
>>> version, or falling back on the deprecated saving of global state in the
>>> library. This would need to be cleaned up, but we could bunch that
>>> change with others as part of a future API rev.
>>>
>>>>
>>>> -- Hal
>>>>
>>>>>>> plus config file now installs user-read-only.
>>>>>>
>>>>>> This part is best as a separate patch.
>>>>> I agree. I was worried that applying the config change without the
>>>>> permissions change may open a potential vulnerability, but the answer
>>>>> there is just to have the permissions change earlier in the tree than
>>>>> the new parameter.
>>>>>
>>>>> Jim
>>>>>>
>>>>>> -- Hal
>>>>>>
>>>>>>> Signed-off-by: Jim Foraker <forak...@llnl.gov>
>>>>>>> ---
>>>>>>> Makefile.am | 2 +-
>>>>>>> etc/ibdiag.conf | 2 ++
>>>>>>> src/ibdiag_common.c | 2 ++
>>>>>>> 3 files changed, 5 insertions(+), 1 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Makefile.am b/Makefile.am
>>>>>>> index 950f95b..ef59bd2 100644
>>>>>>> --- a/Makefile.am
>>>>>>> +++ b/Makefile.am
>>>>>>> @@ -112,4 +112,4 @@ install-data-hook:
>>>>>>> fi
>>>>>>> $(top_srcdir)/config/install-sh -c -m 444
>>>>>>> $(top_srcdir)/scripts/IBswcountlimits.pm
>>>>>>> $(DESTDIR)/$(PERL_INSTALLDIR)/IBswcountlimits.pm
>>>>>>> $(top_srcdir)/config/install-sh -c -m 444
>>>>>>> $(top_srcdir)/etc/error_thresholds
>>>>>>> $(DESTDIR)/$(sysconfdir)/infiniband-diags
>>>>>>> - $(top_srcdir)/config/install-sh -c -m 444
>>>>>>> $(top_srcdir)/etc/ibdiag.conf $(DESTDIR)/$(sysconfdir)/infiniband-diags
>>>>>>> + $(top_srcdir)/config/install-sh -c -m 400
>>>>>>> $(top_srcdir)/etc/ibdiag.conf $(DESTDIR)/$(sysconfdir)/infiniband-diags
>>>>>>> diff --git a/etc/ibdiag.conf b/etc/ibdiag.conf
>>>>>>> index 77f3ce9..2a2334f 100644
>>>>>>> --- a/etc/ibdiag.conf
>>>>>>> +++ b/etc/ibdiag.conf
>>>>>>> @@ -15,3 +15,5 @@
>>>>>>> # Default = true
>>>>>>> #MLX_EPI=false
>>>>>>>
>>>>>>> +# define a default m_key
>>>>>>> +#m_key=0x00
>>>>>>> diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
>>>>>>> index 0901231..2089847 100644
>>>>>>> --- a/src/ibdiag_common.c
>>>>>>> +++ b/src/ibdiag_common.c
>>>>>>> @@ -155,6 +155,8 @@ void read_ibdiag_config(const char *file)
>>>>>>> } else {
>>>>>>> ibd_ibnetdisc_flags &=
>>>>>>> ~IBND_CONFIG_MLX_EPI;
>>>>>>> }
>>>>>>> + } else if (strncmp(name, "m_key", strlen("m_key")) ==
>>>>>>> 0) {
>>>>>>> + ibd_mkey = strtoull(val_str, 0, 0);
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>>> the body of a message to majord...@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
