[PATCH 00/11] Demux IB CM requests in the rdma_cm module

[PATCH 00/11] Demux IB CM requests in the rdma_cm module

[Haggai Eran]

The rdma_cm module relies today on the ib_cm module to demux incoming
requests based on their service ID and IP address. The ib_cm module is the
wrong place to perform this task, as it can also be used with services that
do not adhere to the RDMA IP CM service as defined in the IBA
specifications. It is forced to use an opaque private data struct and mask
to compare incoming requests against.

This series moves that demux task responsibility to the rdma_cm module. The
rdma_cm module can look into the private data attached to a CM request,
containing the IP addresses related to the request. It uses the details of
the request to find the net device associated with the request, and use
that net device to find the correct listening rdma_cm_id.

The series applies against Doug's for-v4.2 tree with the patch adding a
rwsem to IB core [2] applied.

The series is structured as follows:
Patches 1-2 add the ability to lookup a network device according to the IB
device, port, P_Key, GID and IP address. They find the matching IPoIB
interfaces, and return a matching net_device if one exists.

Patches 3-5 make necessary changes in ib_cm to allow RDMA CM get the
information it needs out of CM and SIDR requests, and share a single
ib_cm_id with multiple RDMA CM listeners.

Patches 6-7 do some preliminary refactoring to the rdma_cm module. They
allow extracting information out of incoming requests instead of retrieving
them from a listening CM ID, and add helper functions to access the port
space IDRs.

Finally, patches 8-10 change rdma_cm to demultiplex requests on its own, and
patch 11 cleans up the now unneeded code in ib_cm to compare against the
private data.

This series contains a subset of the RDMA CM namespaces patches [1]. The
changes from v4 of the relevant patches are:
- Patch 1
  * in addition to the IB device, port, P_Key and IP address, pass
    also the GID, to make future IPoIB devices with alias GIDs to unique.
  * return the matching net_device instead of a network namespace.
- Patch 2: use IS_ENABLED(CONFIG_IPV6) without ifdefs.
- Patch 5:
  * rename sharecount -> listen_sharecount.
  * use a regular int instead of atomic for the share count, protected by
    the cm.lock spinlock.
  * change id destruction and shared listener creation to prevent the case
    where an id is found but it is under destruction.

[1] [PATCH v4 for-next 00/12] Add network namespace support in the RDMA-CM
    http://www.spinics.net/lists/linux-rdma/msg25244.html
[2] [PATCH for-next V5 02/12] IB/core: Add rwsem to allow reading device list or client list
    http://www.spinics.net/lists/linux-rdma/msg25931.html

Guy Shapiro (1):
  IB/ipoib: Return IPoIB devices matching connection parameters

Haggai Eran (9):
  IB/cm: Expose service ID in request events
  IB/cm: Expose DGID in SIDR request events
  IB/cm: Share listening CM IDs
  IB/cma: Refactor RDMA IP CM private-data parsing code
  IB/cma: Helper functions to access port space IDRs
  IB/cma: Add net_dev and private data checks to RDMA CM
  IB/cma: validate routing of incoming requests
  IB/cma: Share ib_cm_ids between rdma_cm_ids
  IB/cm: Remove compare_data checks

Yotam Kenneth (1):
  IB/core: Find the network device matching connection parameters

 drivers/infiniband/core/cm.c              | 204 +++++++----
 drivers/infiniband/core/cma.c             | 555 ++++++++++++++++++++++--------
 drivers/infiniband/core/device.c          |  29 ++
 drivers/infiniband/core/ucm.c             |   3 +-
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   |   2 +-
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 141 +++++++-
 drivers/infiniband/ulp/srpt/ib_srpt.c     |   2 +-
 include/rdma/ib_cm.h                      |  21 +-
 include/rdma/ib_verbs.h                   |  35 ++
 9 files changed, 765 insertions(+), 227 deletions(-)

-- 
1.7.11.2

--
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 05/11] IB/cm: Share listening CM IDs

[Haggai Eran]

Enabling network namespaces for RDMA CM will allow processes on different
namespaces to listen on the same port. In order to leave namespace support
out of the CM layer, this requires that multiple RDMA CM IDs will be able
to share a single CM ID.

This patch adds infrastructure to retrieve an existing listening ib_cm_id,
based on its device and service ID, or create a new one if one does not
already exist. It also adds a reference count for such instances
(cm_id_private.listen_sharecount), and prevents cm_destroy_id from
destroying a CM if it is still shared. See the relevant discussion [1].

[1] Re: [PATCH v3 for-next 05/13] IB/cm: Reference count ib_cm_ids
    http://www.spinics.net/lists/netdev/msg328860.html

Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Signed-off-by: Haggai Eran <haggaie@mellanox.com>
---
 drivers/infiniband/core/cm.c | 127 +++++++++++++++++++++++++++++++++++++++++--
 include/rdma/ib_cm.h         |   4 ++
 2 files changed, 125 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 46f99ec4080a..d124a891430b 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -212,6 +212,9 @@ struct cm_id_private {
 	spinlock_t lock;	/* Do not acquire inside cm.lock */
 	struct completion comp;
 	atomic_t refcount;
+	/* Number of clients sharing this ib_cm_id. Only valid for listeners.
+	 * Protected by the cm.lock spinlock. */
+	int listen_sharecount;
 
 	struct ib_mad_send_buf *msg;
 	struct cm_timewait_info *timewait_info;
@@ -722,6 +725,7 @@ struct ib_cm_id *ib_create_cm_id(struct ib_device *device,
 	INIT_LIST_HEAD(&cm_id_priv->work_list);
 	atomic_set(&cm_id_priv->work_count, -1);
 	atomic_set(&cm_id_priv->refcount, 1);
+	cm_id_priv->listen_sharecount = 1;
 	return &cm_id_priv->id;
 
 error:
@@ -847,11 +851,21 @@ retest:
 	spin_lock_irq(&cm_id_priv->lock);
 	switch (cm_id->state) {
 	case IB_CM_LISTEN:
-		cm_id->state = IB_CM_IDLE;
 		spin_unlock_irq(&cm_id_priv->lock);
+
 		spin_lock_irq(&cm.lock);
+		if (--cm_id_priv->listen_sharecount > 0) {
+			/* The id is still shared. */
+			atomic_dec(&cm_id_priv->refcount);
+			spin_unlock_irq(&cm.lock);
+			return;
+		}
 		rb_erase(&cm_id_priv->service_node, &cm.listen_service_table);
 		spin_unlock_irq(&cm.lock);
+
+		spin_lock_irq(&cm_id_priv->lock);
+		cm_id->state = IB_CM_IDLE;
+		spin_unlock_irq(&cm_id_priv->lock);
 		break;
 	case IB_CM_SIDR_REQ_SENT:
 		cm_id->state = IB_CM_IDLE;
@@ -929,11 +943,32 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id)
 }
 EXPORT_SYMBOL(ib_destroy_cm_id);
 
-int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
-		 struct ib_cm_compare_data *compare_data)
+/**
+ * __ib_cm_listen - Initiates listening on the specified service ID for
+ *   connection and service ID resolution requests.
+ * @cm_id: Connection identifier associated with the listen request.
+ * @service_id: Service identifier matched against incoming connection
+ *   and service ID resolution requests.  The service ID should be specified
+ *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
+ *   assign a service ID to the caller.
+ * @service_mask: Mask applied to service ID used to listen across a
+ *   range of service IDs.  If set to 0, the service ID is matched
+ *   exactly.  This parameter is ignored if %service_id is set to
+ *   IB_CM_ASSIGN_SERVICE_ID.
+ * @compare_data: This parameter is optional.  It specifies data that must
+ *   appear in the private data of a connection request for the specified
+ *   listen request.
+ * @lock: If set, lock the cm.lock spin-lock when adding the id to the
+ *   listener tree. When false, the caller must already hold the spin-lock,
+ *   and compare_data must be NULL.
+ */
+static int __ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
+			  __be64 service_mask,
+			  struct ib_cm_compare_data *compare_data,
+			  bool lock)
 {
 	struct cm_id_private *cm_id_priv, *cur_cm_id_priv;
-	unsigned long flags;
+	unsigned long flags = 0;
 	int ret = 0;
 
 	service_mask = service_mask ? service_mask : ~cpu_to_be64(0);
@@ -959,7 +994,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
 
 	cm_id->state = IB_CM_LISTEN;
 
-	spin_lock_irqsave(&cm.lock, flags);
+	if (lock)
+		spin_lock_irqsave(&cm.lock, flags);
 	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
 		cm_id->service_id = cpu_to_be64(cm.listen_service_id++);
 		cm_id->service_mask = ~cpu_to_be64(0);
@@ -968,7 +1004,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
 		cm_id->service_mask = service_mask;
 	}
 	cur_cm_id_priv = cm_insert_listen(cm_id_priv);
-	spin_unlock_irqrestore(&cm.lock, flags);
+	if (lock)
+		spin_unlock_irqrestore(&cm.lock, flags);
 
 	if (cur_cm_id_priv) {
 		cm_id->state = IB_CM_IDLE;
@@ -978,8 +1015,86 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
 	}
 	return ret;
 }
+
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
+		 struct ib_cm_compare_data *compare_data)
+{
+	return __ib_cm_listen(cm_id, service_id, service_mask, compare_data,
+			      true);
+}
 EXPORT_SYMBOL(ib_cm_listen);
 
+/**
+ * Create a new listening ib_cm_id and listen on the given service ID.
+ *
+ * If there's an existing ID listening on that same device and service ID,
+ * return it.
+ *
+ * @device: Device associated with the cm_id.  All related communication will
+ * be associated with the specified device.
+ * @cm_handler: Callback invoked to notify the user of CM events.
+ * @service_id: Service identifier matched against incoming connection
+ *   and service ID resolution requests.  The service ID should be specified
+ *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
+ *   assign a service ID to the caller.
+ * @service_mask: Mask applied to service ID used to listen across a
+ *   range of service IDs.  If set to 0, the service ID is matched
+ *   exactly.  This parameter is ignored if %service_id is set to
+ *   IB_CM_ASSIGN_SERVICE_ID.
+ *
+ * Callers should call ib_destroy_cm_id when done with the listener ID.
+ */
+struct ib_cm_id *ib_cm_id_create_and_listen(
+		struct ib_device *device,
+		ib_cm_handler cm_handler,
+		__be64 service_id,
+		__be64 service_mask)
+{
+	struct cm_id_private *cm_id_priv;
+	struct ib_cm_id *cm_id;
+	unsigned long flags;
+	int err = 0;
+
+	/* Create an ID in advance, since the creation may sleep */
+	cm_id = ib_create_cm_id(device, cm_handler, NULL);
+	if (IS_ERR(cm_id))
+		return cm_id;
+
+	spin_lock_irqsave(&cm.lock, flags);
+
+	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
+		goto new_id;
+
+	/* Find an existing ID */
+	cm_id_priv = cm_find_listen(device, service_id, NULL);
+	if (cm_id_priv) {
+		atomic_inc(&cm_id_priv->refcount);
+		++cm_id_priv->listen_sharecount;
+		spin_unlock_irqrestore(&cm.lock, flags);
+
+		ib_destroy_cm_id(cm_id);
+		cm_id = &cm_id_priv->id;
+		if (cm_id->cm_handler != cm_handler || cm_id->context)
+			/* Sharing an ib_cm_id with different handlers is not
+			 * supported */
+			return ERR_PTR(-EINVAL);
+		return cm_id;
+	}
+
+new_id:
+	/* Use newly created ID */
+	err = __ib_cm_listen(cm_id, service_id, service_mask, NULL, false);
+
+	spin_unlock_irqrestore(&cm.lock, flags);
+
+	if (err) {
+		ib_destroy_cm_id(cm_id);
+		return ERR_PTR(err);
+	}
+	return cm_id;
+}
+EXPORT_SYMBOL(ib_cm_id_create_and_listen);
+
 static __be64 cm_form_tid(struct cm_id_private *cm_id_priv,
 			  enum cm_msg_sequence msg_seq)
 {
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 3a5d70d79790..0dc2ff983198 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -364,6 +364,10 @@ struct ib_cm_compare_data {
 int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
 		 struct ib_cm_compare_data *compare_data);
 
+struct ib_cm_id *ib_cm_id_create_and_listen(
+		struct ib_device *device, ib_cm_handler cm_handler,
+		__be64 service_id, __be64 service_mask);
+
 struct ib_cm_req_param {
 	struct ib_sa_path_rec	*primary_path;
 	struct ib_sa_path_rec	*alternate_path;
-- 
1.7.11.2

RE: [PATCH 05/11] IB/cm: Share listening CM IDs

[Hefty, Sean]

> @@ -722,6 +725,7 @@ struct ib_cm_id *ib_create_cm_id(struct ib_device
> *device,
>  	INIT_LIST_HEAD(&cm_id_priv->work_list);
>  	atomic_set(&cm_id_priv->work_count, -1);
>  	atomic_set(&cm_id_priv->refcount, 1);
> +	cm_id_priv->listen_sharecount = 1;

This is setting the listen count before we know whether the cm_id will actually be used to listen.


>  	return &cm_id_priv->id;
> 
>  error:
> @@ -847,11 +851,21 @@ retest:
>  	spin_lock_irq(&cm_id_priv->lock);
>  	switch (cm_id->state) {
>  	case IB_CM_LISTEN:
> -		cm_id->state = IB_CM_IDLE;
>  		spin_unlock_irq(&cm_id_priv->lock);
> +
>  		spin_lock_irq(&cm.lock);
> +		if (--cm_id_priv->listen_sharecount > 0) {
> +			/* The id is still shared. */
> +			atomic_dec(&cm_id_priv->refcount);
> +			spin_unlock_irq(&cm.lock);
> +			return;
> +		}
>  		rb_erase(&cm_id_priv->service_node, &cm.listen_service_table);
>  		spin_unlock_irq(&cm.lock);
> +
> +		spin_lock_irq(&cm_id_priv->lock);
> +		cm_id->state = IB_CM_IDLE;
> +		spin_unlock_irq(&cm_id_priv->lock);

Why is the state being changed?  The cm_id is about to be freed anyway.


>  		break;
>  	case IB_CM_SIDR_REQ_SENT:
>  		cm_id->state = IB_CM_IDLE;
> @@ -929,11 +943,32 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id)
>  }
>  EXPORT_SYMBOL(ib_destroy_cm_id);
> 
> -int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64
> service_mask,
> -		 struct ib_cm_compare_data *compare_data)
> +/**
> + * __ib_cm_listen - Initiates listening on the specified service ID for
> + *   connection and service ID resolution requests.
> + * @cm_id: Connection identifier associated with the listen request.
> + * @service_id: Service identifier matched against incoming connection
> + *   and service ID resolution requests.  The service ID should be
> specified
> + *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
> + *   assign a service ID to the caller.
> + * @service_mask: Mask applied to service ID used to listen across a
> + *   range of service IDs.  If set to 0, the service ID is matched
> + *   exactly.  This parameter is ignored if %service_id is set to
> + *   IB_CM_ASSIGN_SERVICE_ID.
> + * @compare_data: This parameter is optional.  It specifies data that
> must
> + *   appear in the private data of a connection request for the specified
> + *   listen request.
> + * @lock: If set, lock the cm.lock spin-lock when adding the id to the
> + *   listener tree. When false, the caller must already hold the spin-
> lock,
> + *   and compare_data must be NULL.
> + */
> +static int __ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
> +			  __be64 service_mask,
> +			  struct ib_cm_compare_data *compare_data,
> +			  bool lock)
>  {
>  	struct cm_id_private *cm_id_priv, *cur_cm_id_priv;
> -	unsigned long flags;
> +	unsigned long flags = 0;
>  	int ret = 0;
> 
>  	service_mask = service_mask ? service_mask : ~cpu_to_be64(0);
> @@ -959,7 +994,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64
> service_id, __be64 service_mask,
> 
>  	cm_id->state = IB_CM_LISTEN;
> 
> -	spin_lock_irqsave(&cm.lock, flags);
> +	if (lock)
> +		spin_lock_irqsave(&cm.lock, flags);

I'm not a fan of this sort of locking structure.  Why not just move the locking into the outside calls completely?  I.e. move to ib_cm_listen() instead of passing in true.


>  	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
>  		cm_id->service_id = cpu_to_be64(cm.listen_service_id++);
>  		cm_id->service_mask = ~cpu_to_be64(0);
> @@ -968,7 +1004,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64
> service_id, __be64 service_mask,
>  		cm_id->service_mask = service_mask;
>  	}
>  	cur_cm_id_priv = cm_insert_listen(cm_id_priv);
> -	spin_unlock_irqrestore(&cm.lock, flags);
> +	if (lock)
> +		spin_unlock_irqrestore(&cm.lock, flags);
> 
>  	if (cur_cm_id_priv) {
>  		cm_id->state = IB_CM_IDLE;
> @@ -978,8 +1015,86 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64
> service_id, __be64 service_mask,
>  	}
>  	return ret;
>  }
> +
> +int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64
> service_mask,
> +		 struct ib_cm_compare_data *compare_data)
> +{
> +	return __ib_cm_listen(cm_id, service_id, service_mask, compare_data,
> +			      true);
> +}
>  EXPORT_SYMBOL(ib_cm_listen);
> 
> +/**
> + * Create a new listening ib_cm_id and listen on the given service ID.
> + *
> + * If there's an existing ID listening on that same device and service
> ID,
> + * return it.
> + *
> + * @device: Device associated with the cm_id.  All related communication
> will
> + * be associated with the specified device.
> + * @cm_handler: Callback invoked to notify the user of CM events.
> + * @service_id: Service identifier matched against incoming connection
> + *   and service ID resolution requests.  The service ID should be
> specified
> + *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
> + *   assign a service ID to the caller.
> + * @service_mask: Mask applied to service ID used to listen across a
> + *   range of service IDs.  If set to 0, the service ID is matched
> + *   exactly.  This parameter is ignored if %service_id is set to
> + *   IB_CM_ASSIGN_SERVICE_ID.
> + *
> + * Callers should call ib_destroy_cm_id when done with the listener ID.
> + */
> +struct ib_cm_id *ib_cm_id_create_and_listen(

Maybe ib_cm_insert_listen() instead?

> +		struct ib_device *device,
> +		ib_cm_handler cm_handler,
> +		__be64 service_id,
> +		__be64 service_mask)
> +{
> +	struct cm_id_private *cm_id_priv;
> +	struct ib_cm_id *cm_id;
> +	unsigned long flags;
> +	int err = 0;
> +
> +	/* Create an ID in advance, since the creation may sleep */
> +	cm_id = ib_create_cm_id(device, cm_handler, NULL);
> +	if (IS_ERR(cm_id))
> +		return cm_id;
> +
> +	spin_lock_irqsave(&cm.lock, flags);
> +
> +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
> +		goto new_id;
> +
> +	/* Find an existing ID */
> +	cm_id_priv = cm_find_listen(device, service_id, NULL);
> +	if (cm_id_priv) {
> +		atomic_inc(&cm_id_priv->refcount);
> +		++cm_id_priv->listen_sharecount;
> +		spin_unlock_irqrestore(&cm.lock, flags);
> +
> +		ib_destroy_cm_id(cm_id);
> +		cm_id = &cm_id_priv->id;
> +		if (cm_id->cm_handler != cm_handler || cm_id->context)
> +			/* Sharing an ib_cm_id with different handlers is not
> +			 * supported */
> +			return ERR_PTR(-EINVAL);

This leaks listen_sharecount references.


> +		return cm_id;
> +	}
> +
> +new_id:
> +	/* Use newly created ID */
> +	err = __ib_cm_listen(cm_id, service_id, service_mask, NULL, false);
> +
> +	spin_unlock_irqrestore(&cm.lock, flags);
> +
> +	if (err) {
> +		ib_destroy_cm_id(cm_id);
> +		return ERR_PTR(err);
> +	}
> +	return cm_id;
> +}
> +EXPORT_SYMBOL(ib_cm_id_create_and_listen);
--
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 05/11] IB/cm: Share listening CM IDs

