net/smc and the RDMA core

net/smc and the RDMA core

[Christoph Hellwig]

Hi Ursual, hi netdev reviewers,

how did the smc protocol manage to get merged without any review 
on linux-rdma at all?  As the results it seems it's very substandard
in terms of RDMA API usage, e.g. it neither uses the proper CQ API
nor the RDMA R/W API, and other will probably find additional issues
as well.
--
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: net/smc and the RDMA core

[Bart Van Assche]

On Mon, 2017-05-01 at 18:33 +0200, Christoph Hellwig wrote:
> Hi Ursual, hi netdev reviewers,
> 
> how did the smc protocol manage to get merged without any review 
> on linux-rdma at all?  As the results it seems it's very substandard
> in terms of RDMA API usage, e.g. it neither uses the proper CQ API
> nor the RDMA R/W API, and other will probably find additional issues
> as well.

Hello Dave and Ursula,

It seems very rude to me to have merged the SMC protocol driver without
having involved the linux-rdma community. Anyway, I have the following
questions for Dave and Ursula:
* Since the Linux kernel is standards based: where can we find the standard
  that defines the SMC wire protocol? If this protocol has not been
  standardized yet: in what file (other than *.[ch]) in the Linux kernel
  tree has this protocol been documented?
* What are the differences between the SMC protocol, the SDP protocol and
  the rsockets protocol? How do existing implementations for these protocols
  compare to each other from a performance point of view? If no performance
  comparison between these protocols is available, shouldn't the performance
  of these protocols have been compared with each other before a review of
  the SMC driver even started?
* What are the reasons why the SDP driver was never accepted upstream? Do
  the arguments why SDP was not accepted upstream also apply to the SMC
  driver (SDP = Sockets Direct Protocol)?
* Since SMC has to be selected by specifying AF_SMC, how are users expected
  to specify whether AF_INET, AF_INET6 or yet another address family should
  be used to set up a connection between SMC
endpoints?
* Is the SMC driver limited to RoCE? Are you aware that the rsockets library
  supports multiple transport layers (RoCE, IB and iWARP)?
* Since functionality that is similar what the SMC driver provides already
  exists in user space (rsockets), why has this functionality been
  reimplemented as a kernel driver (SMC)?

Bart.

RE: net/smc and the RDMA core

[Parav Pandit]

Hi Bart, Ursula, Dave,

I am particularly concerned about SMC as address family.
It should not be treated as address family, but rather an additional protocol similar for socket type SOCK_STREAM.
While doing performance benchmarking last month and while porting few database application,

I encountered a major hurdle where user space library heavily depend on AF_INET and AF_INET6 family through get_addrinfo and other friend functions.
Adding or treating AF_SMC as AF_INET just doesn't sound right.

Most user space code doesn't care for the protocol field, but do handle domain field.

I personally believe it's not too late to modify SMC to drop expose AF_SMC and have it exposed through new protocol that can be exposed through socket() API.

Parav

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Bart Van Assche
> Sent: Monday, May 1, 2017 12:30 PM
> To: hch-jcswGhMUV9g@public.gmane.org; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; ubraun-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: net/smc and the RDMA core
> 
> On Mon, 2017-05-01 at 18:33 +0200, Christoph Hellwig wrote:
> > Hi Ursual, hi netdev reviewers,
> >
> > how did the smc protocol manage to get merged without any review on
> > linux-rdma at all?  As the results it seems it's very substandard in
> > terms of RDMA API usage, e.g. it neither uses the proper CQ API nor
> > the RDMA R/W API, and other will probably find additional issues as
> > well.
> 
> Hello Dave and Ursula,
> 
> It seems very rude to me to have merged the SMC protocol driver without
> having involved the linux-rdma community. Anyway, I have the following
> questions for Dave and Ursula:
> * Since the Linux kernel is standards based: where can we find the standard
>   that defines the SMC wire protocol? If this protocol has not been
>   standardized yet: in what file (other than *.[ch]) in the Linux kernel
>   tree has this protocol been documented?
> * What are the differences between the SMC protocol, the SDP protocol and
>   the rsockets protocol? How do existing implementations for these protocols
>   compare to each other from a performance point of view? If no performance
>   comparison between these protocols is available, shouldn't the performance
>   of these protocols have been compared with each other before a review of
>   the SMC driver even started?
> * What are the reasons why the SDP driver was never accepted upstream? Do
>   the arguments why SDP was not accepted upstream also apply to the SMC
>   driver (SDP = Sockets Direct Protocol)?
> * Since SMC has to be selected by specifying AF_SMC, how are users expected
>   to specify whether AF_INET, AF_INET6 or yet another address family should
>   be used to set up a connection between SMC endpoints?
> * Is the SMC driver limited to RoCE? Are you aware that the rsockets library
>   supports multiple transport layers (RoCE, IB and iWARP)?
> * Since functionality that is similar what the SMC driver provides already
>   exists in user space (rsockets), why has this functionality been
>   reimplemented as a kernel driver (SMC)?
> 
> Bart.--
> 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: net/smc and the RDMA core

