[PATCH 3/3] infiniband-diags: Add mkey support to config file

[PATCH 3/3] infiniband-diags: Add mkey support to config file

[Jim Foraker]

Support for "m_key=" parameter, plus config file
now installs user-read-only.

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 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);
 		}
 	}
 
-- 
1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Jim Foraker]

Support for "m_key=" parameter, plus config file
now installs user-read-only.

Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
---
 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);
 		}
 	}
 
-- 
1.7.1



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Hal Rosenstock]

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 ?

> plus config file now installs user-read-only.

This part is best as a separate patch.

-- Hal

> Signed-off-by: Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org>
> ---
>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Jim Foraker]

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.  We should do better down the road, but I
don't think this patch gets in the way of that.

> > 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 <foraker1-i2BcT+NCU+M@public.gmane.org>
> > ---
> >  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Hal Rosenstock]

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.

> 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.

-- 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 <foraker1-i2BcT+NCU+M@public.gmane.org>
>>> ---
>>>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Jim Foraker]

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.  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 <foraker1-i2BcT+NCU+M@public.gmane.org>
> >>> ---
> >>>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Ira Weiny]

On Thu, 15 Mar 2012 11:23:02 -0700
Jim Foraker <foraker1-i2BcT+NCU+M@public.gmane.org> 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.  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.

Actually this is "solved" via the current patch.  While "storing" the mkey in
the source port structure is less than ideal.  Using this method allows the
source port to become a context for the other calls.  Using the
smp_mkey_set/get functions allow you to change the mkey for each call without
disturbing the ABI.

That said there are ways to make this right but they involve massive changes
to this library and the diags.  I have made attempts in the past to move
things in this direction and have been met with resistance.  Until I come up
with a real solution I suggest we not belabour that point any more.

At this point for better or worse this is the interface we have.  I don't want
to bump the ABI unless _absolutely_ necessary.

Ira

> 
> > 
> > -- 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 <foraker1-i2BcT+NCU+M@public.gmane.org>
> > >>> ---
> > >>>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ira Weiny
Member of Technical Staff
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Hal Rosenstock]

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 ?

-- 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 <foraker1-i2BcT+NCU+M@public.gmane.org>
>>>>> ---
>>>>>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Ira Weiny]

On Fri, 16 Mar 2012 11:17:16 -0400
Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> 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.  Jim has been kind enough to pick up the torch.

In my opinion, the first step was to get OpenSM and the diags to correctly support a single mkey.  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.

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.

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.)  That said, in general for diagnostic purposes, having an mkey option is not a bad idea; especially on single port utilities.

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 <foraker1-i2BcT+NCU+M@public.gmane.org>
> >>>>> ---
> >>>>>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ira Weiny
Member of Technical Staff
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

[Hal Rosenstock]

On 3/16/2012 11:44 AM, Ira Weiny wrote:
> On Fri, 16 Mar 2012 11:17:16 -0400
> Hal Rosenstock <hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> 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 <foraker1-i2BcT+NCU+M@public.gmane.org>
>>>>>>> ---
>>>>>>>  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html