[Haggai Eran]

On 16/06/2015 01:13, Hefty, Sean wrote:
>> @@ -722,6 +725,7 @@ struct ib_cm_id *ib_create_cm_id(struct ib_device
>> *device,
>>  	INIT_LIST_HEAD(&cm_id_priv->work_list);
>>  	atomic_set(&cm_id_priv->work_count, -1);
>>  	atomic_set(&cm_id_priv->refcount, 1);
>> +	cm_id_priv->listen_sharecount = 1;
> 
> This is setting the listen count before we know whether the cm_id will actually be used to listen.

Right. I'll move it to the new_id case in ib_cm_id_create_and_listen.

> 
> 
>>  	return &cm_id_priv->id;
>>
>>  error:
>> @@ -847,11 +851,21 @@ retest:
>>  	spin_lock_irq(&cm_id_priv->lock);
>>  	switch (cm_id->state) {
>>  	case IB_CM_LISTEN:
>> -		cm_id->state = IB_CM_IDLE;
>>  		spin_unlock_irq(&cm_id_priv->lock);
>> +
>>  		spin_lock_irq(&cm.lock);
>> +		if (--cm_id_priv->listen_sharecount > 0) {
>> +			/* The id is still shared. */
>> +			atomic_dec(&cm_id_priv->refcount);
>> +			spin_unlock_irq(&cm.lock);
>> +			return;
>> +		}
>>  		rb_erase(&cm_id_priv->service_node, &cm.listen_service_table);
>>  		spin_unlock_irq(&cm.lock);
>> +
>> +		spin_lock_irq(&cm_id_priv->lock);
>> +		cm_id->state = IB_CM_IDLE;
>> +		spin_unlock_irq(&cm_id_priv->lock);
> 
> Why is the state being changed?  The cm_id is about to be freed anyway.

It matches the rest of the code, but I don't think it is actually being
used for listening ids. I will drop it.

> 
> 
>>  		break;
>>  	case IB_CM_SIDR_REQ_SENT:
>>  		cm_id->state = IB_CM_IDLE;
>> @@ -929,11 +943,32 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id)
>>  }
>>  EXPORT_SYMBOL(ib_destroy_cm_id);
>>
>> -int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64
>> service_mask,
>> -		 struct ib_cm_compare_data *compare_data)
>> +/**
>> + * __ib_cm_listen - Initiates listening on the specified service ID for
>> + *   connection and service ID resolution requests.
>> + * @cm_id: Connection identifier associated with the listen request.
>> + * @service_id: Service identifier matched against incoming connection
>> + *   and service ID resolution requests.  The service ID should be
>> specified
>> + *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
>> + *   assign a service ID to the caller.
>> + * @service_mask: Mask applied to service ID used to listen across a
>> + *   range of service IDs.  If set to 0, the service ID is matched
>> + *   exactly.  This parameter is ignored if %service_id is set to
>> + *   IB_CM_ASSIGN_SERVICE_ID.
>> + * @compare_data: This parameter is optional.  It specifies data that
>> must
>> + *   appear in the private data of a connection request for the specified
>> + *   listen request.
>> + * @lock: If set, lock the cm.lock spin-lock when adding the id to the
>> + *   listener tree. When false, the caller must already hold the spin-
>> lock,
>> + *   and compare_data must be NULL.
>> + */
>> +static int __ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
>> +			  __be64 service_mask,
>> +			  struct ib_cm_compare_data *compare_data,
>> +			  bool lock)
>>  {
>>  	struct cm_id_private *cm_id_priv, *cur_cm_id_priv;
>> -	unsigned long flags;
>> +	unsigned long flags = 0;
>>  	int ret = 0;
>>
>>  	service_mask = service_mask ? service_mask : ~cpu_to_be64(0);
>> @@ -959,7 +994,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64
>> service_id, __be64 service_mask,
>>
>>  	cm_id->state = IB_CM_LISTEN;
>>
>> -	spin_lock_irqsave(&cm.lock, flags);
>> +	if (lock)
>> +		spin_lock_irqsave(&cm.lock, flags);
> 
> I'm not a fan of this sort of locking structure.  Why not just move the locking into the outside calls completely?  I.e. move to ib_cm_listen() instead of passing in true.

The reason is that this function can sleep when called compare_data !=
NULL, allocating the id's compare_data with GFP_KERNEL. But, since the
compare_data is going away in a later patch, I can actually fix the
locking at that point. I'll change the patch that removes compare_data
to also remove the lock parameter.

> 
> 
>>  	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
>>  		cm_id->service_id = cpu_to_be64(cm.listen_service_id++);
>>  		cm_id->service_mask = ~cpu_to_be64(0);
>> @@ -968,7 +1004,8 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64
>> service_id, __be64 service_mask,
>>  		cm_id->service_mask = service_mask;
>>  	}
>>  	cur_cm_id_priv = cm_insert_listen(cm_id_priv);
>> -	spin_unlock_irqrestore(&cm.lock, flags);
>> +	if (lock)
>> +		spin_unlock_irqrestore(&cm.lock, flags);
>>
>>  	if (cur_cm_id_priv) {
>>  		cm_id->state = IB_CM_IDLE;
>> @@ -978,8 +1015,86 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64
>> service_id, __be64 service_mask,
>>  	}
>>  	return ret;
>>  }
>> +
>> +int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64
>> service_mask,
>> +		 struct ib_cm_compare_data *compare_data)
>> +{
>> +	return __ib_cm_listen(cm_id, service_id, service_mask, compare_data,
>> +			      true);
>> +}
>>  EXPORT_SYMBOL(ib_cm_listen);
>>
>> +/**
>> + * Create a new listening ib_cm_id and listen on the given service ID.
>> + *
>> + * If there's an existing ID listening on that same device and service
>> ID,
>> + * return it.
>> + *
>> + * @device: Device associated with the cm_id.  All related communication
>> will
>> + * be associated with the specified device.
>> + * @cm_handler: Callback invoked to notify the user of CM events.
>> + * @service_id: Service identifier matched against incoming connection
>> + *   and service ID resolution requests.  The service ID should be
>> specified
>> + *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
>> + *   assign a service ID to the caller.
>> + * @service_mask: Mask applied to service ID used to listen across a
>> + *   range of service IDs.  If set to 0, the service ID is matched
>> + *   exactly.  This parameter is ignored if %service_id is set to
>> + *   IB_CM_ASSIGN_SERVICE_ID.
>> + *
>> + * Callers should call ib_destroy_cm_id when done with the listener ID.
>> + */
>> +struct ib_cm_id *ib_cm_id_create_and_listen(
> 
> Maybe ib_cm_insert_listen() instead?

Okay.

> 
>> +		struct ib_device *device,
>> +		ib_cm_handler cm_handler,
>> +		__be64 service_id,
>> +		__be64 service_mask)
>> +{
>> +	struct cm_id_private *cm_id_priv;
>> +	struct ib_cm_id *cm_id;
>> +	unsigned long flags;
>> +	int err = 0;
>> +
>> +	/* Create an ID in advance, since the creation may sleep */
>> +	cm_id = ib_create_cm_id(device, cm_handler, NULL);
>> +	if (IS_ERR(cm_id))
>> +		return cm_id;
>> +
>> +	spin_lock_irqsave(&cm.lock, flags);
>> +
>> +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
>> +		goto new_id;
>> +
>> +	/* Find an existing ID */
>> +	cm_id_priv = cm_find_listen(device, service_id, NULL);
>> +	if (cm_id_priv) {
>> +		atomic_inc(&cm_id_priv->refcount);
>> +		++cm_id_priv->listen_sharecount;
>> +		spin_unlock_irqrestore(&cm.lock, flags);
>> +
>> +		ib_destroy_cm_id(cm_id);
>> +		cm_id = &cm_id_priv->id;
>> +		if (cm_id->cm_handler != cm_handler || cm_id->context)
>> +			/* Sharing an ib_cm_id with different handlers is not
>> +			 * supported */
>> +			return ERR_PTR(-EINVAL);
> 
> This leaks listen_sharecount references.

Thanks. I'll fix that.

> 
> 
>> +		return cm_id;
>> +	}
>> +
>> +new_id:
>> +	/* Use newly created ID */
>> +	err = __ib_cm_listen(cm_id, service_id, service_mask, NULL, false);
>> +
>> +	spin_unlock_irqrestore(&cm.lock, flags);
>> +
>> +	if (err) {
>> +		ib_destroy_cm_id(cm_id);
>> +		return ERR_PTR(err);
>> +	}
>> +	return cm_id;
>> +}
>> +EXPORT_SYMBOL(ib_cm_id_create_and_listen);

--
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 01/11] IB/core: Find the network device matching connection parameters

[Haggai Eran]

From: Yotam Kenneth <yotamke-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In the case of IPoIB, and maybe in other cases, the network device is
managed by an upper-layer protocol (ULP). In order to expose this
network device to other users of the IB device, let ULPs implement
a callback that returns network device according to connection parameters.

The IB device and port, together with the P_Key and the GID should
be enough to uniquely identify the ULP net device. However, in current
kernels there can be multiple IPoIB interfaces created with the same GID.
Furthermore, such configuration may be desireable to support ipvlan-like
configurations for RDMA CM with IPoIB.  To resolve the device in these
cases the code will also take the IP address as an additional input.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yotam Kenneth <yotamke-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/device.c | 29 +++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h          | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index f08d438205ed..300609e8f841 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -38,6 +38,7 @@
 #include <linux/slab.h>
 #include <linux/init.h>
 #include <linux/mutex.h>
+#include <linux/netdevice.h>
 #include <rdma/rdma_netlink.h>
 
 #include "core_priv.h"
@@ -746,6 +747,34 @@ int ib_find_pkey(struct ib_device *device,
 }
 EXPORT_SYMBOL(ib_find_pkey);
 