[Steve Wise]

> 
> Hi Ursual, hi netdev reviewers,
> 
> how did the smc protocol manage to get merged without any review
> on linux-rdma at all?  As the results it seems it's very substandard
> in terms of RDMA API usage, e.g. it neither uses the proper CQ API
> nor the RDMA R/W API, and other will probably find additional issues
> as well.
> --

And it only supports RoCE (and maybe IB).

RE: net/smc and the RDMA core

[Steve Wise]

> 
> Hi Ursual, hi netdev reviewers,
> 
> how did the smc protocol manage to get merged without any review
> on linux-rdma at all?  As the results it seems it's very substandard
> in terms of RDMA API usage, e.g. it neither uses the proper CQ API
> nor the RDMA R/W API, and other will probably find additional issues
> as well.
> --

And it only supports RoCE (and maybe IB).

Re: net/smc and the RDMA core

[Ursula Braun]

On 05/01/2017 06:33 PM, Christoph Hellwig wrote:
> Hi Ursual, hi netdev reviewers,
> 
> how did the smc protocol manage to get merged without any review 
> on linux-rdma at all?  As the results it seems it's very substandard
> in terms of RDMA API usage, e.g. it neither uses the proper CQ API
> nor the RDMA R/W API, and other will probably find additional issues
> as well.
>
Hi Christoph,

We have been posting SMC-R patches on netdev since 2015, there was never 
any secrecy about it. Still sorry for omitting linux-rdma, will include 
with future postings from now on. 
Of course, we are open to any further code reviews, so if you can point 
out specific issues, we will be happy to work with you to get them 
addressed!
 
Regards, Ursula

--
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: net/smc and the RDMA core

[Ursula Braun]

On 05/01/2017 07:29 PM, Bart Van Assche wrote:
> On Mon, 2017-05-01 at 18:33 +0200, Christoph Hellwig wrote:
>> Hi Ursual, hi netdev reviewers,
>>
>> how did the smc protocol manage to get merged without any review 
>> on linux-rdma at all?  As the results it seems it's very substandard
>> in terms of RDMA API usage, e.g. it neither uses the proper CQ API
>> nor the RDMA R/W API, and other will probably find additional issues
>> as well.
> 
> Hello Dave and Ursula,
> 
> It seems very rude to me to have merged the SMC protocol driver without
> having involved the linux-rdma community. Anyway, I have the following
> questions for Dave and Ursula:
> * Since the Linux kernel is standards based: where can we find the standard
>   that defines the SMC wire protocol? If this protocol has not been
>   standardized yet: in what file (other than *.[ch]) in the Linux kernel
>   tree has this protocol been documented?

Hello Bart,

The protocol is standardized, see: http://www.rfc-editor.org/info/rfc7609.
I described this and some more protocol details in my patch series
overview mail, see for instance:
     http://marc.info/?l=linux-s390&m=148397751211964&w=2

This description explains the reasons to come up with SMC-R.

> * What are the differences between the SMC protocol, the SDP protocol and
>   the rsockets protocol? How do existing implementations for these protocols
>   compare to each other from a performance point of view? If no performance
>   comparison between these protocols is available, shouldn't the performance
>   of these protocols have been compared with each other before a review of
>   the SMC driver even started?
> * What are the reasons why the SDP driver was never accepted upstream? Do
>   the arguments why SDP was not accepted upstream also apply to the SMC
>   driver (SDP = Sockets Direct Protocol)?
> * Since SMC has to be selected by specifying AF_SMC, how are users expected
>   to specify whether AF_INET, AF_INET6 or yet another address family should
>   be used to set up a connection between SMC
> endpoints?

The IPv6 support in SMC-R is on our todo-list.

> * Is the SMC driver limited to RoCE? Are you aware that the rsockets library
>   supports multiple transport layers (RoCE, IB and iWARP)?

For now, only RoCE is supported. Other transports might be added in the future.

> * Since functionality that is similar what the SMC driver provides already
>   exists in user space (rsockets), why has this functionality been
>   reimplemented as a kernel driver (SMC)?
> 
> Bart.
> 

Regards, Ursula

--
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: net/smc and the RDMA core

[Ursula Braun]