+struct net_device *ib_get_net_dev_by_params(struct ib_device *dev,
+					    u8 port,
+					    u16 pkey,
+					    const union ib_gid *gid,
+					    const struct sockaddr *addr)
+{
+	struct net_device *net_dev = NULL;
+	struct ib_client *client;
+
+	if (!rdma_protocol_ib(dev, port))
+		return NULL;
+
+	down_read(&lists_rwsem);
+
+	list_for_each_entry(client, &client_list, list)
+		if (client->get_net_dev_by_params) {
+			net_dev = client->get_net_dev_by_params(dev, port, pkey,
+								gid, addr);
+			if (net_dev)
+				break;
+		}
+
+	up_read(&lists_rwsem);
+
+	return net_dev;
+}
+EXPORT_SYMBOL(ib_get_net_dev_by_params);
+
 static int __init ib_core_init(void)
 {
 	int ret;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 986fddb08579..03899eebf53c 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -48,6 +48,7 @@
 #include <linux/rwsem.h>
 #include <linux/scatterlist.h>
 #include <linux/workqueue.h>
+#include <linux/socket.h>
 #include <uapi/linux/if_ether.h>
 
 #include <linux/atomic.h>
@@ -1762,6 +1763,26 @@ struct ib_client {
 	void (*add)   (struct ib_device *);
 	void (*remove)(struct ib_device *);
 
+	/* Returns the net_dev belonging to this ib_client and matching the
+	 * given parameters.
+	 * @dev:	An RDMA device that the net_dev use for communication.
+	 * @port:	A physical port number on the RDMA device.
+	 * @pkey:	P_Key that the net_dev uses if applicable.
+	 * @gid:	A GID that the net_dev uses to communicate.
+	 * @addr:	An IP address the net_dev is configured with.
+	 *
+	 * An ib_client that implements a net_dev on top of RDMA devices
+	 * (such as IP over IB) should implement this callback, allowing the
+	 * rdma_cm module to find the right net_dev for a given request.
+	 *
+	 * The caller is responsible for calling dev_put on the returned
+	 * netdev. */
+	struct net_device *(*get_net_dev_by_params)(
+			struct ib_device *dev,
+			u8 port,
+			u16 pkey,
+			const union ib_gid *gid,
+			const struct sockaddr *addr);
 	struct list_head list;
 };
 
@@ -3026,4 +3047,18 @@ static inline int ib_check_mr_access(int flags)
 int ib_check_mr_status(struct ib_mr *mr, u32 check_mask,
 		       struct ib_mr_status *mr_status);
 
+/**
+ * ib_get_net_dev_by_params() - Return the appropriate net_dev
+ * for a received CM request
+ * @dev:	An RDMA device on which the request has been received.
+ * @port:	Port number on the RDMA device.
+ * @pkey:	The Pkey the request came on.
+ * @gid:	A GID that the net_dev uses to communicate.
+ * @addr:	Contains the IP address that the request specified as its
+ *		destination.
+ */
+struct net_device *ib_get_net_dev_by_params(struct ib_device *dev, u8 port,
+					    u16 pkey, const union ib_gid *gid,
+					    const struct sockaddr *addr);
+
 #endif /* IB_VERBS_H */
-- 
1.7.11.2

--
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 02/11] IB/ipoib: Return IPoIB devices matching connection parameters

[Haggai Eran]

From: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Implement the get_net_device_by_port_pkey_ip callback that returns network
device to ib_core according to connection parameters. Check the ipoib
device and iterate over all child devices to look for a match.

For each IPoIB device we iterate through all upper devices when searching
for a matching IP, in order to support bonding.

Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yotam Kenneth <yotamke-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 141 +++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index da149c278cb8..15af32665a87 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -48,6 +48,9 @@
 
 #include <linux/jhash.h>
 #include <net/arp.h>
+#include <net/addrconf.h>
+#include <linux/inetdevice.h>
+#include <rdma/ib_cache.h>
 
 #define DRV_VERSION "1.0.0"
 
@@ -91,11 +94,15 @@ struct ib_sa_client ipoib_sa_client;
 static void ipoib_add_one(struct ib_device *device);
 static void ipoib_remove_one(struct ib_device *device);
 static void ipoib_neigh_reclaim(struct rcu_head *rp);
+static struct net_device *ipoib_get_net_dev_by_params(
+		struct ib_device *dev, u8 port, u16 pkey,
+		const union ib_gid *gid, const struct sockaddr *addr);
 
 static struct ib_client ipoib_client = {
 	.name   = "ipoib",
 	.add    = ipoib_add_one,
-	.remove = ipoib_remove_one
+	.remove = ipoib_remove_one,
+	.get_net_dev_by_params = ipoib_get_net_dev_by_params,
 };
 
 int ipoib_open(struct net_device *dev)
@@ -222,6 +229,138 @@ static int ipoib_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
+/* Called with an RCU read lock taken */
+static bool ipoib_is_dev_match_addr(const struct sockaddr *addr,
+				    struct net_device *dev)
+{
+	struct net *net = dev_net(dev);
+	struct in_device *in_dev;
+	struct sockaddr_in *addr_in = (struct sockaddr_in *)addr;
+	struct sockaddr_in6 *addr_in6 = (struct sockaddr_in6 *)addr;
+	__be32 ret_addr;
+
+	switch (addr->sa_family) {
+	case AF_INET:
+		in_dev = in_dev_get(dev);
+		if (!in_dev)
+			return false;
+
+		ret_addr = inet_confirm_addr(net, in_dev, 0,
+					     addr_in->sin_addr.s_addr,
+					     RT_SCOPE_HOST);
+		in_dev_put(in_dev);
+		if (ret_addr)
+			return true;
+
+		break;
+	case AF_INET6:
+		if (IS_ENABLED(CONFIG_IPV6) &&
+		    ipv6_chk_addr(net, &addr_in6->sin6_addr, dev, 1))
+			return true;
+
+		break;
+	}
+	return false;
+}
+
+/**
+ * Find a net_device matching the given address, which is an upper device of
+ * the given net_device.
+ * @addr: IP address to look for.
+ * @dev: base IPoIB net_device
+ *
+ * If found, returns the net_device with a reference held. Otherwise return
+ * NULL.
+ */
+static struct net_device *ipoib_get_net_dev_match_addr(
+		const struct sockaddr *addr, struct net_device *dev)
+{
+	struct net_device *upper,
+			  *result = NULL;
+	struct list_head *iter;
+
+	rcu_read_lock();
+	if (ipoib_is_dev_match_addr(addr, dev)) {
+		dev_hold(dev);
+		result = dev;
+		goto out;
+	}
+
+	netdev_for_each_all_upper_dev_rcu(dev, upper, iter) {
+		if (ipoib_is_dev_match_addr(addr, upper)) {
+			dev_hold(upper);
+			result = upper;
+			break;
+		}
+	}
+out:
+	rcu_read_unlock();
+	return result;
+}
+
+/* returns an IPoIB netdev on top a given ipoib device matching a pkey_index
+ * and address, if one exists. */
+static struct net_device *ipoib_match_gid_pkey_addr(struct ipoib_dev_priv *priv,
+						    const union ib_gid *gid,
+						    u16 pkey_index,
+						    const struct sockaddr *addr)
+{
+	struct ipoib_dev_priv *child_priv;
+	struct net_device *net_dev = NULL;
+
+	if (priv->pkey_index == pkey_index &&
+	    (!gid || !memcmp(gid, &priv->local_gid, sizeof(*gid)))) {
+		net_dev = ipoib_get_net_dev_match_addr(addr, priv->dev);
+		if (net_dev)
+			return net_dev;
+	}
+
+	/* Check child interfaces */
+	down_read(&priv->vlan_rwsem);
+	list_for_each_entry(child_priv, &priv->child_intfs, list) {
+		net_dev = ipoib_match_gid_pkey_addr(child_priv, gid,
+						    pkey_index, addr);
+		if (net_dev)
+			break;
+	}
+	up_read(&priv->vlan_rwsem);
+
+	return net_dev;
+}
+
+static struct net_device *ipoib_get_net_dev_by_params(
+		struct ib_device *dev, u8 port, u16 pkey,
+		const union ib_gid *gid, const struct sockaddr *addr)
+{
+	struct ipoib_dev_priv *priv;
+	struct list_head *dev_list;
+	struct net_device *net_dev;
+	u16 pkey_index;
+	int ret;
+
+	ret = ib_find_cached_pkey(dev, port, pkey, &pkey_index);
+	if (ret)
+		return NULL;
+
+	if (!rdma_protocol_ib(dev, port))
+		return NULL;
+
+	dev_list = ib_get_client_data(dev, &ipoib_client);
+	if (!dev_list)
+		return NULL;
+
+	list_for_each_entry(priv, dev_list, list) {
+		if (priv->port != port)
+			continue;
+
+		net_dev = ipoib_match_gid_pkey_addr(priv, gid, pkey_index,
+						    addr);
+		if (net_dev)
+			return net_dev;
+	}
+	return NULL;
+}
+
 int ipoib_set_mode(struct net_device *dev, const char *buf)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
-- 
1.7.11.2

--
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 02/11] IB/ipoib: Return IPoIB devices matching connection parameters

[Jason Gunthorpe]

On Mon, Jun 15, 2015 at 11:47:07AM +0300, Haggai Eran wrote:

> +/* Called with an RCU read lock taken */

Add _rcu to the name? That is the standard convention.