On 05/01/2017 07:55 PM, Parav Pandit wrote:
> Hi Bart, Ursula, Dave,
> 
> I am particularly concerned about SMC as address family.
> It should not be treated as address family, but rather an additional protocol similar for socket type SOCK_STREAM.

We tried to avoid changes of the kernel TCP code. A new address family
seemed to be a feasible way to achieve this.

> While doing performance benchmarking last month and while porting few database application,
> 
> I encountered a major hurdle where user space library heavily depend on AF_INET and AF_INET6 family through get_addrinfo and other friend functions.
> Adding or treating AF_SMC as AF_INET just doesn't sound right.
> 
> Most user space code doesn't care for the protocol field, but do handle domain field.
> 
> I personally believe it's not too late to modify SMC to drop expose AF_SMC and have it exposed through new protocol that can be exposed through socket() API.
> 
> Parav
> 
>> -----Original Message-----
>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Bart Van Assche
>> Sent: Monday, May 1, 2017 12:30 PM
>> To: hch-jcswGhMUV9g@public.gmane.org; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; ubraun-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org
>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: net/smc and the RDMA core
>>
>> On Mon, 2017-05-01 at 18:33 +0200, Christoph Hellwig wrote:
>>> Hi Ursual, hi netdev reviewers,
>>>
>>> how did the smc protocol manage to get merged without any review on
>>> linux-rdma at all?  As the results it seems it's very substandard in
>>> terms of RDMA API usage, e.g. it neither uses the proper CQ API nor
>>> the RDMA R/W API, and other will probably find additional issues as
>>> well.
>>
>> Hello Dave and Ursula,
>>
>> It seems very rude to me to have merged the SMC protocol driver without
>> having involved the linux-rdma community. Anyway, I have the following
>> questions for Dave and Ursula:
>> * Since the Linux kernel is standards based: where can we find the standard
>>   that defines the SMC wire protocol? If this protocol has not been
>>   standardized yet: in what file (other than *.[ch]) in the Linux kernel
>>   tree has this protocol been documented?
>> * What are the differences between the SMC protocol, the SDP protocol and
>>   the rsockets protocol? How do existing implementations for these protocols
>>   compare to each other from a performance point of view? If no performance
>>   comparison between these protocols is available, shouldn't the performance
>>   of these protocols have been compared with each other before a review of
>>   the SMC driver even started?
>> * What are the reasons why the SDP driver was never accepted upstream? Do
>>   the arguments why SDP was not accepted upstream also apply to the SMC
>>   driver (SDP = Sockets Direct Protocol)?
>> * Since SMC has to be selected by specifying AF_SMC, how are users expected
>>   to specify whether AF_INET, AF_INET6 or yet another address family should
>>   be used to set up a connection between SMC endpoints?
>> * Is the SMC driver limited to RoCE? Are you aware that the rsockets library
>>   supports multiple transport layers (RoCE, IB and iWARP)?
>> * Since functionality that is similar what the SMC driver provides already
>>   exists in user space (rsockets), why has this functionality been
>>   reimplemented as a kernel driver (SMC)?
>>
>> Bart.--
>> 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: net/smc and the RDMA core

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 5819 bytes --]

On 5/2/2017 8:34 AM, Ursula Braun wrote:
> On 05/01/2017 07:29 PM, Bart Van Assche wrote:
>> On Mon, 2017-05-01 at 18:33 +0200, Christoph Hellwig wrote:
>>> Hi Ursual, hi netdev reviewers,
>>>
>>> how did the smc protocol manage to get merged without any review 
>>> on linux-rdma at all?  As the results it seems it's very substandard
>>> in terms of RDMA API usage, e.g. it neither uses the proper CQ API
>>> nor the RDMA R/W API, and other will probably find additional issues
>>> as well.
>>
>> Hello Dave and Ursula,
>>
>> It seems very rude to me to have merged the SMC protocol driver without
>> having involved the linux-rdma community. Anyway, I have the following
>> questions for Dave and Ursula:
>> * Since the Linux kernel is standards based: where can we find the standard
>>   that defines the SMC wire protocol? If this protocol has not been
>>   standardized yet: in what file (other than *.[ch]) in the Linux kernel
>>   tree has this protocol been documented?
> 
> Hello Bart,
> 
> The protocol is standardized, see: http://www.rfc-editor.org/info/rfc7609.

No, SMC-R is *not* standardized.  It is informationally publicized.  In
other words, you guys dumped what you considered your standard out and
rfc-editor published it under the "information" status category.  To be
an Internet Standard requires more work, and additionally requires a
period of time for people to comment on this proposal.  Given that this
is for a linux RDMA communications layer, advertising this RFC on the
linux-rdma@ mailing list seems the absolute *minimum* advertising that
should be done during the comment phase (at least as relates to the RDMA
portion of this protocol, which this protocol is first and foremost an
RDMA protocol with only TCP as a control/fallback).  It's been 18 months
since you published this, and this is the first that it's been brought
to the linux-rdma@ mailing list to my knowledge.