> +/* returns an IPoIB netdev on top a given ipoib device matching a pkey_index
> + * and address, if one exists. */
> +static struct net_device *ipoib_match_gid_pkey_addr(struct ipoib_dev_priv *priv,
> +						    const union ib_gid *gid,
> +						    u16 pkey_index,
> +						    const struct sockaddr *addr)
> +{
> +	struct ipoib_dev_priv *child_priv;
> +	struct net_device *net_dev = NULL;
> +
> +	if (priv->pkey_index == pkey_index &&
> +	    (!gid || !memcmp(gid, &priv->local_gid, sizeof(*gid)))) {
> +		net_dev = ipoib_get_net_dev_match_addr(addr, priv->dev);
> +		if (net_dev)
> +			return net_dev;

As I said already, this should not even look at the sockaddr unless
there are multiple possible hits on the other parameters, and there
should be a comment explaining the sockaddr is only a hack to make up
for having an incomplete LLADDR.

That way people not using same guid children do not get incorrect
functionality..

> +static struct net_device *ipoib_get_net_dev_by_params(
> +		struct ib_device *dev, u8 port, u16 pkey,
> +		const union ib_gid *gid, const struct sockaddr *addr)

[..]

> +	ret = ib_find_cached_pkey(dev, port, pkey, &pkey_index);
> +	if (ret)
> +		return NULL;
> +
> +	if (!rdma_protocol_ib(dev, port))
> +		return NULL;

This if should be first I'd think.


> +	dev_list = ib_get_client_data(dev, &ipoib_client);
> +	if (!dev_list)
> +		return NULL;

Is the locking OK here? This access protected by lists_rwsem -
but for instance ib_unregister_device holds only the device_mutex when
calling client->remove, which kfree's dev_list. Looks wrong to me.

Jason
--
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 02/11] IB/ipoib: Return IPoIB devices matching connection parameters

[Haggai Eran]

On 15/06/2015 20:22, Jason Gunthorpe wrote:
> On Mon, Jun 15, 2015 at 11:47:07AM +0300, Haggai Eran wrote:
> 
>> +/* Called with an RCU read lock taken */
> 
> Add _rcu to the name? That is the standard convention.

Sure, I'll change that.

> 
>> +/* returns an IPoIB netdev on top a given ipoib device matching a pkey_index
>> + * and address, if one exists. */
>> +static struct net_device *ipoib_match_gid_pkey_addr(struct ipoib_dev_priv *priv,
>> +						    const union ib_gid *gid,
>> +						    u16 pkey_index,
>> +						    const struct sockaddr *addr)
>> +{
>> +	struct ipoib_dev_priv *child_priv;
>> +	struct net_device *net_dev = NULL;
>> +
>> +	if (priv->pkey_index == pkey_index &&
>> +	    (!gid || !memcmp(gid, &priv->local_gid, sizeof(*gid)))) {
>> +		net_dev = ipoib_get_net_dev_match_addr(addr, priv->dev);
>> +		if (net_dev)
>> +			return net_dev;
> 
> As I said already, this should not even look at the sockaddr unless
> there are multiple possible hits on the other parameters,
What is the goal here? The only difference omitting the IP check will
make is when sending a request to a matching GID but with the wrong IP.
Is it important that we pass these requests here so that they will be
dropped at the rdma_cm module?

Also, note that ipoib_get_net_dev_match_addr can return a different
net_dev from the one ipoib created. When using bonding, it will find the
IP address on the bonding device, and return the bonding net_dev instead.

> and there
> should be a comment explaining the sockaddr is only a hack to make up
> for having an incomplete LLADDR.

Sure, I'll add a comment.

> 
> That way people not using same guid children do not get incorrect
> functionality..
> 
>> +static struct net_device *ipoib_get_net_dev_by_params(
>> +		struct ib_device *dev, u8 port, u16 pkey,
>> +		const union ib_gid *gid, const struct sockaddr *addr)
> 
> [..]
> 
>> +	ret = ib_find_cached_pkey(dev, port, pkey, &pkey_index);
>> +	if (ret)
>> +		return NULL;
>> +
>> +	if (!rdma_protocol_ib(dev, port))
>> +		return NULL;
> 
> This if should be first I'd think.

Okay.

> 
> 
>> +	dev_list = ib_get_client_data(dev, &ipoib_client);
>> +	if (!dev_list)
>> +		return NULL;
> 
> Is the locking OK here? This access protected by lists_rwsem -
> but for instance ib_unregister_device holds only the device_mutex when
> calling client->remove, which kfree's dev_list. Looks wrong to me.

I think you're right. Perhaps we can switch the client data to NULL in
ib_unregister_device under the lists_rwsem. Then the
ipoib_get_net_dev_by_params call will know to skip it. The remove()
callback will need to be augmented with the client data as a parameter,
because it won't be able to retrieve it using ib_get_client_data anymore.

Haggai

Re: [PATCH 02/11] IB/ipoib: Return IPoIB devices matching connection parameters

[Haggai Eran]

On 15/06/2015 20:22, Jason Gunthorpe wrote:
> On Mon, Jun 15, 2015 at 11:47:07AM +0300, Haggai Eran wrote:
> 
>> +/* Called with an RCU read lock taken */
> 
> Add _rcu to the name? That is the standard convention.

Sure, I'll change that.

> 
>> +/* returns an IPoIB netdev on top a given ipoib device matching a pkey_index
>> + * and address, if one exists. */
>> +static struct net_device *ipoib_match_gid_pkey_addr(struct ipoib_dev_priv *priv,
>> +						    const union ib_gid *gid,
>> +						    u16 pkey_index,
>> +						    const struct sockaddr *addr)
>> +{
>> +	struct ipoib_dev_priv *child_priv;
>> +	struct net_device *net_dev = NULL;
>> +
>> +	if (priv->pkey_index == pkey_index &&
>> +	    (!gid || !memcmp(gid, &priv->local_gid, sizeof(*gid)))) {
>> +		net_dev = ipoib_get_net_dev_match_addr(addr, priv->dev);
>> +		if (net_dev)
>> +			return net_dev;
> 
> As I said already, this should not even look at the sockaddr unless
> there are multiple possible hits on the other parameters,
What is the goal here? The only difference omitting the IP check will
make is when sending a request to a matching GID but with the wrong IP.
Is it important that we pass these requests here so that they will be
dropped at the rdma_cm module?

Also, note that ipoib_get_net_dev_match_addr can return a different
net_dev from the one ipoib created. When using bonding, it will find the
IP address on the bonding device, and return the bonding net_dev instead.

> and there
> should be a comment explaining the sockaddr is only a hack to make up
> for having an incomplete LLADDR.

Sure, I'll add a comment.

> 
> That way people not using same guid children do not get incorrect
> functionality..
> 
>> +static struct net_device *ipoib_get_net_dev_by_params(
>> +		struct ib_device *dev, u8 port, u16 pkey,
>> +		const union ib_gid *gid, const struct sockaddr *addr)
> 
> [..]
> 
>> +	ret = ib_find_cached_pkey(dev, port, pkey, &pkey_index);
>> +	if (ret)
>> +		return NULL;
>> +
>> +	if (!rdma_protocol_ib(dev, port))
>> +		return NULL;
> 
> This if should be first I'd think.

Okay.

> 
> 
>> +	dev_list = ib_get_client_data(dev, &ipoib_client);
>> +	if (!dev_list)
>> +		return NULL;
> 
> Is the locking OK here? This access protected by lists_rwsem -
> but for instance ib_unregister_device holds only the device_mutex when
> calling client->remove, which kfree's dev_list. Looks wrong to me.

I think you're right. Perhaps we can switch the client data to NULL in
ib_unregister_device under the lists_rwsem. Then the
ipoib_get_net_dev_by_params call will know to skip it. The remove()
callback will need to be augmented with the client data as a parameter,
because it won't be able to retrieve it using ib_get_client_data anymore.

Haggai

[PATCH 03/11] IB/cm: Expose service ID in request events

[Haggai Eran]

Expose the service ID on an incoming CM or SIDR request to the event
handler. This will allow the RDMA CM module to de-multiplex connection
requests based on the information encoded in the service ID.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cm.c | 3 +++
 include/rdma/ib_cm.h         | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 32063add9280..c5f5f89e274a 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -1256,6 +1256,7 @@ static void cm_format_paths_from_req(struct cm_req_msg *req_msg,
 	primary_path->packet_life_time =
 		cm_req_get_primary_local_ack_timeout(req_msg);
 	primary_path->packet_life_time -= (primary_path->packet_life_time > 0);
+	primary_path->service_id = req_msg->service_id;
 
 	if (req_msg->alt_local_lid) {
 		memset(alt_path, 0, sizeof *alt_path);
@@ -1277,6 +1278,7 @@ static void cm_format_paths_from_req(struct cm_req_msg *req_msg,
 		alt_path->packet_life_time =
 			cm_req_get_alt_local_ack_timeout(req_msg);
 		alt_path->packet_life_time -= (alt_path->packet_life_time > 0);
+		alt_path->service_id = req_msg->service_id;
 	}
 }
 
@@ -2980,6 +2982,7 @@ static void cm_format_sidr_req_event(struct cm_work *work,
 	param = &work->cm_event.param.sidr_req_rcvd;
 	param->pkey = __be16_to_cpu(sidr_req_msg->pkey);
 	param->listen_id = listen_id;
+	param->service_id = sidr_req_msg->service_id;
 	param->port = work->port->port_num;
 	work->cm_event.private_data = &sidr_req_msg->private_data;
 }
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 39ed2d2fbd51..1b567bbc3ad4 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -223,6 +223,7 @@ struct ib_cm_apr_event_param {
 
 struct ib_cm_sidr_req_event_param {
 	struct ib_cm_id		*listen_id;
+	__be64			service_id;
 	u8			port;
 	u16			pkey;
 };
-- 
1.7.11.2

--
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 03/11] IB/cm: Expose service ID in request events

[Hefty, Sean]

> Expose the service ID on an incoming CM or SIDR request to the event
> handler. This will allow the RDMA CM module to de-multiplex connection
> requests based on the information encoded in the service ID.
> 
> Signed-off-by: Haggai Eran <haggaie@mellanox.com>

Acked-by: Sean Hefty <sean.hefty@intel.com>

[PATCH 04/11] IB/cm: Expose DGID in SIDR request events

[Haggai Eran]

The destination GID can be used to uniquely resolve the request. Expose the
GID in SIDR request events when it is available, so that the rdma_cm module
can use that information.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cm.c | 7 +++++++
 include/rdma/ib_cm.h         | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index c5f5f89e274a..46f99ec4080a 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -2983,6 +2983,13 @@ static void cm_format_sidr_req_event(struct cm_work *work,
 	param->pkey = __be16_to_cpu(sidr_req_msg->pkey);
 	param->listen_id = listen_id;
 	param->service_id = sidr_req_msg->service_id;
+	if (work->mad_recv_wc->wc->wc_flags & IB_WC_GRH) {
+		param->grh = 1;
+		memcpy(&param->dgid, &work->mad_recv_wc->recv_buf.grh->dgid,
+		       sizeof(param->dgid));
+	} else {
+		param->grh = 0;
+	}
 	param->port = work->port->port_num;
 	work->cm_event.private_data = &sidr_req_msg->private_data;
 }
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 1b567bbc3ad4..3a5d70d79790 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -224,6 +224,8 @@ struct ib_cm_apr_event_param {
 struct ib_cm_sidr_req_event_param {
 	struct ib_cm_id		*listen_id;
 	__be64			service_id;
+	union ib_gid		dgid;
+	u8			grh:1;
 	u8			port;
 	u16			pkey;
 };
-- 
1.7.11.2

--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Hefty, Sean]

>  drivers/infiniband/core/cm.c | 7 +++++++
>  include/rdma/ib_cm.h         | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index c5f5f89e274a..46f99ec4080a 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -2983,6 +2983,13 @@ static void cm_format_sidr_req_event(struct cm_work
> *work,
>  	param->pkey = __be16_to_cpu(sidr_req_msg->pkey);
>  	param->listen_id = listen_id;
>  	param->service_id = sidr_req_msg->service_id;
> +	if (work->mad_recv_wc->wc->wc_flags & IB_WC_GRH) {
> +		param->grh = 1;
> +		memcpy(&param->dgid, &work->mad_recv_wc->recv_buf.grh->dgid,
> +		       sizeof(param->dgid));
> +	} else {
> +		param->grh = 0;

What is the use case here?  Are you trying to sort by device?  How does the GID of the GMP relate to the listen?

--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Jason Gunthorpe]

On Mon, Jun 15, 2015 at 09:32:53PM +0000, Hefty, Sean wrote:
> >  drivers/infiniband/core/cm.c | 7 +++++++
> >  include/rdma/ib_cm.h         | 2 ++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> > index c5f5f89e274a..46f99ec4080a 100644
> > +++ b/drivers/infiniband/core/cm.c
> > @@ -2983,6 +2983,13 @@ static void cm_format_sidr_req_event(struct cm_work
> > *work,
> >  	param->pkey = __be16_to_cpu(sidr_req_msg->pkey);
> >  	param->listen_id = listen_id;
> >  	param->service_id = sidr_req_msg->service_id;
> > +	if (work->mad_recv_wc->wc->wc_flags & IB_WC_GRH) {
> > +		param->grh = 1;
> > +		memcpy(&param->dgid, &work->mad_recv_wc->recv_buf.grh->dgid,
> > +		       sizeof(param->dgid));
> > +	} else {
> > +		param->grh = 0;
> 
> What is the use case here?  Are you trying to sort by device?  How does the GID of the GMP relate to the listen?

Ouch, that is getting fugly, it used by cma_save_req_info, which feeds
the data into ib_get_net_dev_by_params - basically it chooses the
alias GUID'd netdev to use.

But how is that going to work? How is the sender to know it should be
sending a GRH with the CM message?

Falling back to use the primary_path sgid seems like a poor
substitute, if APM is being used that might be a totally different
port than the CM message.

I'm also not sure about the pkey, it seems to me that the pkey used to
select the ingress netdevice should be the pkey of the rx'd CM GMP,
not the pkey of the future RDMA channel, so this looks like it should
change:

+static int cma_save_req_info(const struct ib_cm_event *ib_event,
+			     struct cma_req_info *req)
[..]
+	switch (ib_event->event) {
+	case IB_CM_REQ_RECEIVED:
+		req->pkey	= be16_to_cpu(req_param->primary_path->pkey);
[..]
+	case IB_CM_SIDR_REQ_RECEIVED:
+		req->pkey	= sidr_param->pkey;

Some comment for the GID, if the GRH is present, then the DGID from
there should alwas be used, not the content of the REQ.

All this is because the CM IP protocol didn't include the LLADDR of
the target's IPoIB interface.. If we are already looking at a breaking
change to force GRH, how hard would it be to add on the LLADDR
somehow instead?

Jason
--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Haggai Eran]

On 16/06/2015 01:08, Jason Gunthorpe wrote:
> On Mon, Jun 15, 2015 at 09:32:53PM +0000, Hefty, Sean wrote:
>>>  drivers/infiniband/core/cm.c | 7 +++++++
>>>  include/rdma/ib_cm.h         | 2 ++
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>>> index c5f5f89e274a..46f99ec4080a 100644
>>> +++ b/drivers/infiniband/core/cm.c
>>> @@ -2983,6 +2983,13 @@ static void cm_format_sidr_req_event(struct cm_work
>>> *work,
>>>  	param->pkey = __be16_to_cpu(sidr_req_msg->pkey);
>>>  	param->listen_id = listen_id;
>>>  	param->service_id = sidr_req_msg->service_id;
>>> +	if (work->mad_recv_wc->wc->wc_flags & IB_WC_GRH) {
>>> +		param->grh = 1;
>>> +		memcpy(&param->dgid, &work->mad_recv_wc->recv_buf.grh->dgid,
>>> +		       sizeof(param->dgid));
>>> +	} else {
>>> +		param->grh = 0;
>>
>> What is the use case here?  Are you trying to sort by device?  How does the GID of the GMP relate to the listen?
> 
> Ouch, that is getting fugly, it used by cma_save_req_info, which feeds
> the data into ib_get_net_dev_by_params - basically it chooses the
> alias GUID'd netdev to use.
> 
> But how is that going to work? How is the sender to know it should be
> sending a GRH with the CM message?

If the admin wants to use SIDR with alias GIDs, they will need to
configure the system to enable GRH for such GMPs. (This series doesn't
include such a patch).

> 
> Falling back to use the primary_path sgid seems like a poor
> substitute, if APM is being used that might be a totally different
> port than the CM message.

Note that the patchset uses primary_path for CM requests always (not as
a fallback), and uses GRH as a fallback for SIDR requests.

Regarding APM, currently the ib_cm code always sends the GMP to the
primary path anyway, right? And in any case, one would expect the
primary path's GID to have a valid net_device and local routing rules,
so I think for the purpose of demuxing and validating the request using
the primary path should be fine.

> 
> I'm also not sure about the pkey, it seems to me that the pkey used to
> select the ingress netdevice should be the pkey of the rx'd CM GMP,
> not the pkey of the future RDMA channel, so this looks like it should
> change:
> 
> +static int cma_save_req_info(const struct ib_cm_event *ib_event,
> +			     struct cma_req_info *req)
> [..]
> +	switch (ib_event->event) {
> +	case IB_CM_REQ_RECEIVED:
> +		req->pkey	= be16_to_cpu(req_param->primary_path->pkey);
> [..]
> +	case IB_CM_SIDR_REQ_RECEIVED:
> +		req->pkey	= sidr_param->pkey;
> 
> Some comment for the GID, if the GRH is present, then the DGID from
> there should alwas be used, not the content of the REQ.

Why do you think the GMP's net_device should be used over the one of the
future RDMA channel?

Thinking about the network namespaces implications, I'm having trouble
thinking of a good use case where a port redirector is in one namespace
and the future RDMA channel is in another namespace.

> 
> All this is because the CM IP protocol didn't include the LLADDR of
> the target's IPoIB interface.. If we are already looking at a breaking
> change to force GRH, how hard would it be to add on the LLADDR
> somehow instead?

So far we can work without GRH for CM requests, and also without GRH for
SIDR requests if we rely on layer 3 for the interface resolution. I'm
not against adding a LLADDR to the protocol somehow, but I don't think
we should abandon all these use cases and the interoperability with
existing software.

Regards,
Haggai
--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Jason Gunthorpe]

On Tue, Jun 16, 2015 at 02:25:07PM +0300, Haggai Eran wrote:

> > But how is that going to work? How is the sender to know it should be
> > sending a GRH with the CM message?
> 
> If the admin wants to use SIDR with alias GIDs, they will need to
> configure the system to enable GRH for such GMPs. (This series doesn't
> include such a patch).

Gross.

> Regarding APM, currently the ib_cm code always sends the GMP to the
> primary path anyway, right? And in any case, one would expect the
> primary path's GID to have a valid net_device and local routing rules,
> so I think for the purpose of demuxing and validating the request using
> the primary path should be fine.

The current code works that way, but it is not what I'd expect
generally.

For instance, future APM support will be able to drive dual-rail and
policy will decide which rail is the current best rail for data
transfer. So the GMP may be directed to the IPoIB device with port 1,
but the data transfer may happen on the RDMA port 2. [Note, I already
have very rough patches that do this de-coupling]

> Why do you think the GMP's net_device should be used over the one of the
> future RDMA channel?

The code needs to match the incoming GMP with the logical netdev that
rx's *that GMP*. The fact that goes on to setup an RDMA channel is not
relevant, the nature of the future RDMA channel should not impact how
the GMP is recieved.

> So far we can work without GRH for CM requests, and also without GRH for
> SIDR requests if we rely on layer 3 for the interface resolution. I'm
> not against adding a LLADDR to the protocol somehow, but I don't think
> we should abandon all these use cases and the interoperability with
> existing software.

Well, there is a middle ground. Lets say we get the LLADDR in the GMP
somehow, then we get 100% correct operation when it is present.

For degraded operation we have the (device,port,pkey) and possibly
(device,port,pkey,gid) if there was a GRH. We also have the IP address
hack.

So, I'd say, search in this sequence:
 - If the LLADDR is present, just find the right netdev
 - Otherwise search for (device,port,pkey) / (device,port,pkey,gid)
   If there is only one match then that is unambiguously the correct
   device to use.
 - Repeat the above search, but add the IP address. This is the hack
   we perform for compatibility.

There is no reason to hackily look at the GMP path parameters if we are
relying on #3 above as the hack to save us in the legacy ambiguous case.

.. and to answer your question in the other email, I think we should
keep the hack clearly distinct from the proper operation (in fact,
perhaps it should be user configurable). So #3 should be a distinct
step taken when all else fails, not integrated into earlier steps.

So, this series as it stands just needs to do #2/#3 above and you guys
can figure out the LLADDR business later.

Jason

Re: [PATCH 04/11] IB/cm: Expose DGID in SIDR request events

[Haggai Eran]

On 17/06/2015 20:06, Jason Gunthorpe wrote:
> On Tue, Jun 16, 2015 at 02:25:07PM +0300, Haggai Eran wrote:

>> Regarding APM, currently the ib_cm code always sends the GMP to the
>> primary path anyway, right? And in any case, one would expect the
>> primary path's GID to have a valid net_device and local routing rules,
>> so I think for the purpose of demuxing and validating the request using
>> the primary path should be fine.
> 
> The current code works that way, but it is not what I'd expect
> generally.
> 
> For instance, future APM support will be able to drive dual-rail and
> policy will decide which rail is the current best rail for data
> transfer. So the GMP may be directed to the IPoIB device with port 1,
> but the data transfer may happen on the RDMA port 2. [Note, I already
> have very rough patches that do this de-coupling]
> 
>> Why do you think the GMP's net_device should be used over the one of the
>> future RDMA channel?
> 
> The code needs to match the incoming GMP with the logical netdev that
> rx's *that GMP*. The fact that goes on to setup an RDMA channel is not
> relevant, the nature of the future RDMA channel should not impact how
> the GMP is recieved.

>From what I understand, ib_cm and rdma_cm keeps their own addresses. I
thought that ib_cm's addresses would be used to handle GMPs, and the
rdma_cm addresses (id.route.addr) to represent the created RDMA channel.
After all, that is what ucma_query_addr returns. So are you proposing
that we use the logical netdev that was resolved by the GMP to fill up
the source address returned to user-space? It sounds like it would
prevent the APM usage you described above.

> 
>> So far we can work without GRH for CM requests, and also without GRH for
>> SIDR requests if we rely on layer 3 for the interface resolution. I'm
>> not against adding a LLADDR to the protocol somehow, but I don't think
>> we should abandon all these use cases and the interoperability with
>> existing software.
> 
> Well, there is a middle ground. Lets say we get the LLADDR in the GMP
> somehow, then we get 100% correct operation when it is present.
> 
> For degraded operation we have the (device,port,pkey) and possibly
> (device,port,pkey,gid) if there was a GRH. We also have the IP address
> hack.
> 
> So, I'd say, search in this sequence:
>  - If the LLADDR is present, just find the right netdev
>  - Otherwise search for (device,port,pkey) / (device,port,pkey,gid)
>    If there is only one match then that is unambiguously the correct
>    device to use.
>  - Repeat the above search, but add the IP address. This is the hack
>    we perform for compatibility.
> 
> There is no reason to hackily look at the GMP path parameters if we are
> relying on #3 above as the hack to save us in the legacy ambiguous case.
> 
> .. and to answer your question in the other email, I think we should
> keep the hack clearly distinct from the proper operation (in fact,
> perhaps it should be user configurable). So #3 should be a distinct
> step taken when all else fails, not integrated into earlier steps.
> 
> So, this series as it stands just needs to do #2/#3 above and you guys
> can figure out the LLADDR business later.

Okay. I can add a first search without the IP address.

--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Haggai Eran]

On 16/06/2015 00:32, Hefty, Sean wrote:
>>  drivers/infiniband/core/cm.c | 7 +++++++
>>  include/rdma/ib_cm.h         | 2 ++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index c5f5f89e274a..46f99ec4080a 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -2983,6 +2983,13 @@ static void cm_format_sidr_req_event(struct cm_work
>> *work,
>>  	param->pkey = __be16_to_cpu(sidr_req_msg->pkey);
>>  	param->listen_id = listen_id;
>>  	param->service_id = sidr_req_msg->service_id;
>> +	if (work->mad_recv_wc->wc->wc_flags & IB_WC_GRH) {
>> +		param->grh = 1;
>> +		memcpy(&param->dgid, &work->mad_recv_wc->recv_buf.grh->dgid,
>> +		       sizeof(param->dgid));
>> +	} else {
>> +		param->grh = 0;
> 
> What is the use case here?  Are you trying to sort by device?  How does the GID of the GMP relate to the listen?

The idea is to allow SIDR request to be sorted by the GID, when we will
have alias GIDs for IPoIB.

Unlike the CM requests, SIDR requests do not contain the remote GID, so
I thought we could use the GID from the GRH and turn on GRH on such systems.

--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Hefty, Sean]

> The idea is to allow SIDR request to be sorted by the GID, when we will
> have alias GIDs for IPoIB.

Please limit this series, or at least the early patches in this series, to simply moving the de-mux out of the ib_cm and into the rdma_cm.
--
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 04/11] IB/cm: Expose DGID in SIDR request events

[Haggai Eran]

On Tuesday, June 16, 2015 7:47 PM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> To: Haggai Eran; Doug Ledford
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Liran Liss; Guy Shapiro; Shachar Raindel; Yotam Kenneth; Jason Gunthorpe
> Subject: RE: [PATCH 04/11] IB/cm: Expose DGID in SIDR request events
> 
>> The idea is to allow SIDR request to be sorted by the GID, when we will
>> have alias GIDs for IPoIB.
> 
> Please limit this series, or at least the early patches in this series, to simply moving the de-mux out of the ib_cm and into the rdma_cm.

Fair enough. I'll remove this patch and the related code from this series.

Haggai
--
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 06/11] IB/cma: Refactor RDMA IP CM private-data parsing code

[Haggai Eran]

When receiving a connection request, rdma_cm needs to associate the request
with a network device, in order to disambiguate requests. To do this, it
needs to know the request's destination IP. For this the module needs to
allow getting this information from the private data in the request packet,
instead of relying on the information already being in the listening RDMA
CM ID.

When creating a new incoming connection ID, the code in
cma_save_ip{4,6}_info can no longer rely on the listener's private data to
find the port number, so it reads it from the requested service ID.

Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yotam Kenneth <yotamke-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 150 ++++++++++++++++++++++++++----------------
 1 file changed, 92 insertions(+), 58 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 3b943b700a63..bc061987485d 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -870,97 +870,122 @@ static inline int cma_any_port(struct sockaddr *addr)
 	return !cma_port(addr);
 }
 
-static void cma_save_ib_info(struct rdma_cm_id *id, struct rdma_cm_id *listen_id,
+static void cma_save_ib_info(struct sockaddr *src_addr,
+			     struct sockaddr *dst_addr,
 			     struct ib_sa_path_rec *path)
 {
-	struct sockaddr_ib *listen_ib, *ib;
+	struct sockaddr_ib *ib;
 
-	listen_ib = (struct sockaddr_ib *) &listen_id->route.addr.src_addr;
-	ib = (struct sockaddr_ib *) &id->route.addr.src_addr;
-	ib->sib_family = listen_ib->sib_family;
-	ib->sib_pkey = path->pkey;
-	ib->sib_flowinfo = path->flow_label;
-	memcpy(&ib->sib_addr, &path->sgid, 16);
-	ib->sib_sid = listen_ib->sib_sid;
-	ib->sib_sid_mask = cpu_to_be64(0xffffffffffffffffULL);
-	ib->sib_scope_id = listen_ib->sib_scope_id;
-
-	ib = (struct sockaddr_ib *) &id->route.addr.dst_addr;
-	ib->sib_family = listen_ib->sib_family;
-	ib->sib_pkey = path->pkey;
-	ib->sib_flowinfo = path->flow_label;
-	memcpy(&ib->sib_addr, &path->dgid, 16);
-}
-
-static __be16 ss_get_port(const struct sockaddr_storage *ss)
-{
-	if (ss->ss_family == AF_INET)
-		return ((struct sockaddr_in *)ss)->sin_port;
-	else if (ss->ss_family == AF_INET6)
-		return ((struct sockaddr_in6 *)ss)->sin6_port;
-	BUG();
+	if (src_addr) {
+		ib = (struct sockaddr_ib *)src_addr;
+		ib->sib_family = AF_IB;
+		ib->sib_pkey = path->pkey;
+		ib->sib_flowinfo = path->flow_label;
+		memcpy(&ib->sib_addr, &path->sgid, 16);
+		ib->sib_sid = path->service_id;
+		ib->sib_sid_mask = cpu_to_be64(0xffffffffffffffffULL);
+		ib->sib_scope_id = 0;
+	}
+	if (dst_addr) {
+		ib = (struct sockaddr_ib *)dst_addr;
+		ib->sib_family = AF_IB;
+		ib->sib_pkey = path->pkey;
+		ib->sib_flowinfo = path->flow_label;
+		memcpy(&ib->sib_addr, &path->dgid, 16);
+	}
 }
 
-static void cma_save_ip4_info(struct rdma_cm_id *id, struct rdma_cm_id *listen_id,
-			      struct cma_hdr *hdr)
+static void cma_save_ip4_info(struct sockaddr *src_addr,
+			      struct sockaddr *dst_addr,
+			      struct cma_hdr *hdr,
+			      __be16 local_port)
 {
 	struct sockaddr_in *ip4;
 
-	ip4 = (struct sockaddr_in *) &id->route.addr.src_addr;
-	ip4->sin_family = AF_INET;
-	ip4->sin_addr.s_addr = hdr->dst_addr.ip4.addr;
-	ip4->sin_port = ss_get_port(&listen_id->route.addr.src_addr);
+	if (src_addr) {
+		ip4 = (struct sockaddr_in *)src_addr;
+		ip4->sin_family = AF_INET;
+		ip4->sin_addr.s_addr = hdr->dst_addr.ip4.addr;
+		ip4->sin_port = local_port;
+	}
 
-	ip4 = (struct sockaddr_in *) &id->route.addr.dst_addr;
-	ip4->sin_family = AF_INET;
-	ip4->sin_addr.s_addr = hdr->src_addr.ip4.addr;
-	ip4->sin_port = hdr->port;
+	if (dst_addr) {
+		ip4 = (struct sockaddr_in *)dst_addr;
+		ip4->sin_family = AF_INET;
+		ip4->sin_addr.s_addr = hdr->src_addr.ip4.addr;
+		ip4->sin_port = hdr->port;
+	}
 }
 
-static void cma_save_ip6_info(struct rdma_cm_id *id, struct rdma_cm_id *listen_id,
-			      struct cma_hdr *hdr)
+static void cma_save_ip6_info(struct sockaddr *src_addr,
+			      struct sockaddr *dst_addr,
+			      struct cma_hdr *hdr,
+			      __be16 local_port)
 {
 	struct sockaddr_in6 *ip6;
 
-	ip6 = (struct sockaddr_in6 *) &id->route.addr.src_addr;
-	ip6->sin6_family = AF_INET6;
-	ip6->sin6_addr = hdr->dst_addr.ip6;
-	ip6->sin6_port = ss_get_port(&listen_id->route.addr.src_addr);
+	if (src_addr) {
+		ip6 = (struct sockaddr_in6 *)src_addr;
+		ip6->sin6_family = AF_INET6;
+		ip6->sin6_addr = hdr->dst_addr.ip6;
+		ip6->sin6_port = local_port;
+	}
 
-	ip6 = (struct sockaddr_in6 *) &id->route.addr.dst_addr;
-	ip6->sin6_family = AF_INET6;
-	ip6->sin6_addr = hdr->src_addr.ip6;
-	ip6->sin6_port = hdr->port;
+	if (dst_addr) {
+		ip6 = (struct sockaddr_in6 *)dst_addr;
+		ip6->sin6_family = AF_INET6;
+		ip6->sin6_addr = hdr->src_addr.ip6;
+		ip6->sin6_port = hdr->port;
+	}
 }
 
-static int cma_save_net_info(struct rdma_cm_id *id, struct rdma_cm_id *listen_id,
-			     struct ib_cm_event *ib_event)
+static u16 cma_port_from_service_id(__be64 service_id)
 {
-	struct cma_hdr *hdr;
+	return be64_to_cpu(service_id);
+}
 
-	if ((listen_id->route.addr.src_addr.ss_family == AF_IB) &&
-	    (ib_event->event == IB_CM_REQ_RECEIVED)) {
-		cma_save_ib_info(id, listen_id, ib_event->param.req_rcvd.primary_path);
-		return 0;
-	}
+static int cma_save_ip_info(struct sockaddr *src_addr,
+			    struct sockaddr *dst_addr,
+			    struct ib_cm_event *ib_event,
+			    __be64 service_id)
+{
+	struct cma_hdr *hdr;
+	__be16 port;
 
 	hdr = ib_event->private_data;
 	if (hdr->cma_version != CMA_VERSION)
 		return -EINVAL;
 
+	port = htons(cma_port_from_service_id(service_id));
+
 	switch (cma_get_ip_ver(hdr)) {
 	case 4:
-		cma_save_ip4_info(id, listen_id, hdr);
+		cma_save_ip4_info(src_addr, dst_addr, hdr, port);
 		break;
 	case 6:
-		cma_save_ip6_info(id, listen_id, hdr);
+		cma_save_ip6_info(src_addr, dst_addr, hdr, port);
 		break;
 	default:
 		return -EINVAL;
 	}
+
 	return 0;
 }
 
+static int cma_save_net_info(struct sockaddr *src_addr,
+			     struct sockaddr *dst_addr,
+			     struct ib_cm_event *ib_event,
+			     sa_family_t sa_family, __be64 service_id)
+{
+	if (sa_family == AF_IB && ib_event->event == IB_CM_REQ_RECEIVED) {
+		cma_save_ib_info(src_addr, dst_addr,
+				 ib_event->param.req_rcvd.primary_path);
+		return 0;
+	}
+
+	return cma_save_ip_info(src_addr, dst_addr, ib_event, service_id);
+}
+
 static inline int cma_user_data_offset(struct rdma_id_private *id_priv)
 {
 	return cma_family(id_priv) == AF_IB ? 0 : sizeof(struct cma_hdr);
@@ -1211,6 +1236,9 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
 	struct rdma_id_private *id_priv;
 	struct rdma_cm_id *id;
 	struct rdma_route *rt;
+	const sa_family_t ss_family = listen_id->route.addr.src_addr.ss_family;
+	const __be64 service_id =
+		      ib_event->param.req_rcvd.primary_path->service_id;
 	int ret;
 
 	id = rdma_create_id(listen_id->event_handler, listen_id->context,
@@ -1219,7 +1247,9 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
 		return NULL;
 
 	id_priv = container_of(id, struct rdma_id_private, id);
-	if (cma_save_net_info(id, listen_id, ib_event))
+	if (cma_save_net_info((struct sockaddr *)&id->route.addr.src_addr,
+			      (struct sockaddr *)&id->route.addr.dst_addr,
+			      ib_event, ss_family, service_id))
 		goto err;
 
 	rt = &id->route;
@@ -1265,7 +1295,11 @@ static struct rdma_id_private *cma_new_udp_id(struct rdma_cm_id *listen_id,
 		return NULL;
 
 	id_priv = container_of(id, struct rdma_id_private, id);
-	if (cma_save_net_info(id, listen_id, ib_event))
+	if (cma_save_net_info((struct sockaddr *)&id->route.addr.src_addr,
+			      (struct sockaddr *)&id->route.addr.dst_addr,
+			      ib_event,
+			      listen_id->route.addr.src_addr.ss_family,
+			      ib_event->param.sidr_req_rcvd.service_id))
 		goto err;
 
 	if (!cma_any_addr((struct sockaddr *) &id->route.addr.src_addr)) {
-- 
1.7.11.2

--
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 06/11] IB/cma: Refactor RDMA IP CM private-data parsing code

[Hefty, Sean]

> -static int cma_save_net_info(struct rdma_cm_id *id, struct rdma_cm_id
> *listen_id,
> -			     struct ib_cm_event *ib_event)
> +static u16 cma_port_from_service_id(__be64 service_id)
>  {
> -	struct cma_hdr *hdr;
> +	return be64_to_cpu(service_id);
> +}

Nit - Does the compiler not complain about the cast from u64 to u16?

--
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 06/11] IB/cma: Refactor RDMA IP CM private-data parsing code

[Haggai Eran]

On 16/06/2015 01:33, Hefty, Sean wrote:
>> -static int cma_save_net_info(struct rdma_cm_id *id, struct rdma_cm_id
>> *listen_id,
>> -			     struct ib_cm_event *ib_event)
>> +static u16 cma_port_from_service_id(__be64 service_id)
>>  {
>> -	struct cma_hdr *hdr;
>> +	return be64_to_cpu(service_id);
>> +}
> 
> Nit - Does the compiler not complain about the cast from u64 to u16?
> 

Apparently it does, but only with W=3 (-Wconversion is included there).
W=3 produces about 6k warnings when compiling cma.c, so I don't usually
enable it.

I'll add a cast there to prevent the warning.

Haggai
--
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 07/11] IB/cma: Helper functions to access port space IDRs

[Haggai Eran]

Add helper functions to access the IDRs by port-space and port number.

Pass around the port-space enum in cma.c instead of using pointers to
port-space IDRs.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yotam Kenneth <yotamke-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Shachar Raindel <raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Guy Shapiro <guysh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 81 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index bc061987485d..bb90d462f819 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -113,6 +113,22 @@ static DEFINE_IDR(udp_ps);
 static DEFINE_IDR(ipoib_ps);
 static DEFINE_IDR(ib_ps);
 
+static struct idr *cma_idr(enum rdma_port_space ps)
+{
+	switch (ps) {
+	case RDMA_PS_TCP:
+		return &tcp_ps;
+	case RDMA_PS_UDP:
+		return &udp_ps;
+	case RDMA_PS_IPOIB:
+		return &ipoib_ps;
+	case RDMA_PS_IB:
+		return &ib_ps;
+	default:
+		return NULL;
+	}
+}
+
 struct cma_device {
 	struct list_head	list;
 	struct ib_device	*device;
@@ -122,11 +138,33 @@ struct cma_device {
 };
 
 struct rdma_bind_list {
-	struct idr		*ps;
+	enum rdma_port_space	ps;
 	struct hlist_head	owners;
 	unsigned short		port;
 };
 
+static int cma_ps_alloc(enum rdma_port_space ps,
+			struct rdma_bind_list *bind_list, int snum)
+{
+	struct idr *idr = cma_idr(ps);
+
+	return idr_alloc(idr, bind_list, snum, snum + 1, GFP_KERNEL);
+}
+
+static struct rdma_bind_list *cma_ps_find(enum rdma_port_space ps, int snum)
+{
+	struct idr *idr = cma_idr(ps);
+
+	return idr_find(idr, snum);
+}
+
+static void cma_ps_remove(enum rdma_port_space ps, int snum)
+{
+	struct idr *idr = cma_idr(ps);
+
+	idr_remove(idr, snum);
+}
+
 enum {
 	CMA_OPTION_AFONLY,
 };
@@ -1053,7 +1091,7 @@ static void cma_release_port(struct rdma_id_private *id_priv)
 	mutex_lock(&lock);
 	hlist_del(&id_priv->node);
 	if (hlist_empty(&bind_list->owners)) {
-		idr_remove(bind_list->ps, bind_list->port);
+		cma_ps_remove(bind_list->ps, bind_list->port);
 		kfree(bind_list);
 	}
 	mutex_unlock(&lock);
@@ -2349,8 +2387,8 @@ static void cma_bind_port(struct rdma_bind_list *bind_list,
 	hlist_add_head(&id_priv->node, &bind_list->owners);
 }
 
-static int cma_alloc_port(struct idr *ps, struct rdma_id_private *id_priv,
-			  unsigned short snum)
+static int cma_alloc_port(enum rdma_port_space ps,
+			  struct rdma_id_private *id_priv, unsigned short snum)
 {
 	struct rdma_bind_list *bind_list;
 	int ret;
@@ -2359,7 +2397,7 @@ static int cma_alloc_port(struct idr *ps, struct rdma_id_private *id_priv,
 	if (!bind_list)
 		return -ENOMEM;
 
-	ret = idr_alloc(ps, bind_list, snum, snum + 1, GFP_KERNEL);
+	ret = cma_ps_alloc(ps, bind_list, snum);
 	if (ret < 0)
 		goto err;
 
@@ -2372,7 +2410,8 @@ err:
 	return ret == -ENOSPC ? -EADDRNOTAVAIL : ret;
 }
 
-static int cma_alloc_any_port(struct idr *ps, struct rdma_id_private *id_priv)
+static int cma_alloc_any_port(enum rdma_port_space ps,
+			      struct rdma_id_private *id_priv)
 {
 	static unsigned int last_used_port;
 	int low, high, remaining;
@@ -2383,7 +2422,7 @@ static int cma_alloc_any_port(struct idr *ps, struct rdma_id_private *id_priv)
 	rover = prandom_u32() % remaining + low;
 retry:
 	if (last_used_port != rover &&
-	    !idr_find(ps, (unsigned short) rover)) {
+	    !cma_ps_find(ps, (unsigned short)rover)) {
 		int ret = cma_alloc_port(ps, id_priv, rover);
 		/*
 		 * Remember previously used port number in order to avoid
@@ -2438,7 +2477,8 @@ static int cma_check_port(struct rdma_bind_list *bind_list,
 	return 0;
 }
 
-static int cma_use_port(struct idr *ps, struct rdma_id_private *id_priv)
+static int cma_use_port(enum rdma_port_space ps,
+			struct rdma_id_private *id_priv)
 {
 	struct rdma_bind_list *bind_list;
 	unsigned short snum;
@@ -2448,7 +2488,7 @@ static int cma_use_port(struct idr *ps, struct rdma_id_private *id_priv)
 	if (snum < PROT_SOCK && !capable(CAP_NET_BIND_SERVICE))
 		return -EACCES;
 
-	bind_list = idr_find(ps, snum);
+	bind_list = cma_ps_find(ps, snum);
 	if (!bind_list) {
 		ret = cma_alloc_port(ps, id_priv, snum);
 	} else {
@@ -2471,25 +2511,24 @@ static int cma_bind_listen(struct rdma_id_private *id_priv)
 	return ret;
 }
 
-static struct idr *cma_select_inet_ps(struct rdma_id_private *id_priv)
+static enum rdma_port_space cma_select_inet_ps(
+		struct rdma_id_private *id_priv)
 {
 	switch (id_priv->id.ps) {
 	case RDMA_PS_TCP:
-		return &tcp_ps;
 	case RDMA_PS_UDP:
-		return &udp_ps;
 	case RDMA_PS_IPOIB:
-		return &ipoib_ps;
 	case RDMA_PS_IB:
-		return &ib_ps;
+		return id_priv->id.ps;
 	default:
-		return NULL;
+
+		return 0;
 	}
 }
 
-static struct idr *cma_select_ib_ps(struct rdma_id_private *id_priv)
+static enum rdma_port_space cma_select_ib_ps(struct rdma_id_private *id_priv)
 {
-	struct idr *ps = NULL;
+	enum rdma_port_space ps = 0;
 	struct sockaddr_ib *sib;
 	u64 sid_ps, mask, sid;
 
@@ -2499,15 +2538,15 @@ static struct idr *cma_select_ib_ps(struct rdma_id_private *id_priv)
 
 	if ((id_priv->id.ps == RDMA_PS_IB) && (sid == (RDMA_IB_IP_PS_IB & mask))) {
 		sid_ps = RDMA_IB_IP_PS_IB;
-		ps = &ib_ps;
+		ps = RDMA_PS_IB;
 	} else if (((id_priv->id.ps == RDMA_PS_IB) || (id_priv->id.ps == RDMA_PS_TCP)) &&
 		   (sid == (RDMA_IB_IP_PS_TCP & mask))) {
 		sid_ps = RDMA_IB_IP_PS_TCP;
-		ps = &tcp_ps;
+		ps = RDMA_PS_TCP;
 	} else if (((id_priv->id.ps == RDMA_PS_IB) || (id_priv->id.ps == RDMA_PS_UDP)) &&
 		   (sid == (RDMA_IB_IP_PS_UDP & mask))) {
 		sid_ps = RDMA_IB_IP_PS_UDP;
-		ps = &udp_ps;
+		ps = RDMA_PS_UDP;
 	}
 
 	if (ps) {
@@ -2520,7 +2559,7 @@ static struct idr *cma_select_ib_ps(struct rdma_id_private *id_priv)
 
 static int cma_get_port(struct rdma_id_private *id_priv)
 {
-	struct idr *ps;
+	enum rdma_port_space ps;
 	int ret;
 
 	if (cma_family(id_priv) != AF_IB)
-- 
1.7.11.2

--
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 07/11] IB/cma: Helper functions to access port space IDRs

[Hefty, Sean]

> Add helper functions to access the IDRs by port-space and port number.
> 
> Pass around the port-space enum in cma.c instead of using pointers to
> port-space IDRs.

What is the motivation for this change?
--
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 07/11] IB/cma: Helper functions to access port space IDRs

[Haggai Eran]

On 16/06/2015 01:36, Hefty, Sean wrote:
>> Add helper functions to access the IDRs by port-space and port number.
>>
>> Pass around the port-space enum in cma.c instead of using pointers to
>> port-space IDRs.
> 
> What is the motivation for this change?

In the next patch ("IB/cma: Add net_dev and private data checks to RDMA
CM"), in cma_id_from_event(), I pass the port-space that is extracted
from the service ID to cma_ps_find(). Without this patch, I would need
to find the correct port-space IDR, effectively duplicating the code in
cma_select_inet_ps().

Regards,
Haggai

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in

[PATCH 09/11] IB/cma: validate routing of incoming requests

[Haggai Eran]

Pass incoming request parameters through the relevant IPv4/IPv6 routing
tables and make sure the network stack is configured to handle such
requests.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 100 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index a43bbd57400c..74e562ec5b93 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -46,6 +46,8 @@
 
 #include <net/tcp.h>
 #include <net/ipv6.h>
+#include <net/ip_fib.h>
+#include <net/ip6_route.h>
 
 #include <rdma/rdma_cm.h>
 #include <rdma/rdma_cm_ib.h>
@@ -1062,15 +1064,102 @@ static int cma_save_req_info(const struct ib_cm_event *ib_event,
 	return 0;
 }
 
+static bool validate_ipv4_net_dev(struct net_device *net_dev,
+				  const struct sockaddr_in *dst_addr,
+				  const struct sockaddr_in *src_addr)
+{
+	struct in_device *in_dev = in_dev_get(net_dev);
+	__be32 daddr = dst_addr->sin_addr.s_addr,
+	       saddr = src_addr->sin_addr.s_addr;
+	struct fib_result res;
+	struct flowi4 fl4;
+	int err;
+	bool ret = false;
+
+	if (!in_dev)
+		return false;
+
+	if (ipv4_is_multicast(saddr) || ipv4_is_lbcast(saddr) ||
+	    ipv4_is_lbcast(daddr) || ipv4_is_zeronet(saddr) ||
+	    ipv4_is_zeronet(daddr) || ipv4_is_loopback(daddr) ||
+	    ipv4_is_loopback(saddr))
+		goto out;
+
+	memset(&fl4, 0, sizeof(fl4));
+	fl4.flowi4_iif = net_dev->ifindex;
+	fl4.daddr = daddr;
+	fl4.saddr = saddr;
+	err = fib_lookup(dev_net(net_dev), &fl4, &res);
+	if (err)
+		goto out;
+
+	if (res.fi->fib_dev != net_dev)
+		goto out;
+
+	ret = true;
+out:
+	in_dev_put(in_dev);
+	return ret;
+}
+
+static bool validate_ipv6_net_dev(struct net_device *net_dev,
+				  const struct sockaddr_in6 *dst_addr,
+				  const struct sockaddr_in6 *src_addr)
+{
+#if IS_ENABLED(CONFIG_IPV6)
+	const int strict = ipv6_addr_type(&dst_addr->sin6_addr) &
+			   IPV6_ADDR_LINKLOCAL;
+	struct rt6_info *rt = rt6_lookup(dev_net(net_dev), &dst_addr->sin6_addr,
+					 &src_addr->sin6_addr, net_dev->ifindex,
+					 strict);
+	bool ret;
+
+	if (!rt)
+		return false;
+
+	ret = rt->rt6i_idev->dev == net_dev;
+	ip6_rt_put(rt);
+
+	return ret;
+#else
+	return false;
+#endif
+}
+
+static bool validate_net_dev(struct net_device *net_dev,
+			     const struct sockaddr *daddr,
+			     const struct sockaddr *saddr)
+{
+	const struct sockaddr_in *daddr4 = (const struct sockaddr_in *)daddr;
+	const struct sockaddr_in *saddr4 = (const struct sockaddr_in *)saddr;
+	const struct sockaddr_in6 *daddr6 = (const struct sockaddr_in6 *)daddr;
+	const struct sockaddr_in6 *saddr6 = (const struct sockaddr_in6 *)saddr;
+
+	switch (daddr->sa_family) {
+	case AF_INET:
+		return saddr->sa_family == AF_INET &&
+		       validate_ipv4_net_dev(net_dev, daddr4, saddr4);
+
+	case AF_INET6:
+		return saddr->sa_family == AF_INET6 &&
+		       validate_ipv6_net_dev(net_dev, daddr6, saddr6);
+
+	default:
+		return false;
+	}
+}
+
 static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
 					  const struct cma_req_info *req)
 {
-	struct sockaddr_storage listen_addr_storage;
-	struct sockaddr *listen_addr = (struct sockaddr *)&listen_addr_storage;
+	struct sockaddr_storage listen_addr_storage, src_addr_storage;
+	struct sockaddr *listen_addr = (struct sockaddr *)&listen_addr_storage,
+			*src_addr = (struct sockaddr *)&src_addr_storage;
 	struct net_device *net_dev;
 	int err;
 
-	err = cma_save_ip_info(listen_addr, NULL, ib_event, req->service_id);
+	err = cma_save_ip_info(listen_addr, src_addr, ib_event,
+			       req->service_id);
 	if (err)
 		return ERR_PTR(err);
 
@@ -1079,6 +1168,11 @@ static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
 	if (!net_dev)
 		return ERR_PTR(-ENODEV);
 
+	if (!validate_net_dev(net_dev, listen_addr, src_addr)) {
+		dev_put(net_dev);
+		return ERR_PTR(-EHOSTUNREACH);
+	}
+
 	return net_dev;
 }
 
-- 
1.7.11.2

--
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 10/11] IB/cma: Share ib_cm_ids between rdma_cm_ids

[Haggai Eran]

Use ib_cm_id_create_and_listen to create listening IB CM IDs or share
existing ones if needed. When given a request on a specific CM ID, the code
now matches the request to the RDMA CM ID based on the request parameters,
so it no longer needs to rely on the ib_cm's private data matching
capabilities.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cma.c | 60 ++++---------------------------------------
 1 file changed, 5 insertions(+), 55 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 74e562ec5b93..c6d5fa7ab64a 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1722,42 +1722,6 @@ __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr)
 }
 EXPORT_SYMBOL(rdma_get_service_id);
 
-static void cma_set_compare_data(enum rdma_port_space ps, struct sockaddr *addr,
-				 struct ib_cm_compare_data *compare)
-{
-	struct cma_hdr *cma_data, *cma_mask;
-	__be32 ip4_addr;
-	struct in6_addr ip6_addr;
-
-	memset(compare, 0, sizeof *compare);
-	cma_data = (void *) compare->data;
-	cma_mask = (void *) compare->mask;
-
-	switch (addr->sa_family) {
-	case AF_INET:
-		ip4_addr = ((struct sockaddr_in *) addr)->sin_addr.s_addr;
-		cma_set_ip_ver(cma_data, 4);
-		cma_set_ip_ver(cma_mask, 0xF);
-		if (!cma_any_addr(addr)) {
-			cma_data->dst_addr.ip4.addr = ip4_addr;
-			cma_mask->dst_addr.ip4.addr = htonl(~0);
-		}
-		break;
-	case AF_INET6:
-		ip6_addr = ((struct sockaddr_in6 *) addr)->sin6_addr;
-		cma_set_ip_ver(cma_data, 6);
-		cma_set_ip_ver(cma_mask, 0xF);
-		if (!cma_any_addr(addr)) {
-			cma_data->dst_addr.ip6 = ip6_addr;
-			memset(&cma_mask->dst_addr.ip6, 0xFF,
-			       sizeof cma_mask->dst_addr.ip6);
-		}
-		break;
-	default:
-		break;
-	}
-}
-
 static int cma_iw_handler(struct iw_cm_id *iw_id, struct iw_cm_event *iw_event)
 {
 	struct rdma_id_private *id_priv = iw_id->context;
@@ -1911,33 +1875,19 @@ out:
 
 static int cma_ib_listen(struct rdma_id_private *id_priv)
 {
-	struct ib_cm_compare_data compare_data;
 	struct sockaddr *addr;
 	struct ib_cm_id	*id;
 	__be64 svc_id;
-	int ret;
 
-	id = ib_create_cm_id(id_priv->id.device, cma_req_handler, id_priv);
+	addr = cma_src_addr(id_priv);
+	svc_id = rdma_get_service_id(&id_priv->id, addr);
+	id = ib_cm_id_create_and_listen(id_priv->id.device, cma_req_handler,
+					svc_id, 0);
 	if (IS_ERR(id))
 		return PTR_ERR(id);
-
 	id_priv->cm_id.ib = id;
 
-	addr = cma_src_addr(id_priv);
-	svc_id = rdma_get_service_id(&id_priv->id, addr);
-	if (cma_any_addr(addr) && !id_priv->afonly)
-		ret = ib_cm_listen(id_priv->cm_id.ib, svc_id, 0, NULL);
-	else {
-		cma_set_compare_data(id_priv->id.ps, addr, &compare_data);
-		ret = ib_cm_listen(id_priv->cm_id.ib, svc_id, 0, &compare_data);
-	}
-
-	if (ret) {
-		ib_destroy_cm_id(id_priv->cm_id.ib);
-		id_priv->cm_id.ib = NULL;
-	}
-
-	return ret;
+	return 0;
 }
 
 static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog)
-- 
1.7.11.2

--
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 11/11] IB/cm: Remove compare_data checks

[Haggai Eran]

Now that there are no ib_cm clients using the compare_data feature for
matching IB CM requests' private data, remove the compare_data parameter of
ib_cm_listen and remove the code implementing the feature.

Signed-off-by: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/cm.c            | 87 ++++-----------------------------
 drivers/infiniband/core/ucm.c           |  3 +-
 drivers/infiniband/ulp/ipoib/ipoib_cm.c |  2 +-
 drivers/infiniband/ulp/srpt/ib_srpt.c   |  2 +-
 include/rdma/ib_cm.h                    | 14 +-----
 5 files changed, 14 insertions(+), 94 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index d124a891430b..2d769aad5c4f 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -221,7 +221,6 @@ struct cm_id_private {
 	/* todo: use alternate port on send failure */
 	struct cm_av av;
 	struct cm_av alt_av;
-	struct ib_cm_compare_data *compare_data;
 
 	void *private_data;
 	__be64 tid;
@@ -442,40 +441,6 @@ static struct cm_id_private * cm_acquire_id(__be32 local_id, __be32 remote_id)
 	return cm_id_priv;
 }
 
-static void cm_mask_copy(u32 *dst, const u32 *src, const u32 *mask)
-{
-	int i;
-
-	for (i = 0; i < IB_CM_COMPARE_SIZE; i++)
-		dst[i] = src[i] & mask[i];
-}
-
-static int cm_compare_data(struct ib_cm_compare_data *src_data,
-			   struct ib_cm_compare_data *dst_data)
-{
-	u32 src[IB_CM_COMPARE_SIZE];
-	u32 dst[IB_CM_COMPARE_SIZE];
-
-	if (!src_data || !dst_data)
-		return 0;
-
-	cm_mask_copy(src, src_data->data, dst_data->mask);
-	cm_mask_copy(dst, dst_data->data, src_data->mask);
-	return memcmp(src, dst, sizeof(src));
-}
-
-static int cm_compare_private_data(u32 *private_data,
-				   struct ib_cm_compare_data *dst_data)
-{
-	u32 src[IB_CM_COMPARE_SIZE];
-
-	if (!dst_data)
-		return 0;
-
-	cm_mask_copy(src, private_data, dst_data->mask);
-	return memcmp(src, dst_data->data, sizeof(src));
-}
-
 /*
  * Trivial helpers to strip endian annotation and compare; the
  * endianness doesn't actually matter since we just need a stable
@@ -508,18 +473,14 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
 	struct cm_id_private *cur_cm_id_priv;
 	__be64 service_id = cm_id_priv->id.service_id;
 	__be64 service_mask = cm_id_priv->id.service_mask;
-	int data_cmp;
 
 	while (*link) {
 		parent = *link;
 		cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
 					  service_node);
-		data_cmp = cm_compare_data(cm_id_priv->compare_data,
-					   cur_cm_id_priv->compare_data);
 		if ((cur_cm_id_priv->id.service_mask & service_id) ==
 		    (service_mask & cur_cm_id_priv->id.service_id) &&
-		    (cm_id_priv->id.device == cur_cm_id_priv->id.device) &&
-		    !data_cmp)
+		    (cm_id_priv->id.device == cur_cm_id_priv->id.device))
 			return cur_cm_id_priv;
 
 		if (cm_id_priv->id.device < cur_cm_id_priv->id.device)
@@ -530,8 +491,6 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
 			link = &(*link)->rb_left;
 		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
 			link = &(*link)->rb_right;
-		else if (data_cmp < 0)
-			link = &(*link)->rb_left;
 		else
 			link = &(*link)->rb_right;
 	}
@@ -541,20 +500,16 @@ static struct cm_id_private * cm_insert_listen(struct cm_id_private *cm_id_priv)
 }
 
 static struct cm_id_private * cm_find_listen(struct ib_device *device,
-					     __be64 service_id,
-					     u32 *private_data)
+					     __be64 service_id)
 {
 	struct rb_node *node = cm.listen_service_table.rb_node;
 	struct cm_id_private *cm_id_priv;
-	int data_cmp;
 
 	while (node) {
 		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
-		data_cmp = cm_compare_private_data(private_data,
-						   cm_id_priv->compare_data);
 		if ((cm_id_priv->id.service_mask & service_id) ==
 		     cm_id_priv->id.service_id &&
-		    (cm_id_priv->id.device == device) && !data_cmp)
+		    (cm_id_priv->id.device == device))
 			return cm_id_priv;
 
 		if (device < cm_id_priv->id.device)
@@ -565,8 +520,6 @@ static struct cm_id_private * cm_find_listen(struct ib_device *device,
 			node = node->rb_left;
 		else if (be64_gt(service_id, cm_id_priv->id.service_id))
 			node = node->rb_right;
-		else if (data_cmp < 0)
-			node = node->rb_left;
 		else
 			node = node->rb_right;
 	}
@@ -932,7 +885,6 @@ retest:
 	wait_for_completion(&cm_id_priv->comp);
 	while ((work = cm_dequeue_work(cm_id_priv)) != NULL)
 		cm_free_work(work);
-	kfree(cm_id_priv->compare_data);
 	kfree(cm_id_priv->private_data);
 	kfree(cm_id_priv);
 }
@@ -955,16 +907,12 @@ EXPORT_SYMBOL(ib_destroy_cm_id);
  *   range of service IDs.  If set to 0, the service ID is matched
  *   exactly.  This parameter is ignored if %service_id is set to
  *   IB_CM_ASSIGN_SERVICE_ID.
- * @compare_data: This parameter is optional.  It specifies data that must
- *   appear in the private data of a connection request for the specified
- *   listen request.
  * @lock: If set, lock the cm.lock spin-lock when adding the id to the
  *   listener tree. When false, the caller must already hold the spin-lock,
  *   and compare_data must be NULL.
  */
 static int __ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
 			  __be64 service_mask,
-			  struct ib_cm_compare_data *compare_data,
 			  bool lock)
 {
 	struct cm_id_private *cm_id_priv, *cur_cm_id_priv;
@@ -981,17 +929,6 @@ static int __ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
 	if (cm_id->state != IB_CM_IDLE)
 		return -EINVAL;
 
-	if (compare_data) {
-		cm_id_priv->compare_data = kzalloc(sizeof *compare_data,
-						   GFP_KERNEL);
-		if (!cm_id_priv->compare_data)
-			return -ENOMEM;
-		cm_mask_copy(cm_id_priv->compare_data->data,
-			     compare_data->data, compare_data->mask);
-		memcpy(cm_id_priv->compare_data->mask, compare_data->mask,
-		       sizeof(compare_data->mask));
-	}
-
 	cm_id->state = IB_CM_LISTEN;
 
 	if (lock)
@@ -1009,18 +946,14 @@ static int __ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
 
 	if (cur_cm_id_priv) {
 		cm_id->state = IB_CM_IDLE;
-		kfree(cm_id_priv->compare_data);
-		cm_id_priv->compare_data = NULL;
 		ret = -EBUSY;
 	}
 	return ret;
 }
 
-int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
-		 struct ib_cm_compare_data *compare_data)
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask)
 {
-	return __ib_cm_listen(cm_id, service_id, service_mask, compare_data,
-			      true);
+	return __ib_cm_listen(cm_id, service_id, service_mask, true);
 }
 EXPORT_SYMBOL(ib_cm_listen);
 
@@ -1066,7 +999,7 @@ struct ib_cm_id *ib_cm_id_create_and_listen(
 		goto new_id;
 
 	/* Find an existing ID */
-	cm_id_priv = cm_find_listen(device, service_id, NULL);
+	cm_id_priv = cm_find_listen(device, service_id);
 	if (cm_id_priv) {
 		atomic_inc(&cm_id_priv->refcount);
 		++cm_id_priv->listen_sharecount;
@@ -1083,7 +1016,7 @@ struct ib_cm_id *ib_cm_id_create_and_listen(
 
 new_id:
 	/* Use newly created ID */
-	err = __ib_cm_listen(cm_id, service_id, service_mask, NULL, false);
+	err = __ib_cm_listen(cm_id, service_id, service_mask, false);
 
 	spin_unlock_irqrestore(&cm.lock, flags);
 
@@ -1589,8 +1522,7 @@ static struct cm_id_private * cm_match_req(struct cm_work *work,
 
 	/* Find matching listen request. */
 	listen_cm_id_priv = cm_find_listen(cm_id_priv->id.device,
-					   req_msg->service_id,
-					   req_msg->private_data);
+					   req_msg->service_id);
 	if (!listen_cm_id_priv) {
 		cm_cleanup_timewait(cm_id_priv->timewait_info);
 		spin_unlock_irq(&cm.lock);
@@ -3144,8 +3076,7 @@ static int cm_sidr_req_handler(struct cm_work *work)
 	}
 	cm_id_priv->id.state = IB_CM_SIDR_REQ_RCVD;
 	cur_cm_id_priv = cm_find_listen(cm_id->device,
-					sidr_req_msg->service_id,
-					sidr_req_msg->private_data);
+					sidr_req_msg->service_id);
 	if (!cur_cm_id_priv) {
 		spin_unlock_irq(&cm.lock);
 		cm_reject_sidr_req(cm_id_priv, IB_SIDR_UNSUPPORTED);
diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index 62c24b1452b8..1003b5ec5591 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -658,8 +658,7 @@ static ssize_t ib_ucm_listen(struct ib_ucm_file *file,
 	if (result)
 		goto out;
 
-	result = ib_cm_listen(ctx->cm_id, cmd.service_id, cmd.service_mask,
-			      NULL);
+	result = ib_cm_listen(ctx->cm_id, cmd.service_id, cmd.service_mask);
 out:
 	ib_ucm_ctx_put(ctx);
 	return result;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index cf32a778e7d0..626959927f4d 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -854,7 +854,7 @@ int ipoib_cm_dev_open(struct net_device *dev)
 	}
 
 	ret = ib_cm_listen(priv->cm.id, cpu_to_be64(IPOIB_CM_IETF_ID | priv->qp->qp_num),
-			   0, NULL);
+			   0);
 	if (ret) {
 		printk(KERN_WARNING "%s: failed to listen on ID 0x%llx\n", priv->ca->name,
 		       IPOIB_CM_IETF_ID | priv->qp->qp_num);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0b2857b1b112..481ce1fffc62 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3259,7 +3259,7 @@ static void srpt_add_one(struct ib_device *device)
 	 * in the system as service_id; therefore, the target_id will change
 	 * if this HCA is gone bad and replaced by different HCA
 	 */
-	if (ib_cm_listen(sdev->cm_id, cpu_to_be64(srpt_service_guid), 0, NULL))
+	if (ib_cm_listen(sdev->cm_id, cpu_to_be64(srpt_service_guid), 0))
 		goto err_cm;
 
 	INIT_IB_EVENT_HANDLER(&sdev->event_handler, sdev->device,
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 0dc2ff983198..4e315c982ec3 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -105,8 +105,6 @@ enum ib_cm_data_size {
 	IB_CM_SIDR_REQ_PRIVATE_DATA_SIZE = 216,
 	IB_CM_SIDR_REP_PRIVATE_DATA_SIZE = 136,
 	IB_CM_SIDR_REP_INFO_LENGTH	 = 72,
-	/* compare done u32 at a time */
-	IB_CM_COMPARE_SIZE		 = (64 / sizeof(u32))
 };
 
 struct ib_cm_id;
@@ -340,11 +338,6 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id);
 #define IB_SDP_SERVICE_ID	cpu_to_be64(0x0000000000010000ULL)
 #define IB_SDP_SERVICE_ID_MASK	cpu_to_be64(0xFFFFFFFFFFFF0000ULL)
 
-struct ib_cm_compare_data {
-	u32  data[IB_CM_COMPARE_SIZE];
-	u32  mask[IB_CM_COMPARE_SIZE];
-};
-
 /**
  * ib_cm_listen - Initiates listening on the specified service ID for
  *   connection and service ID resolution requests.
@@ -357,12 +350,9 @@ struct ib_cm_compare_data {
  *   range of service IDs.  If set to 0, the service ID is matched
  *   exactly.  This parameter is ignored if %service_id is set to
  *   IB_CM_ASSIGN_SERVICE_ID.
- * @compare_data: This parameter is optional.  It specifies data that must
- *   appear in the private data of a connection request for the specified
- *   listen request.
  */
-int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask,
-		 struct ib_cm_compare_data *compare_data);
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
+		 __be64 service_mask);
 
 struct ib_cm_id *ib_cm_id_create_and_listen(
 		struct ib_device *device, ib_cm_handler cm_handler,
-- 
1.7.11.2

--
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 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Haggai Eran]

Instead of relying on a the ib_cm module to check an incoming CM request's
private data header, add these checks to the RDMA CM module. This allows a
following patch to to clean up the ib_cm interface and remove the code that
looks into the private headers. It will also allow supporting namespaces in
RDMA CM by making these checks namespace aware later on.

Signed-off-by: Haggai Eran <haggaie@mellanox.com>
---
 drivers/infiniband/core/cma.c | 170 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 168 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index bb90d462f819..a43bbd57400c 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -300,7 +300,7 @@ static enum rdma_cm_state cma_exch(struct rdma_id_private *id_priv,
 	return old;
 }
 
-static inline u8 cma_get_ip_ver(struct cma_hdr *hdr)
+static inline u8 cma_get_ip_ver(const struct cma_hdr *hdr)
 {
 	return hdr->ip_version >> 4;
 }
@@ -1024,6 +1024,169 @@ static int cma_save_net_info(struct sockaddr *src_addr,
 	return cma_save_ip_info(src_addr, dst_addr, ib_event, service_id);
 }
 
+struct cma_req_info {
+	struct ib_device *device;
+	int port;
+	const union ib_gid *local_gid;
+	__be64 service_id;
+	u16 pkey;
+};
+
+static int cma_save_req_info(const struct ib_cm_event *ib_event,
+			     struct cma_req_info *req)
+{
+	const struct ib_cm_req_event_param *req_param =
+		&ib_event->param.req_rcvd;
+	const struct ib_cm_sidr_req_event_param *sidr_param =
+		&ib_event->param.sidr_req_rcvd;
+
+	switch (ib_event->event) {
+	case IB_CM_REQ_RECEIVED:
+		req->device	= req_param->listen_id->device;
+		req->port	= req_param->port;
+		req->local_gid	= &req_param->primary_path->sgid;
+		req->service_id	= req_param->primary_path->service_id;
+		req->pkey	= be16_to_cpu(req_param->primary_path->pkey);
+		break;
+	case IB_CM_SIDR_REQ_RECEIVED:
+		req->device	= sidr_param->listen_id->device;
+		req->port	= sidr_param->port;
+		req->local_gid	= sidr_param->grh ? &sidr_param->dgid : NULL;
+		req->service_id	= sidr_param->service_id;
+		req->pkey	= sidr_param->pkey;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
+					  const struct cma_req_info *req)
+{
+	struct sockaddr_storage listen_addr_storage;
+	struct sockaddr *listen_addr = (struct sockaddr *)&listen_addr_storage;
+	struct net_device *net_dev;
+	int err;
+
+	err = cma_save_ip_info(listen_addr, NULL, ib_event, req->service_id);
+	if (err)
+		return ERR_PTR(err);
+
+	net_dev = ib_get_net_dev_by_params(req->device, req->port, req->pkey,
+					   req->local_gid, listen_addr);
+	if (!net_dev)
+		return ERR_PTR(-ENODEV);
+
+	return net_dev;
+}
+
+static enum rdma_port_space rdma_ps_from_service_id(__be64 service_id)
+{
+	return (be64_to_cpu(service_id) >> 16) & 0xffff;
+}
+
+static bool cma_match_private_data(struct rdma_id_private *id_priv,
+				   const struct cma_hdr *hdr)
+{
+	struct sockaddr *addr = cma_src_addr(id_priv);
+	__be32 ip4_addr;
+	struct in6_addr ip6_addr;
+
+	if (cma_any_addr(addr) && !id_priv->afonly)
+		return true;
+
+	switch (addr->sa_family) {
+	case AF_INET:
+		ip4_addr = ((struct sockaddr_in *)addr)->sin_addr.s_addr;
+		if (cma_get_ip_ver(hdr) != 4)
+			return false;
+		if (!cma_any_addr(addr) &&
+		    hdr->dst_addr.ip4.addr != ip4_addr)
+			return false;
+		break;
+	case AF_INET6:
+		ip6_addr = ((struct sockaddr_in6 *)addr)->sin6_addr;
+		if (cma_get_ip_ver(hdr) != 6)
+			return false;
+		if (!cma_any_addr(addr) &&
+		    memcmp(&hdr->dst_addr.ip6, &ip6_addr, sizeof(ip6_addr)))
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
+static bool cma_match_net_dev(const struct rdma_id_private *id_priv,
+			      const struct net_device *net_dev)
+{
+	const struct rdma_dev_addr *addr = &id_priv->id.route.addr.dev_addr;
+
+	return !addr->bound_dev_if ||
+	       (net_eq(dev_net(net_dev), &init_net) &&
+		addr->bound_dev_if == net_dev->ifindex);
+}
+
+static struct rdma_id_private *cma_find_listener(
+		const struct rdma_bind_list *bind_list,
+		const struct ib_cm_id *cm_id,
+		const struct ib_cm_event *ib_event,
+		const struct cma_req_info *req,
+		const struct net_device *net_dev)
+{
+	struct rdma_id_private *id_priv, *id_priv_dev;
+
+	if (!bind_list)
+		return ERR_PTR(-EINVAL);
+
+	hlist_for_each_entry(id_priv, &bind_list->owners, node) {
+		if (cma_match_private_data(id_priv, ib_event->private_data)) {
+			if (id_priv->id.device == cm_id->device &&
+			    cma_match_net_dev(id_priv, net_dev))
+				return id_priv;
+			list_for_each_entry(id_priv_dev,
+					    &id_priv->listen_list,
+					    listen_list) {
+				if (id_priv_dev->id.device == cm_id->device &&
+				    cma_match_net_dev(id_priv_dev, net_dev))
+					return id_priv_dev;
+			}
+		}
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+static struct rdma_id_private *cma_id_from_event(struct ib_cm_id *cm_id,
+						 struct ib_cm_event *ib_event)
+{
+	struct cma_req_info req;
+	struct rdma_bind_list *bind_list;
+	struct rdma_id_private *id_priv;
+	struct net_device *net_dev;
+	int err;
+
+	err = cma_save_req_info(ib_event, &req);
+	if (err)
+		return ERR_PTR(err);
+
+	net_dev = cma_get_net_dev(ib_event, &req);
+	if (IS_ERR(net_dev))
+		return ERR_PTR(PTR_ERR(net_dev));
+
+	bind_list = cma_ps_find(rdma_ps_from_service_id(req.service_id),
+				cma_port_from_service_id(req.service_id));
+	id_priv = cma_find_listener(bind_list, cm_id, ib_event, &req, net_dev);
+
+	dev_put(net_dev);
+
+	return id_priv;
+}
+
 static inline int cma_user_data_offset(struct rdma_id_private *id_priv)
 {
 	return cma_family(id_priv) == AF_IB ? 0 : sizeof(struct cma_hdr);
@@ -1383,7 +1546,10 @@ static int cma_req_handler(struct ib_cm_id *cm_id, struct ib_cm_event *ib_event)
 	struct rdma_cm_event event;
 	int offset, ret;
 
-	listen_id = cm_id->context;
+	listen_id = cma_id_from_event(cm_id, ib_event);
+	if (IS_ERR(listen_id))
+		return PTR_ERR(listen_id);
+
 	if (!cma_check_req_qp_type(&listen_id->id, ib_event))
 		return -EINVAL;
 
-- 
1.7.11.2

Re: [PATCH 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Jason Gunthorpe]

On Mon, Jun 15, 2015 at 11:47:13AM +0300, Haggai Eran wrote:
> Instead of relying on a the ib_cm module to check an incoming CM request's
> private data header, add these checks to the RDMA CM module. This allows a
> following patch to to clean up the ib_cm interface and remove the code that
> looks into the private headers. It will also allow supporting namespaces in
> RDMA CM by making these checks namespace aware later on.

I was expecting one of these patches to flow the net_device from here:

> +static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
> +					  const struct cma_req_info *req)
> +{

Down through cma_req_handler and cma_new_conn_id so that we get rid of
the cma_translate_addr on the ingress side.

Having the ingress side use one ingress net_device for all processing
seems very important to me...

Jason

Re: [PATCH 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Haggai Eran]

On 15/06/2015 20:08, Jason Gunthorpe wrote:
> On Mon, Jun 15, 2015 at 11:47:13AM +0300, Haggai Eran wrote:
>> Instead of relying on a the ib_cm module to check an incoming CM request's
>> private data header, add these checks to the RDMA CM module. This allows a
>> following patch to to clean up the ib_cm interface and remove the code that
>> looks into the private headers. It will also allow supporting namespaces in
>> RDMA CM by making these checks namespace aware later on.
> 
> I was expecting one of these patches to flow the net_device from here:
> 
>> +static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
>> +					  const struct cma_req_info *req)
>> +{
> 
> Down through cma_req_handler and cma_new_conn_id so that we get rid of
> the cma_translate_addr on the ingress side.
> 
> Having the ingress side use one ingress net_device for all processing
> seems very important to me...

Is it really very important? I thought the bound_dev_if of a passive
connection id is only used by the netlink statistics mechanism.

If you insist, I can set bound_dev_if based on the results of
cma_get_net_dev. That would cause the rdma_translate_ip call in
cma_translate_addr to exit early after calling rdma_copy_addr with the
chosen net_dev.

Haggai

Re: [PATCH 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Haggai Eran]

On 15/06/2015 20:08, Jason Gunthorpe wrote:
> On Mon, Jun 15, 2015 at 11:47:13AM +0300, Haggai Eran wrote:
>> Instead of relying on a the ib_cm module to check an incoming CM request's
>> private data header, add these checks to the RDMA CM module. This allows a
>> following patch to to clean up the ib_cm interface and remove the code that
>> looks into the private headers. It will also allow supporting namespaces in
>> RDMA CM by making these checks namespace aware later on.
> 
> I was expecting one of these patches to flow the net_device from here:
> 
>> +static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
>> +					  const struct cma_req_info *req)
>> +{
> 
> Down through cma_req_handler and cma_new_conn_id so that we get rid of
> the cma_translate_addr on the ingress side.
> 
> Having the ingress side use one ingress net_device for all processing
> seems very important to me...

Is it really very important? I thought the bound_dev_if of a passive
connection id is only used by the netlink statistics mechanism.

If you insist, I can set bound_dev_if based on the results of
cma_get_net_dev. That would cause the rdma_translate_ip call in
cma_translate_addr to exit early after calling rdma_copy_addr with the
chosen net_dev.

Haggai

Re: [PATCH 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Jason Gunthorpe]

On Tue, Jun 16, 2015 at 08:26:26AM +0300, Haggai Eran wrote:
> On 15/06/2015 20:08, Jason Gunthorpe wrote:
> > On Mon, Jun 15, 2015 at 11:47:13AM +0300, Haggai Eran wrote:
> >> Instead of relying on a the ib_cm module to check an incoming CM request's
> >> private data header, add these checks to the RDMA CM module. This allows a
> >> following patch to to clean up the ib_cm interface and remove the code that
> >> looks into the private headers. It will also allow supporting namespaces in
> >> RDMA CM by making these checks namespace aware later on.
> > 
> > I was expecting one of these patches to flow the net_device from here:
> > 
> >> +static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
> >> +					  const struct cma_req_info *req)
> >> +{
> > 
> > Down through cma_req_handler and cma_new_conn_id so that we get rid of
> > the cma_translate_addr on the ingress side.
> > 
> > Having the ingress side use one ingress net_device for all processing
> > seems very important to me...
> 
> Is it really very important? I thought the bound_dev_if of a passive
> connection id is only used by the netlink statistics mechanism.

I mean 'very important' in the sense it makes the RDMA-CM *make
logical sense*, not so much in the 'can user space tell'.

So yes, cleaning this seems very important to establish that logical
narrative of how the packet flows through this code.

Plus, there is an init_net in the cma_translate_addr path that needs to
be addressed - so purging cma_translate_addr is a great way to handle
that. That would leave only the call in rdma_bind_addr, and for bind,
the process net namespace is the correct thing to use.

Jason
--
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 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Haggai Eran]

On 17/06/2015 20:18, Jason Gunthorpe wrote:
> On Tue, Jun 16, 2015 at 08:26:26AM +0300, Haggai Eran wrote:
>> On 15/06/2015 20:08, Jason Gunthorpe wrote:
>>> On Mon, Jun 15, 2015 at 11:47:13AM +0300, Haggai Eran wrote:
>>>> Instead of relying on a the ib_cm module to check an incoming CM request's
>>>> private data header, add these checks to the RDMA CM module. This allows a
>>>> following patch to to clean up the ib_cm interface and remove the code that
>>>> looks into the private headers. It will also allow supporting namespaces in
>>>> RDMA CM by making these checks namespace aware later on.
>>>
>>> I was expecting one of these patches to flow the net_device from here:
>>>
>>>> +static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
>>>> +					  const struct cma_req_info *req)
>>>> +{
>>>
>>> Down through cma_req_handler and cma_new_conn_id so that we get rid of
>>> the cma_translate_addr on the ingress side.
>>>
>>> Having the ingress side use one ingress net_device for all processing
>>> seems very important to me...
>>
>> Is it really very important? I thought the bound_dev_if of a passive
>> connection id is only used by the netlink statistics mechanism.
> 
> I mean 'very important' in the sense it makes the RDMA-CM *make
> logical sense*, not so much in the 'can user space tell'.
> 
> So yes, cleaning this seems very important to establish that logical
> narrative of how the packet flows through this code.
> 
> Plus, there is an init_net in the cma_translate_addr path that needs to
> be addressed - so purging cma_translate_addr is a great way to handle
> that. That would leave only the call in rdma_bind_addr, and for bind,
> the process net namespace is the correct thing to use.
Okay, I'll add a patch that cleans these cma_translate_addr calls.


--
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 08/11] IB/cma: Add net_dev and private data checks to RDMA CM

[Haggai Eran]

On 17/06/2015 20:18, Jason Gunthorpe wrote:
> On Tue, Jun 16, 2015 at 08:26:26AM +0300, Haggai Eran wrote:
>> On 15/06/2015 20:08, Jason Gunthorpe wrote:
>>> On Mon, Jun 15, 2015 at 11:47:13AM +0300, Haggai Eran wrote:
>>>> Instead of relying on a the ib_cm module to check an incoming CM request's
>>>> private data header, add these checks to the RDMA CM module. This allows a
>>>> following patch to to clean up the ib_cm interface and remove the code that
>>>> looks into the private headers. It will also allow supporting namespaces in
>>>> RDMA CM by making these checks namespace aware later on.
>>>
>>> I was expecting one of these patches to flow the net_device from here:
>>>
>>>> +static struct net_device *cma_get_net_dev(struct ib_cm_event *ib_event,
>>>> +					  const struct cma_req_info *req)
>>>> +{
>>>
>>> Down through cma_req_handler and cma_new_conn_id so that we get rid of
>>> the cma_translate_addr on the ingress side.
>>>
>>> Having the ingress side use one ingress net_device for all processing
>>> seems very important to me...
>>
>> Is it really very important? I thought the bound_dev_if of a passive
>> connection id is only used by the netlink statistics mechanism.
> 
> I mean 'very important' in the sense it makes the RDMA-CM *make
> logical sense*, not so much in the 'can user space tell'.
> 
> So yes, cleaning this seems very important to establish that logical
> narrative of how the packet flows through this code.
> 
> Plus, there is an init_net in the cma_translate_addr path that needs to
> be addressed - so purging cma_translate_addr is a great way to handle
> that. That would leave only the call in rdma_bind_addr, and for bind,
> the process net namespace is the correct thing to use.
Okay, I'll add a patch that cleans these cma_translate_addr calls.


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