Anyway, the RFC is informational, not a standard, and as it stands, I'm
pretty sure the RDMA community would have a number of comments on it
before it were allowed to become a standard, including the fact that it
is currently exclusive to RoCEv1 which is, as far as the RDMA community
is concerned, a deprecated RoCE mode.  Adding a new protocol that only
supports this is just simply short sighted.  Had we been consulted
before this was merged, I have no doubt that we would have had a
considerable list of review requirements.  But, we are where we are, so
the issue is how to move forward.  Moving it to staging doesn't seem so
inappropriate in this particular case...

> I described this and some more protocol details in my patch series
> overview mail, see for instance:
>      http://marc.info/?l=linux-s390&m=148397751211964&w=2
> 
> This description explains the reasons to come up with SMC-R.
> 
>> * What are the differences between the SMC protocol, the SDP protocol and
>>   the rsockets protocol? How do existing implementations for these protocols
>>   compare to each other from a performance point of view? If no performance
>>   comparison between these protocols is available, shouldn't the performance
>>   of these protocols have been compared with each other before a review of
>>   the SMC driver even started?
>> * What are the reasons why the SDP driver was never accepted upstream? Do
>>   the arguments why SDP was not accepted upstream also apply to the SMC
>>   driver (SDP = Sockets Direct Protocol)?
>> * Since SMC has to be selected by specifying AF_SMC, how are users expected
>>   to specify whether AF_INET, AF_INET6 or yet another address family should
>>   be used to set up a connection between SMC
>> endpoints?
> 
> The IPv6 support in SMC-R is on our todo-list.
> 
>> * Is the SMC driver limited to RoCE? Are you aware that the rsockets library
>>   supports multiple transport layers (RoCE, IB and iWARP)?
> 
> For now, only RoCE is supported. Other transports might be added in the future.

We generally don't allow this type of partial support in RDMA code if we
can avoid it.  There are means in place in the RDMA subsystem to hide
the differences between the different RDMA types and we highly frown on
any code that doesn't deal with all of the fabrics.  Old code might get
grandfathered in if it had good reason for being specific to a fabric,
but not new code.  Fixing this would have been high on our list of
review items.  At a minimum supporting iWARP and all flavors of RoCE
would have required as these are all Ethernet devices at their heart and
should all support the required TCP control channel and such.  We
*might* have been more lenient on IB and OPA since they don't have a
native TCP network device, but since IPoIB works on both, even that
isn't really a reason not to support them.  The real issue is the much
more difficult issue of namespaces and SELinux, which is different
between TCP sockets and IB/OPA connections.  That would have been the
difficult part to get right, but as we haven't investigated it, we
really don't know if it would have been solvable in a reasonable fashion.

>> * Since functionality that is similar what the SMC driver provides already
>>   exists in user space (rsockets), why has this functionality been
>>   reimplemented as a kernel driver (SMC)?
>>
>> Bart.
>>
> 
> Regards, Ursula
> 
> --
> 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
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: net/smc and the RDMA core

[Bart Van Assche]

On Tue, 2017-05-02 at 14:41 +0200, Ursula Braun wrote:
> On 05/01/2017 07:55 PM, Parav Pandit wrote:
> > Hi Bart, Ursula, Dave,
> > 
> > I am particularly concerned about SMC as address family.
> > It should not be treated as address family, but rather an additional
> > protocol similar for socket type SOCK_STREAM.
> 
> We tried to avoid changes of the kernel TCP code. A new address family
> seemed to be a feasible way to achieve this.

Hello Ursula,

I agree with Parav that introducing a new address family for SMC was an
unfortunate choice. As one can see in e.g. the implementation of the SCTP
protocol no changes to the TCP implementation are needed to add support
for a new SOCK_STREAM protocol. I think the SCTP implementation uses
inet_register_protosw() to register itself dynamically.

Bart.

Re: net/smc and the RDMA core

[Bart Van Assche]

On Tue, 2017-05-02 at 14:25 +0200, Ursula Braun wrote:
> if you can point out specific issues, we will be happy to work with you
> to get them addressed!

Hello Ursula,

My list of issues that I would like to see addressed can be found below. Doug,
Christoph and others may have additional inputs. The issues that have not yet
been mentioned in other e-mails are:
- The SMC driver only supports one RDMA transport type (RoCE v1) but
  none of the other RDMA transport types (RoCE v2, IB and iWARP). New
  RDMA drivers should support all RDMA transport types transparently.
  The traditional approach to support multiple RDMA transport types is
  by using the RDMA/CM to establish connections.
- The implementation of the SMC driver only supports RoCEv1. This is
  a very unfortunate choice because:
  - RoCEv1 is not routable and hence is limited to a single Ethernet
    broadcast domain.
  - RoCEv1 packets escape a whole bunch of mechanisms that only work
    for IP packets. Firewalls pass all RoCEv1 packets and switches
    do not restrict RoCEv1 packets to a single VLAN. This means that
    if the network configuration is changed after an SMC connection
    has been set up such that IP communication between the endpoints
    of an SMC connection is blocked that the SMC RoCEv1 packets will
    not be blocked by the network equipment of which the configuration
    has just been changed.
- As already mentioned by Christoph, the SMC implementation uses RDMA
  calls that probably will be deprecated soon (ib_create_cq()) and
  should use the RDMA R/W API instead of building sge lists itself.

Bart.--
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: net/smc and the RDMA core

[Ursula Braun]

On 05/02/2017 08:39 PM, Bart Van Assche wrote:
> On Tue, 2017-05-02 at 14:25 +0200, Ursula Braun wrote:
>> if you can point out specific issues, we will be happy to work with you
>> to get them addressed!
> 
> Hello Ursula,
> 
> My list of issues that I would like to see addressed can be found below. Doug,
> Christoph and others may have additional inputs. The issues that have not yet
> been mentioned in other e-mails are:
> - The SMC driver only supports one RDMA transport type (RoCE v1) but
>   none of the other RDMA transport types (RoCE v2, IB and iWARP). New
>   RDMA drivers should support all RDMA transport types transparently.
>   The traditional approach to support multiple RDMA transport types is
>   by using the RDMA/CM to establish connections.
> - The implementation of the SMC driver only supports RoCEv1. This is
>   a very unfortunate choice because:
>   - RoCEv1 is not routable and hence is limited to a single Ethernet
>     broadcast domain.
>   - RoCEv1 packets escape a whole bunch of mechanisms that only work
>     for IP packets. Firewalls pass all RoCEv1 packets and switches
>     do not restrict RoCEv1 packets to a single VLAN. This means that
>     if the network configuration is changed after an SMC connection
>     has been set up such that IP communication between the endpoints
>     of an SMC connection is blocked that the SMC RoCEv1 packets will
>     not be blocked by the network equipment of which the configuration
>     has just been changed.
> - As already mentioned by Christoph, the SMC implementation uses RDMA
>   calls that probably will be deprecated soon (ib_create_cq()) and
>   should use the RDMA R/W API instead of building sge lists itself.
> 
> Bart.
> 
Thanks for your list, we will take care of it!

--
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: net/smc and the RDMA core

[Sagi Grimberg]

>> if you can point out specific issues, we will be happy to work with you
>> to get them addressed!
>
> Hello Ursula,
>
> My list of issues that I would like to see addressed can be found below. Doug,
> Christoph and others may have additional inputs. The issues that have not yet
> been mentioned in other e-mails are:
> - The SMC driver only supports one RDMA transport type (RoCE v1) but
>   none of the other RDMA transport types (RoCE v2, IB and iWARP). New
>   RDMA drivers should support all RDMA transport types transparently.
>   The traditional approach to support multiple RDMA transport types is
>   by using the RDMA/CM to establish connections.
> - The implementation of the SMC driver only supports RoCEv1. This is
>   a very unfortunate choice because:
>   - RoCEv1 is not routable and hence is limited to a single Ethernet
>     broadcast domain.
>   - RoCEv1 packets escape a whole bunch of mechanisms that only work
>     for IP packets. Firewalls pass all RoCEv1 packets and switches
>     do not restrict RoCEv1 packets to a single VLAN. This means that
>     if the network configuration is changed after an SMC connection
>     has been set up such that IP communication between the endpoints
>     of an SMC connection is blocked that the SMC RoCEv1 packets will
>     not be blocked by the network equipment of which the configuration
>     has just been changed.
> - As already mentioned by Christoph, the SMC implementation uses RDMA
>   calls that probably will be deprecated soon (ib_create_cq()) and
>   should use the RDMA R/W API instead of building sge lists itself.

I would also suggest that you stop exposing the DMA MR for remote
access (at least by default) and use a proper reg_mr operations with a
limited lifetime on a properly sized buffer.

Also, the cq polling code looks completely wrong, you should really
use the RDMA CQ API.

--
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: net/smc and the RDMA core

[hch-jcswGhMUV9g]

On Thu, May 04, 2017 at 11:43:50AM +0300, Sagi Grimberg wrote:
> I would also suggest that you stop exposing the DMA MR for remote
> access (at least by default) and use a proper reg_mr operations with a
> limited lifetime on a properly sized buffer.

Yes, exposing the default DMA MR is a _major_ security risk.  As soon
as SMC is enabled this will mean a remote system has full read/write
access to the local systems memory.

There іs a reason why I removed the ib_get_dma_mr function and replaced
it with the IB_PD_UNSAFE_GLOBAL_RKEY key that has _UNSAFE_ in the name
and a very long comment explaining why, and I'm really disappointed that
we got a driver merged that instead of asking on the relevant list on
why a change unexpertong a function it needed happened and instead
tried the hard way to keep a security vulnerarbility alive.
--
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: net/smc and the RDMA core

[Ursula Braun]

On 05/04/2017 10:48 AM, hch@lst.de wrote:
> On Thu, May 04, 2017 at 11:43:50AM +0300, Sagi Grimberg wrote:
>> I would also suggest that you stop exposing the DMA MR for remote
>> access (at least by default) and use a proper reg_mr operations with a
>> limited lifetime on a properly sized buffer.
> 
> Yes, exposing the default DMA MR is a _major_ security risk.  As soon
> as SMC is enabled this will mean a remote system has full read/write
> access to the local systems memory.
> 
> There іs a reason why I removed the ib_get_dma_mr function and replaced
> it with the IB_PD_UNSAFE_GLOBAL_RKEY key that has _UNSAFE_ in the name
> and a very long comment explaining why, and I'm really disappointed that
> we got a driver merged that instead of asking on the relevant list on
> why a change unexpertong a function it needed happened and instead
> tried the hard way to keep a security vulnerarbility alive.
> 
Thanks for pointing out these problems. We will address them.

Re: net/smc and the RDMA core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Thu, May 04, 2017 at 03:08:39PM +0200, Ursula Braun wrote:
>
>
> On 05/04/2017 10:48 AM, hch-jcswGhMUV9g@public.gmane.org wrote:
> > On Thu, May 04, 2017 at 11:43:50AM +0300, Sagi Grimberg wrote:
> >> I would also suggest that you stop exposing the DMA MR for remote
> >> access (at least by default) and use a proper reg_mr operations with a
> >> limited lifetime on a properly sized buffer.
> >
> > Yes, exposing the default DMA MR is a _major_ security risk.  As soon
> > as SMC is enabled this will mean a remote system has full read/write
> > access to the local systems memory.
> >
> > There іs a reason why I removed the ib_get_dma_mr function and replaced
> > it with the IB_PD_UNSAFE_GLOBAL_RKEY key that has _UNSAFE_ in the name
> > and a very long comment explaining why, and I'm really disappointed that
> > we got a driver merged that instead of asking on the relevant list on
> > why a change unexpertong a function it needed happened and instead
> > tried the hard way to keep a security vulnerarbility alive.
> >
> Thanks for pointing out these problems. We will address them.
>

Out of curiosity, when do you plan to address all this?

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: net/smc and the RDMA core

[Jason Gunthorpe]

On Thu, May 04, 2017 at 03:08:39PM +0200, Ursula Braun wrote:
> 
> 
> On 05/04/2017 10:48 AM, hch@lst.de wrote:
> > On Thu, May 04, 2017 at 11:43:50AM +0300, Sagi Grimberg wrote:
> >> I would also suggest that you stop exposing the DMA MR for remote
> >> access (at least by default) and use a proper reg_mr operations with a
> >> limited lifetime on a properly sized buffer.
> > 
> > Yes, exposing the default DMA MR is a _major_ security risk.  As soon
> > as SMC is enabled this will mean a remote system has full read/write
> > access to the local systems memory.
> > 
> > There ??s a reason why I removed the ib_get_dma_mr function and replaced
> > it with the IB_PD_UNSAFE_GLOBAL_RKEY key that has _UNSAFE_ in the name
> > and a very long comment explaining why, and I'm really disappointed that
> > we got a driver merged that instead of asking on the relevant list on
> > why a change unexpertong a function it needed happened and instead
> > tried the hard way to keep a security vulnerarbility alive.
> > 
> Thanks for pointing out these problems. We will address them.

So, you've created a huge security hole in the kernel, anyone who
loads your smc module is vunerable.

What are you going to do *RIGHT NOW* to mitigate this?

Jason

Re: net/smc and the RDMA core

[Ursula Braun]

On 05/04/2017 05:31 PM, Jason Gunthorpe wrote:
> On Thu, May 04, 2017 at 03:08:39PM +0200, Ursula Braun wrote:
>>
>>
>> On 05/04/2017 10:48 AM, hch-jcswGhMUV9g@public.gmane.org wrote:
>>> On Thu, May 04, 2017 at 11:43:50AM +0300, Sagi Grimberg wrote:
>>>> I would also suggest that you stop exposing the DMA MR for remote
>>>> access (at least by default) and use a proper reg_mr operations with a
>>>> limited lifetime on a properly sized buffer.
>>>
>>> Yes, exposing the default DMA MR is a _major_ security risk.  As soon
>>> as SMC is enabled this will mean a remote system has full read/write
>>> access to the local systems memory.
>>>
>>> There ??s a reason why I removed the ib_get_dma_mr function and replaced
>>> it with the IB_PD_UNSAFE_GLOBAL_RKEY key that has _UNSAFE_ in the name
>>> and a very long comment explaining why, and I'm really disappointed that
>>> we got a driver merged that instead of asking on the relevant list on
>>> why a change unexpertong a function it needed happened and instead
>>> tried the hard way to keep a security vulnerarbility alive.
>>>
>> Thanks for pointing out these problems. We will address them.
> 
> So, you've created a huge security hole in the kernel, anyone who
> loads your smc module is vunerable.
> 
> What are you going to do *RIGHT NOW* to mitigate this?
> 
> Jason

We do not see that just loading the smc module causes this issue.The security
risk starts with the first connection, that actually uses smc. This is only
possible if an AF_SMC socket connection is created while the so-called
pnet-table is available and offers a mapping between the used Ethernet
interface and RoCE device. Such a mapping has to be configured by a user
(via a netlink interface) and, thus, is a conscious decision by that user.

Nevertheless, thanks for all the valuable feedback; we take this security risk
seriously and addressing it is obviously at the top of our list. We're working
on this issue right now, and will post patches as soon as possible. 
 
Ursula

--
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: net/smc and the RDMA core

[Jason Gunthorpe]

On Fri, May 05, 2017 at 07:06:56PM +0200, Ursula Braun wrote:

> We do not see that just loading the smc module causes this issue.The security
> risk starts with the first connection, that actually uses smc. This is only
> possible if an AF_SMC socket connection is created while the so-called
> pnet-table is available and offers a mapping between the used Ethernet
> interface and RoCE device. Such a mapping has to be configured by a user
> (via a netlink interface) and, thus, is a conscious decision by that user.

At a mimimum this escaltes any local root exploit to a full kernel
exploit in the presense of RDMA hardware, so I do not think you should
be so dimissive of the impact.

I recommend immediately sending a kconfig patch cc'd to stable making
SMC require CONFIG_BROKEN so that nobody inadvertantly turns it on.

Jason

Re: net/smc and the RDMA core

[hch]

On Fri, May 05, 2017 at 11:10:17AM -0600, Jason Gunthorpe wrote:
> I recommend immediately sending a kconfig patch cc'd to stable making
> SMC require CONFIG_BROKEN so that nobody inadvertantly turns it on.

Yes, I'll send the patch.

Re: net/smc and the RDMA core

[Ursula Braun]

On 05/04/2017 10:48 AM, hch-jcswGhMUV9g@public.gmane.org wrote:
> On Thu, May 04, 2017 at 11:43:50AM +0300, Sagi Grimberg wrote:
>> I would also suggest that you stop exposing the DMA MR for remote
>> access (at least by default) and use a proper reg_mr operations with a
>> limited lifetime on a properly sized buffer.
> 
> Yes, exposing the default DMA MR is a _major_ security risk.  As soon
> as SMC is enabled this will mean a remote system has full read/write
> access to the local systems memory.
> 
> There іs a reason why I removed the ib_get_dma_mr function and replaced
> it with the IB_PD_UNSAFE_GLOBAL_RKEY key that has _UNSAFE_ in the name
> and a very long comment explaining why, and I'm really disappointed that
> we got a driver merged that instead of asking on the relevant list on
> why a change unexpertong a function it needed happened and instead
> tried the hard way to keep a security vulnerarbility alive.
> 

Please consider the following patch to make users aware of the
security implications through existing mechanisms.

Work on proper usage of reg_mr operations is underway, respective
patches will follow as soon as possible.

---
 net/smc/smc_core.c | 16 +++-------------
 net/smc/smc_ib.c   | 21 ++-------------------
 net/smc/smc_ib.h   |  2 --
 3 files changed, 5 insertions(+), 34 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index d52a2ee..6ec3d9a 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -613,19 +613,8 @@ int smc_rmb_create(struct smc_sock *smc)
 			rmb_desc = NULL;
 			continue; /* if mapping failed, try smaller one */
 		}
-		rc = smc_ib_get_memory_region(lgr->lnk[SMC_SINGLE_LINK].roce_pd,
-					      IB_ACCESS_REMOTE_WRITE |
-					      IB_ACCESS_LOCAL_WRITE,
-					     &rmb_desc->mr_rx[SMC_SINGLE_LINK]);
-		if (rc) {
-			smc_ib_buf_unmap(lgr->lnk[SMC_SINGLE_LINK].smcibdev,
-					 tmp_bufsize, rmb_desc,
-					 DMA_FROM_DEVICE);
-			kfree(rmb_desc->cpu_addr);
-			kfree(rmb_desc);
-			rmb_desc = NULL;
-			continue;
-		}
+		rmb_desc->mr_rx[SMC_SINGLE_LINK] =
+			lgr->lnk[SMC_SINGLE_LINK].roce_pd->__internal_mr;
 		rmb_desc->used = 1;
 		write_lock_bh(&lgr->rmbs_lock);
 		list_add(&rmb_desc->list,
@@ -668,6 +657,7 @@ int smc_rmb_rtoken_handling(struct smc_connection *conn,
 
 	for (i = 0; i < SMC_RMBS_PER_LGR_MAX; i++) {
 		if ((lgr->rtokens[i][SMC_SINGLE_LINK].rkey == rkey) &&
+		    (lgr->rtokens[i][SMC_SINGLE_LINK].dma_addr == dma_addr) &&
 		    test_bit(i, lgr->rtokens_used_mask)) {
 			conn->rtoken_idx = i;
 			return 0;
diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
index 3d0d91c..6af9ebf 100644
--- a/net/smc/smc_ib.c
+++ b/net/smc/smc_ib.c
@@ -37,24 +37,6 @@ u8 local_systemid[SMC_SYSTEMID_LEN] = SMC_LOCAL_SYSTEMID_RESET;	/* unique system
 								 * identifier
 								 */
 
-int smc_ib_get_memory_region(struct ib_pd *pd, int access_flags,
-			     struct ib_mr **mr)
-{
-	int rc;
-
-	if (*mr)
-		return 0; /* already done */
-
-	/* obtain unique key -
-	 * next invocation of get_dma_mr returns a different key!
-	 */
-	*mr = pd->device->get_dma_mr(pd, access_flags);
-	rc = PTR_ERR_OR_ZERO(*mr);
-	if (IS_ERR(*mr))
-		*mr = NULL;
-	return rc;
-}
-
 static int smc_ib_modify_qp_init(struct smc_link *lnk)
 {
 	struct ib_qp_attr qp_attr;
@@ -211,7 +193,8 @@ int smc_ib_create_protection_domain(struct smc_link *lnk)
 {
 	int rc;
 
-	lnk->roce_pd = ib_alloc_pd(lnk->smcibdev->ibdev, 0);
+	lnk->roce_pd = ib_alloc_pd(lnk->smcibdev->ibdev,
+				   IB_PD_UNSAFE_GLOBAL_RKEY);
 	rc = PTR_ERR_OR_ZERO(lnk->roce_pd);
 	if (IS_ERR(lnk->roce_pd))
 		lnk->roce_pd = NULL;
diff --git a/net/smc/smc_ib.h b/net/smc/smc_ib.h
index e5bb3c9..55c529b 100644
--- a/net/smc/smc_ib.h
+++ b/net/smc/smc_ib.h
@@ -60,8 +60,6 @@ void smc_ib_dealloc_protection_domain(struct smc_link *lnk);
 int smc_ib_create_protection_domain(struct smc_link *lnk);
 void smc_ib_destroy_queue_pair(struct smc_link *lnk);
 int smc_ib_create_queue_pair(struct smc_link *lnk);
-int smc_ib_get_memory_region(struct ib_pd *pd, int access_flags,
-			     struct ib_mr **mr);
 int smc_ib_ready_link(struct smc_link *lnk);
 int smc_ib_modify_qp_rts(struct smc_link *lnk);
 int smc_ib_modify_qp_reset(struct smc_link *lnk);
-- 

Do you think it is worth the effort for this intermediate patch?

--
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: net/smc and the RDMA core

[hch-jcswGhMUV9g]

On Thu, May 11, 2017 at 06:50:04PM +0200, Ursula Braun wrote:
> Please consider the following patch to make users aware of the
> security implications through existing mechanisms.

Any such patch would be in addition to the BROKEN marker until
there is an actual alternative.

> +		rmb_desc->mr_rx[SMC_SINGLE_LINK] =
> +			lgr->lnk[SMC_SINGLE_LINK].roce_pd->__internal_mr;

And it's named __internal_mr for reason.  The right interface to use
the unsafe rkey is to use pd->unsafe_global_rkey.
--
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