[PATCH 00/15] RDMA patches for kernel v5.5

[PATCH 00/15] RDMA patches for kernel v5.5

[Bart Van Assche]

Hi Jason,

This patch series includes my pending RDMA kernel patches. Please consider
these for inclusion in Linux kernel v5.5.

Thanks,

Bart.

Bart Van Assche (15):
  RDMA/ucma: Reduce the number of rdma_destroy_id() calls
  RDMA/iwcm: Fix a lock inversion issue
  RDMA/siw: Simplify several debug messages
  RDMA/siw: Fix port number endianness in a debug message
  RDMA/siw: Make node GUIDs valid EUI-64 identifiers
  RDMA/srp: Remove two casts
  RDMA/srp: Honor the max_send_sge device attribute
  RDMA/srp: Make route resolving error messages more informative
  RDMA/srpt: Fix handling of SR-IOV and iWARP ports
  RDMA/srpt: Fix handling of iWARP logins
  RDMA/srpt: Improve a debug message
  RDMA/srpt: Rework the approach for closing an RDMA channel
  RDMA/srpt: Rework the code that waits until an RDMA port is no longer
    in use
  RDMA/srpt: Make the code for handling port identities more systematic
  RDMA/srpt: Postpone HCA removal until after configfs directory removal

 drivers/infiniband/core/cma.c         |   3 +-
 drivers/infiniband/core/ucma.c        |  17 ++-
 drivers/infiniband/sw/siw/siw_cm.c    |  45 ++-----
 drivers/infiniband/sw/siw/siw_main.c  |  11 +-
 drivers/infiniband/ulp/srp/ib_srp.c   |  15 ++-
 drivers/infiniband/ulp/srp/ib_srp.h   |   3 +
 drivers/infiniband/ulp/srpt/ib_srpt.c | 186 +++++++++++++-------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  34 +++--
 8 files changed, 156 insertions(+), 158 deletions(-)

-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 01/15] RDMA/ucma: Reduce the number of rdma_destroy_id() calls

[Bart Van Assche]

Instead of calling rdma_destroy_id() after waiting for the context completion
finished, call rdma_destroy_id() from inside the ucma_put_ctx() function.
This patch reduces the number of rdma_destroy_id() calls but does not change
the behavior of this code.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/ucma.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 0274e9b704be..30c09864fd9e 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -160,8 +160,14 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id)
 
 static void ucma_put_ctx(struct ucma_context *ctx)
 {
-	if (atomic_dec_and_test(&ctx->ref))
-		complete(&ctx->comp);
+	if (!atomic_dec_and_test(&ctx->ref))
+		return;
+	/*
+	 * rdma_destroy_id() ensures that no event handlers are inflight
+	 * for that id before releasing it.
+	 */
+	rdma_destroy_id(ctx->cm_id);
+	complete(&ctx->comp);
 }
 
 /*
@@ -199,8 +205,6 @@ static void ucma_close_id(struct work_struct *work)
 	 */
 	ucma_put_ctx(ctx);
 	wait_for_completion(&ctx->comp);
-	/* No new events will be generated after destroying the id. */
-	rdma_destroy_id(ctx->cm_id);
 }
 
 static struct ucma_context *ucma_alloc_ctx(struct ucma_file *file)
@@ -628,7 +632,6 @@ static ssize_t ucma_destroy_id(struct ucma_file *file, const char __user *inbuf,
 		xa_unlock(&ctx_table);
 		ucma_put_ctx(ctx);
 		wait_for_completion(&ctx->comp);
-		rdma_destroy_id(ctx->cm_id);
 	} else {
 		xa_unlock(&ctx_table);
 	}
@@ -1756,10 +1759,6 @@ static int ucma_close(struct inode *inode, struct file *filp)
 			xa_unlock(&ctx_table);
 			ucma_put_ctx(ctx);
 			wait_for_completion(&ctx->comp);
-			/* rdma_destroy_id ensures that no event handlers are
-			 * inflight for that id before releasing it.
-			 */
-			rdma_destroy_id(ctx->cm_id);
 		} else {
 			xa_unlock(&ctx_table);
 		}
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 01/15] RDMA/ucma: Reduce the number of rdma_destroy_id() calls

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:16:53PM -0700, Bart Van Assche wrote:
> Instead of calling rdma_destroy_id() after waiting for the context completion
> finished, call rdma_destroy_id() from inside the ucma_put_ctx() function.
> This patch reduces the number of rdma_destroy_id() calls but does not change
> the behavior of this code.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>  drivers/infiniband/core/ucma.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
> index 0274e9b704be..30c09864fd9e 100644
> +++ b/drivers/infiniband/core/ucma.c
> @@ -160,8 +160,14 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id)
>  
>  static void ucma_put_ctx(struct ucma_context *ctx)
>  {
> -	if (atomic_dec_and_test(&ctx->ref))
> -		complete(&ctx->comp);
> +	if (!atomic_dec_and_test(&ctx->ref))
> +		return;
> +	/*
> +	 * rdma_destroy_id() ensures that no event handlers are inflight
> +	 * for that id before releasing it.
> +	 */
> +	rdma_destroy_id(ctx->cm_id);
> +	complete(&ctx->comp);
>  }

Since all the refcounting here is basically insane, you can't do this
without creating new kinds of bugs related to lifetime of ctx->cm_id

The call to rdma_destroy_id must be after the xa_erase as other
threads can continue to access the context despite its zero ref via
ucma_get_ctx(() as ucma_get_ctx() is not using refcounts properly.

The xa_erase provides the needed barrier.

Maybe this patch could be fixed if the ucma_get_ctx used an
atomic_inc_not_zero ?

Jason

Re: [PATCH 01/15] RDMA/ucma: Reduce the number of rdma_destroy_id() calls

[Bart Van Assche]

On 10/1/19 8:07 AM, Jason Gunthorpe wrote:
> On Mon, Sep 30, 2019 at 04:16:53PM -0700, Bart Van Assche wrote:
>> Instead of calling rdma_destroy_id() after waiting for the context completion
>> finished, call rdma_destroy_id() from inside the ucma_put_ctx() function.
>> This patch reduces the number of rdma_destroy_id() calls but does not change
>> the behavior of this code.
>>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>   drivers/infiniband/core/ucma.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
>> index 0274e9b704be..30c09864fd9e 100644
>> +++ b/drivers/infiniband/core/ucma.c
>> @@ -160,8 +160,14 @@ static struct ucma_context *ucma_get_ctx(struct ucma_file *file, int id)
>>   
>>   static void ucma_put_ctx(struct ucma_context *ctx)
>>   {
>> -	if (atomic_dec_and_test(&ctx->ref))
>> -		complete(&ctx->comp);
>> +	if (!atomic_dec_and_test(&ctx->ref))
>> +		return;
>> +	/*
>> +	 * rdma_destroy_id() ensures that no event handlers are inflight
>> +	 * for that id before releasing it.
>> +	 */
>> +	rdma_destroy_id(ctx->cm_id);
>> +	complete(&ctx->comp);
>>   }
> 
> Since all the refcounting here is basically insane, you can't do this
> without creating new kinds of bugs related to lifetime of ctx->cm_id
> 
> The call to rdma_destroy_id must be after the xa_erase as other
> threads can continue to access the context despite its zero ref via
> ucma_get_ctx(() as ucma_get_ctx() is not using refcounts properly.
> 
> The xa_erase provides the needed barrier.
> 
> Maybe this patch could be fixed if the ucma_get_ctx used an
> atomic_inc_not_zero ?

Hi Jason,

Since I'm not an ucma expert, maybe it's better that I drop this patch.

Thanks,

Bart.

[PATCH 02/15] RDMA/iwcm: Fix a lock inversion issue

[Bart Van Assche]

This patch fixes the lock inversion complaint:

============================================
WARNING: possible recursive locking detected
5.3.0-rc7-dbg+ #1 Not tainted
--------------------------------------------
kworker/u16:6/171 is trying to acquire lock:
00000000035c6e6c (&id_priv->handler_mutex){+.+.}, at: rdma_destroy_id+0x78/0x4a0 [rdma_cm]

but task is already holding lock:
00000000bc7c307d (&id_priv->handler_mutex){+.+.}, at: iw_conn_req_handler+0x151/0x680 [rdma_cm]

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&id_priv->handler_mutex);
  lock(&id_priv->handler_mutex);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

3 locks held by kworker/u16:6/171:
 #0: 00000000e2eaa773 ((wq_completion)iw_cm_wq){+.+.}, at: process_one_work+0x472/0xac0
 #1: 000000001efd357b ((work_completion)(&work->work)#3){+.+.}, at: process_one_work+0x476/0xac0
 #2: 00000000bc7c307d (&id_priv->handler_mutex){+.+.}, at: iw_conn_req_handler+0x151/0x680 [rdma_cm]

stack backtrace:
CPU: 3 PID: 171 Comm: kworker/u16:6 Not tainted 5.3.0-rc7-dbg+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Workqueue: iw_cm_wq cm_work_handler [iw_cm]
Call Trace:
 dump_stack+0x8a/0xd6
 __lock_acquire.cold+0xe1/0x24d
 lock_acquire+0x106/0x240
 __mutex_lock+0x12e/0xcb0
 mutex_lock_nested+0x1f/0x30
 rdma_destroy_id+0x78/0x4a0 [rdma_cm]
 iw_conn_req_handler+0x5c9/0x680 [rdma_cm]
 cm_work_handler+0xe62/0x1100 [iw_cm]
 process_one_work+0x56d/0xac0
 worker_thread+0x7a/0x5d0
 kthread+0x1bc/0x210
 ret_from_fork+0x24/0x30

Cc: Or Gerlitz <gerlitz.or@gmail.com>
Cc: Steve Wise <larrystevenwise@gmail.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Bernard Metzler <BMT@zurich.ibm.com>
Cc: Krishnamraju Eraparaju <krishna2@chelsio.com>
Cc: <stable@vger.kernel.org>
Fixes: de910bd92137 ("RDMA/cma: Simplify locking needed for serialization of callbacks"; v2.6.27).
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/core/cma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 0e3cf3461999..d78f67623f24 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -2396,9 +2396,10 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
 		conn_id->cm_id.iw = NULL;
 		cma_exch(conn_id, RDMA_CM_DESTROYING);
 		mutex_unlock(&conn_id->handler_mutex);
+		mutex_unlock(&listen_id->handler_mutex);
 		cma_deref_id(conn_id);
 		rdma_destroy_id(&conn_id->id);
-		goto out;
+		return ret;
 	}
 
 	mutex_unlock(&conn_id->handler_mutex);
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 02/15] RDMA/iwcm: Fix a lock inversion issue

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:16:54PM -0700, Bart Van Assche wrote:
> This patch fixes the lock inversion complaint:
> 
> ============================================
> WARNING: possible recursive locking detected
> 5.3.0-rc7-dbg+ #1 Not tainted
> kworker/u16:6/171 is trying to acquire lock:
> 00000000035c6e6c (&id_priv->handler_mutex){+.+.}, at: rdma_destroy_id+0x78/0x4a0 [rdma_cm]
> 
> but task is already holding lock:
> 00000000bc7c307d (&id_priv->handler_mutex){+.+.}, at: iw_conn_req_handler+0x151/0x680 [rdma_cm]
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>   lock(&id_priv->handler_mutex);
>   lock(&id_priv->handler_mutex);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 3 locks held by kworker/u16:6/171:
>  #0: 00000000e2eaa773 ((wq_completion)iw_cm_wq){+.+.}, at: process_one_work+0x472/0xac0
>  #1: 000000001efd357b ((work_completion)(&work->work)#3){+.+.}, at: process_one_work+0x476/0xac0
>  #2: 00000000bc7c307d (&id_priv->handler_mutex){+.+.}, at: iw_conn_req_handler+0x151/0x680 [rdma_cm]
> 
> stack backtrace:
> CPU: 3 PID: 171 Comm: kworker/u16:6 Not tainted 5.3.0-rc7-dbg+ #1
> Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> Workqueue: iw_cm_wq cm_work_handler [iw_cm]
> Call Trace:
>  dump_stack+0x8a/0xd6
>  __lock_acquire.cold+0xe1/0x24d
>  lock_acquire+0x106/0x240
>  __mutex_lock+0x12e/0xcb0
>  mutex_lock_nested+0x1f/0x30
>  rdma_destroy_id+0x78/0x4a0 [rdma_cm]
>  iw_conn_req_handler+0x5c9/0x680 [rdma_cm]
>  cm_work_handler+0xe62/0x1100 [iw_cm]
>  process_one_work+0x56d/0xac0
>  worker_thread+0x7a/0x5d0
>  kthread+0x1bc/0x210
>  ret_from_fork+0x24/0x30
> 
> Cc: Or Gerlitz <gerlitz.or@gmail.com>
> Cc: Steve Wise <larrystevenwise@gmail.com>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Bernard Metzler <BMT@zurich.ibm.com>
> Cc: Krishnamraju Eraparaju <krishna2@chelsio.com>
> Cc: <stable@vger.kernel.org>
> Fixes: de910bd92137 ("RDMA/cma: Simplify locking needed for serialization of callbacks"; v2.6.27).
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>  drivers/infiniband/core/cma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 0e3cf3461999..d78f67623f24 100644
> +++ b/drivers/infiniband/core/cma.c
> @@ -2396,9 +2396,10 @@ static int iw_conn_req_handler(struct iw_cm_id *cm_id,
>  		conn_id->cm_id.iw = NULL;
>  		cma_exch(conn_id, RDMA_CM_DESTROYING);
>  		mutex_unlock(&conn_id->handler_mutex);
> +		mutex_unlock(&listen_id->handler_mutex);
>  		cma_deref_id(conn_id);
>  		rdma_destroy_id(&conn_id->id);
> -		goto out;
> +		return ret;
>  	}

Hurm. Minimizing code under lock is always a good fix, but the lockdep
report is not a bug.

The issue is caused by the hacky use of SINGLE_DEPTH_NESTING when we
really have two lock classes, 'listening' and 'connecting' for CM ids.

connecting IDs can be nested under listening IDs but not the other way
around.

So two lock classes will also get rid of the lockdep warning, which is
why it isn't a bug..

Applied to for-rc

Thanks,
Jason

[PATCH 03/15] RDMA/siw: Simplify several debug messages

[Bart Van Assche]

Do not print the remote address if it is not used. Use %pISp instead
of %pI4 %d.

Cc: Bernard Metzler <bmt@zurich.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/sw/siw/siw_cm.c | 36 ++++++------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 8c1931a57f4a..5a75deb9870b 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1373,22 +1373,8 @@ int siw_connect(struct iw_cm_id *id, struct iw_cm_conn_param *params)
 		rv = -EINVAL;
 		goto error;
 	}
-	if (v4)
-		siw_dbg_qp(qp,
-			   "pd_len %d, laddr %pI4 %d, raddr %pI4 %d\n",
-			   pd_len,
-			   &((struct sockaddr_in *)(laddr))->sin_addr,
-			   ntohs(((struct sockaddr_in *)(laddr))->sin_port),
-			   &((struct sockaddr_in *)(raddr))->sin_addr,
-			   ntohs(((struct sockaddr_in *)(raddr))->sin_port));
-	else
-		siw_dbg_qp(qp,
-			   "pd_len %d, laddr %pI6 %d, raddr %pI6 %d\n",
-			   pd_len,
-			   &((struct sockaddr_in6 *)(laddr))->sin6_addr,
-			   ntohs(((struct sockaddr_in6 *)(laddr))->sin6_port),
-			   &((struct sockaddr_in6 *)(raddr))->sin6_addr,
-			   ntohs(((struct sockaddr_in6 *)(raddr))->sin6_port));
+	siw_dbg_qp(qp, "pd_len %d, laddr %pISp, raddr %pISp\n", pd_len, laddr,
+		   raddr);
 
 	rv = sock_create(v4 ? AF_INET : AF_INET6, SOCK_STREAM, IPPROTO_TCP, &s);
 	if (rv < 0)
@@ -1935,7 +1921,7 @@ static void siw_drop_listeners(struct iw_cm_id *id)
 /*
  * siw_create_listen - Create resources for a listener's IWCM ID @id
  *
- * Listens on the socket addresses id->local_addr and id->remote_addr.
+ * Listens on the socket address id->local_addr.
  *
  * If the listener's @id provides a specific local IP address, at most one
  * listening socket is created and associated with @id.
@@ -1959,7 +1945,7 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 	 */
 	if (id->local_addr.ss_family == AF_INET) {
 		struct in_device *in_dev = in_dev_get(dev);
-		struct sockaddr_in s_laddr, *s_raddr;
+		struct sockaddr_in s_laddr;
 		const struct in_ifaddr *ifa;
 
 		if (!in_dev) {
@@ -1967,12 +1953,8 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 			goto out;
 		}
 		memcpy(&s_laddr, &id->local_addr, sizeof(s_laddr));
-		s_raddr = (struct sockaddr_in *)&id->remote_addr;
 
-		siw_dbg(id->device,
-			"laddr %pI4:%d, raddr %pI4:%d\n",
-			&s_laddr.sin_addr, ntohs(s_laddr.sin_port),
-			&s_raddr->sin_addr, ntohs(s_raddr->sin_port));
+		siw_dbg(id->device, "laddr %pISp\n", &s_laddr);
 
 		rtnl_lock();
 		in_dev_for_each_ifa_rtnl(ifa, in_dev) {
@@ -1992,17 +1974,13 @@ int siw_create_listen(struct iw_cm_id *id, int backlog)
 	} else if (id->local_addr.ss_family == AF_INET6) {
 		struct inet6_dev *in6_dev = in6_dev_get(dev);
 		struct inet6_ifaddr *ifp;
-		struct sockaddr_in6 *s_laddr = &to_sockaddr_in6(id->local_addr),
-			*s_raddr = &to_sockaddr_in6(id->remote_addr);
+		struct sockaddr_in6 *s_laddr = &to_sockaddr_in6(id->local_addr);
 
 		if (!in6_dev) {
 			rv = -ENODEV;
 			goto out;
 		}
-		siw_dbg(id->device,
-			"laddr %pI6:%d, raddr %pI6:%d\n",
-			&s_laddr->sin6_addr, ntohs(s_laddr->sin6_port),
-			&s_raddr->sin6_addr, ntohs(s_raddr->sin6_port));
+		siw_dbg(id->device, "laddr %pISp\n", &s_laddr);
 
 		rtnl_lock();
 		list_for_each_entry(ifp, &in6_dev->addr_list, if_list) {
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 03/15] RDMA/siw: Simplify several debug messages

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:16:55PM -0700, Bart Van Assche wrote:
> Do not print the remote address if it is not used. Use %pISp instead
> of %pI4 %d.
> 
> Cc: Bernard Metzler <bmt@zurich.ibm.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 36 ++++++------------------------
>  1 file changed, 7 insertions(+), 29 deletions(-)

Applied to for-next, thanks

Jason

[PATCH 04/15] RDMA/siw: Fix port number endianness in a debug message

[Bart Van Assche]

sin_port and sin6_port are big endian member variables. Convert these port
numbers into CPU endianness before printing.

Cc: Bernard Metzler <bmt@zurich.ibm.com>
Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/sw/siw/siw_cm.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_cm.c b/drivers/infiniband/sw/siw/siw_cm.c
index 5a75deb9870b..3bccfef40e7e 100644
--- a/drivers/infiniband/sw/siw/siw_cm.c
+++ b/drivers/infiniband/sw/siw/siw_cm.c
@@ -1853,14 +1853,7 @@ static int siw_listen_address(struct iw_cm_id *id, int backlog,
 	list_add_tail(&cep->listenq, (struct list_head *)id->provider_data);
 	cep->state = SIW_EPSTATE_LISTENING;
 
-	if (addr_family == AF_INET)
-		siw_dbg(id->device, "Listen at laddr %pI4 %u\n",
-			&(((struct sockaddr_in *)laddr)->sin_addr),
-			((struct sockaddr_in *)laddr)->sin_port);
-	else
-		siw_dbg(id->device, "Listen at laddr %pI6 %u\n",
-			&(((struct sockaddr_in6 *)laddr)->sin6_addr),
-			((struct sockaddr_in6 *)laddr)->sin6_port);
+	siw_dbg(id->device, "Listen at laddr %pISp\n", laddr);
 
 	return 0;
 
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 04/15] RDMA/siw: Fix port number endianness in a debug message

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:16:56PM -0700, Bart Van Assche wrote:
> sin_port and sin6_port are big endian member variables. Convert these port
> numbers into CPU endianness before printing.
> 
> Cc: Bernard Metzler <bmt@zurich.ibm.com>
> Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)

Applied to for-next, thanks

Jason

[PATCH 05/15] RDMA/siw: Make node GUIDs valid EUI-64 identifiers

[Bart Van Assche]

From the IBTA: "GUID (Global Unique Identifier): A globally unique EUI-64
compliant identifier." Make sure that siw GUIDs are valid EUI-64 identifiers.

Cc: Bernard Metzler <bmt@zurich.ibm.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/sw/siw/siw_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
index 05a92f997f60..d1a1b7aa7d83 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
 
+#include <net/addrconf.h>
 #include <rdma/ib_verbs.h>
 #include <rdma/ib_user_verbs.h>
 #include <rdma/rdma_netlink.h>
@@ -350,15 +351,19 @@ static struct siw_device *siw_device_create(struct net_device *netdev)
 	sdev->netdev = netdev;
 
 	if (netdev->type != ARPHRD_LOOPBACK) {
-		memcpy(&base_dev->node_guid, netdev->dev_addr, 6);
+		addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
+				    netdev->dev_addr);
 	} else {
 		/*
 		 * The loopback device does not have a HW address,
 		 * but connection mangagement lib expects gid != 0
 		 */
-		size_t gidlen = min_t(size_t, strlen(base_dev->name), 6);
+		size_t len = min_t(size_t, strlen(base_dev->name), 6);
+		char addr[6] = { };
 
-		memcpy(&base_dev->node_guid, base_dev->name, gidlen);
+		memcpy(addr, base_dev->name, len);
+		addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
+				    addr);
 	}
 	base_dev->uverbs_cmd_mask =
 		(1ull << IB_USER_VERBS_CMD_QUERY_DEVICE) |
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 06/15] RDMA/srp: Remove two casts

[Bart Van Assche]

This patch does not change any functionality.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 4 ++--
 drivers/infiniband/ulp/srp/ib_srp.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b5960351bec0..f015dc4ce22c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -352,8 +352,8 @@ static int srp_new_rdma_cm_id(struct srp_rdma_ch *ch)
 
 	init_completion(&ch->done);
 	ret = rdma_resolve_addr(new_cm_id, target->rdma_cm.src_specified ?
-				(struct sockaddr *)&target->rdma_cm.src : NULL,
-				(struct sockaddr *)&target->rdma_cm.dst,
+				&target->rdma_cm.src.sa : NULL,
+				&target->rdma_cm.dst.sa,
 				SRP_PATH_REC_TIMEOUT_MS);
 	if (ret) {
 		pr_err("No route available from %pIS to %pIS (%d)\n",
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index b2861cd2087a..af9922550ae1 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -245,11 +245,13 @@ struct srp_target_port {
 			union {
 				struct sockaddr_in	ip4;
 				struct sockaddr_in6	ip6;
+				struct sockaddr		sa;
 				struct sockaddr_storage ss;
 			} src;
 			union {
 				struct sockaddr_in	ip4;
 				struct sockaddr_in6	ip6;
+				struct sockaddr		sa;
 				struct sockaddr_storage ss;
 			} dst;
 			bool src_specified;
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 07/15] RDMA/srp: Honor the max_send_sge device attribute

[Bart Van Assche]

Instead of assuming that max_send_sge >= 3, restrict the number of scatter
gather elements to what is supported by the RDMA adapter.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 7 +++++--
 drivers/infiniband/ulp/srp/ib_srp.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index f015dc4ce22c..d77e7dd3e745 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -552,6 +552,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
+	const struct ib_device_attr *attr = &dev->dev->attrs;
 	struct ib_qp_init_attr *init_attr;
 	struct ib_cq *recv_cq, *send_cq;
 	struct ib_qp *qp;
@@ -583,12 +584,14 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	init_attr->cap.max_send_wr     = m * target->queue_size;
 	init_attr->cap.max_recv_wr     = target->queue_size + 1;
 	init_attr->cap.max_recv_sge    = 1;
-	init_attr->cap.max_send_sge    = SRP_MAX_SGE;
+	init_attr->cap.max_send_sge    = min(SRP_MAX_SGE, attr->max_send_sge);
 	init_attr->sq_sig_type         = IB_SIGNAL_REQ_WR;
 	init_attr->qp_type             = IB_QPT_RC;
 	init_attr->send_cq             = send_cq;
 	init_attr->recv_cq             = recv_cq;
 
+	ch->max_imm_sge = min(init_attr->cap.max_send_sge - 1U, 255U);
+
 	if (target->using_rdma_cm) {
 		ret = rdma_create_qp(ch->rdma_cm.cm_id, dev->pd, init_attr);
 		qp = ch->rdma_cm.cm_id->qp;
@@ -1838,7 +1841,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 		return -EIO;
 
 	if (ch->use_imm_data &&
-	    count <= SRP_MAX_IMM_SGE &&
+	    count <= ch->max_imm_sge &&
 	    SRP_IMM_DATA_OFFSET + data_len <= ch->max_it_iu_len &&
 	    scmnd->sc_data_direction == DMA_TO_DEVICE) {
 		struct srp_imm_buf *buf;
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index af9922550ae1..f38fbb00d0e8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -161,6 +161,7 @@ struct srp_rdma_ch {
 	};
 	uint32_t		max_it_iu_len;
 	uint32_t		max_ti_iu_len;
+	u8			max_imm_sge;
 	bool			use_imm_data;
 
 	/* Everything above this point is used in the hot path of
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 08/15] RDMA/srp: Make route resolving error messages more informative

[Bart Van Assche]

The IPv6 scope ID is essential when setting up an iWARP connection
between IPv6 link-local addresses. Report the scope ID in error messages.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index d77e7dd3e745..6fddd14b6bd9 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -356,7 +356,7 @@ static int srp_new_rdma_cm_id(struct srp_rdma_ch *ch)
 				&target->rdma_cm.dst.sa,
 				SRP_PATH_REC_TIMEOUT_MS);
 	if (ret) {
-		pr_err("No route available from %pIS to %pIS (%d)\n",
+		pr_err("No route available from %pISpsc to %pISpsc (%d)\n",
 		       &target->rdma_cm.src, &target->rdma_cm.dst, ret);
 		goto out;
 	}
@@ -366,7 +366,7 @@ static int srp_new_rdma_cm_id(struct srp_rdma_ch *ch)
 
 	ret = ch->status;
 	if (ret) {
-		pr_err("Resolving address %pIS failed (%d)\n",
+		pr_err("Resolving address %pISpsc failed (%d)\n",
 		       &target->rdma_cm.dst, ret);
 		goto out;
 	}
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Bart Van Assche]

Management datagrams (MADs) are not supported by SR-IOV VFs nor by iWARP
ports. Support SR-IOV VFs and iWARP ports by only logging an error message
if MAD handler registration fails.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 41 +++++++++++++--------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index e25c70a56be6..4f99a5e040c3 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -556,24 +556,16 @@ static int srpt_refresh_port(struct srpt_port *sport)
 	struct ib_port_attr port_attr;
 	int ret;
 
-	memset(&port_modify, 0, sizeof(port_modify));
-	port_modify.set_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP;
-	port_modify.clr_port_cap_mask = 0;
-
-	ret = ib_modify_port(sport->sdev->device, sport->port, 0, &port_modify);
-	if (ret)
-		goto err_mod_port;
-
 	ret = ib_query_port(sport->sdev->device, sport->port, &port_attr);
 	if (ret)
-		goto err_query_port;
+		return ret;
 
 	sport->sm_lid = port_attr.sm_lid;
 	sport->lid = port_attr.lid;
 
 	ret = rdma_query_gid(sport->sdev->device, sport->port, 0, &sport->gid);
 	if (ret)
-		goto err_query_port;
+		return ret;
 
 	sport->port_guid_wwn.priv = sport;
 	srpt_format_guid(sport->port_guid, sizeof(sport->port_guid),
@@ -584,6 +576,20 @@ static int srpt_refresh_port(struct srpt_port *sport)
 		 be64_to_cpu(sport->gid.global.subnet_prefix),
 		 be64_to_cpu(sport->gid.global.interface_id));
 
+	if (rdma_protocol_iwarp(sport->sdev->device, sport->port))
+		return 0;
+
+	memset(&port_modify, 0, sizeof(port_modify));
+	port_modify.set_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP;
+	port_modify.clr_port_cap_mask = 0;
+
+	ret = ib_modify_port(sport->sdev->device, sport->port, 0, &port_modify);
+	if (ret) {
+		pr_warn("%s-%d: enabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n",
+			dev_name(&sport->sdev->device->dev), sport->port, ret);
+		return 0;
+	}
+
 	if (!sport->mad_agent) {
 		memset(&reg_req, 0, sizeof(reg_req));
 		reg_req.mgmt_class = IB_MGMT_CLASS_DEVICE_MGMT;
@@ -599,23 +605,14 @@ static int srpt_refresh_port(struct srpt_port *sport)
 							 srpt_mad_recv_handler,
 							 sport, 0);
 		if (IS_ERR(sport->mad_agent)) {
-			ret = PTR_ERR(sport->mad_agent);
+			pr_err("%s-%d: MAD agent registration failed (%ld). Note: this is expected if SR-IOV is enabled.\n",
+			       dev_name(&sport->sdev->device->dev), sport->port,
+			       PTR_ERR(sport->mad_agent));
 			sport->mad_agent = NULL;
-			goto err_query_port;
 		}
 	}
 
 	return 0;
-
-err_query_port:
-
-	port_modify.set_port_cap_mask = 0;
-	port_modify.clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP;
-	ib_modify_port(sport->sdev->device, sport->port, 0, &port_modify);
-
-err_mod_port:
-
-	return ret;
 }
 
 /**
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:17:01PM -0700, Bart Van Assche wrote:
> Management datagrams (MADs) are not supported by SR-IOV VFs nor by
> iWARP

Really? This seems surprising to me..

Jason

Re: [PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Bart Van Assche]

On 10/2/19 7:14 AM, Jason Gunthorpe wrote:
> On Mon, Sep 30, 2019 at 04:17:01PM -0700, Bart Van Assche wrote:
>> Management datagrams (MADs) are not supported by SR-IOV VFs nor by
>> iWARP
> 
> Really? This seems surprising to me..

Hi Jason,

Last time I checked the Mellanox drivers allow MADs to be sent over a
SR-IOV VF but do not allow MADs to be received through such a VF.

I haven't been able to find any reference to management datagrams in the
iWARP RFC. Maybe that means that I overlooked something?

Thanks,

Bart.

Re: [PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Jason Gunthorpe]

On Wed, Oct 02, 2019 at 08:21:45AM -0700, Bart Van Assche wrote:
> On 10/2/19 7:14 AM, Jason Gunthorpe wrote:
> > On Mon, Sep 30, 2019 at 04:17:01PM -0700, Bart Van Assche wrote:
> >> Management datagrams (MADs) are not supported by SR-IOV VFs nor by
> >> iWARP
> > 
> > Really? This seems surprising to me..
> 
> Hi Jason,
> 
> Last time I checked the Mellanox drivers allow MADs to be sent over a
> SR-IOV VF but do not allow MADs to be received through such a VF.

I think that is only true of mlx4, mlx5 allows receive, AFAIK

I don't know if registering a mad agent fails though. Jack?

> I haven't been able to find any reference to management datagrams in the
> iWARP RFC. Maybe that means that I overlooked something?

Iwarp makes sense

Jason

Re: [PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Leon Romanovsky]

On Wed, Oct 02, 2019 at 01:51:00PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2019 at 08:21:45AM -0700, Bart Van Assche wrote:
> > On 10/2/19 7:14 AM, Jason Gunthorpe wrote:
> > > On Mon, Sep 30, 2019 at 04:17:01PM -0700, Bart Van Assche wrote:
> > >> Management datagrams (MADs) are not supported by SR-IOV VFs nor by
> > >> iWARP
> > >
> > > Really? This seems surprising to me..
> >
> > Hi Jason,
> >
> > Last time I checked the Mellanox drivers allow MADs to be sent over a
> > SR-IOV VF but do not allow MADs to be received through such a VF.
>
> I think that is only true of mlx4, mlx5 allows receive, AFAIK
>
> I don't know if registering a mad agent fails though. Jack?

According to internal mlx5 specification, MAD is fully operational for
every Virtual HCA used to connect such virtual devices to IBTA
virtualization spec.

 "Each PCI function (PF or VF) represents a vHCA. Each vHCA virtual port
  is mapped to an InfiniBand vport. The mapping is arbitrary and determined
  by the device, as the InfiniBand management is agnostic to it (the
  InfiniBand specification has no notion of hosts or PCI functions)."

Most probably the observed by Bart behaviour is related to the fact that
vport0 has special meaning to allow legacy SMs to connect.

Thanks

>
> > I haven't been able to find any reference to management datagrams in the
> > iWARP RFC. Maybe that means that I overlooked something?
>
> Iwarp makes sense
>
> Jason

Re: [PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Bart Van Assche]

On 10/2/19 10:24 AM, Leon Romanovsky wrote:
> On Wed, Oct 02, 2019 at 01:51:00PM -0300, Jason Gunthorpe wrote:
>> On Wed, Oct 02, 2019 at 08:21:45AM -0700, Bart Van Assche wrote:
>>> On 10/2/19 7:14 AM, Jason Gunthorpe wrote:
>>>> On Mon, Sep 30, 2019 at 04:17:01PM -0700, Bart Van Assche wrote:
>>>>> Management datagrams (MADs) are not supported by SR-IOV VFs nor by
>>>>> iWARP
>>>>
>>>> Really? This seems surprising to me..
>>>
>>> Hi Jason,
>>>
>>> Last time I checked the Mellanox drivers allow MADs to be sent over a
>>> SR-IOV VF but do not allow MADs to be received through such a VF.
>>
>> I think that is only true of mlx4, mlx5 allows receive, AFAIK
>>
>> I don't know if registering a mad agent fails though. Jack?
> 
> According to internal mlx5 specification, MAD is fully operational for
> every Virtual HCA used to connect such virtual devices to IBTA
> virtualization spec.
> 
>   "Each PCI function (PF or VF) represents a vHCA. Each vHCA virtual port
>    is mapped to an InfiniBand vport. The mapping is arbitrary and determined
>    by the device, as the InfiniBand management is agnostic to it (the
>    InfiniBand specification has no notion of hosts or PCI functions)."
> 
> Most probably the observed by Bart behaviour is related to the fact that
> vport0 has special meaning to allow legacy SMs to connect.

Hi Jason and Leon,

Is it essential that we figure out which HCAs support MADs for VFs or is 
it perhaps sufficient that I change the description of this patch such 
that it mentions that device management and MAD support is not 
guaranteed to be available?

Thanks,

Bart.

Re: [PATCH 09/15] RDMA/srpt: Fix handling of SR-IOV and iWARP ports

[Leon Romanovsky]

On Wed, Oct 02, 2019 at 10:43:59AM -0700, Bart Van Assche wrote:
> On 10/2/19 10:24 AM, Leon Romanovsky wrote:
> > On Wed, Oct 02, 2019 at 01:51:00PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Oct 02, 2019 at 08:21:45AM -0700, Bart Van Assche wrote:
> > > > On 10/2/19 7:14 AM, Jason Gunthorpe wrote:
> > > > > On Mon, Sep 30, 2019 at 04:17:01PM -0700, Bart Van Assche wrote:
> > > > > > Management datagrams (MADs) are not supported by SR-IOV VFs nor by
> > > > > > iWARP
> > > > >
> > > > > Really? This seems surprising to me..
> > > >
> > > > Hi Jason,
> > > >
> > > > Last time I checked the Mellanox drivers allow MADs to be sent over a
> > > > SR-IOV VF but do not allow MADs to be received through such a VF.
> > >
> > > I think that is only true of mlx4, mlx5 allows receive, AFAIK
> > >
> > > I don't know if registering a mad agent fails though. Jack?
> >
> > According to internal mlx5 specification, MAD is fully operational for
> > every Virtual HCA used to connect such virtual devices to IBTA
> > virtualization spec.
> >
> >   "Each PCI function (PF or VF) represents a vHCA. Each vHCA virtual port
> >    is mapped to an InfiniBand vport. The mapping is arbitrary and determined
> >    by the device, as the InfiniBand management is agnostic to it (the
> >    InfiniBand specification has no notion of hosts or PCI functions)."
> >
> > Most probably the observed by Bart behaviour is related to the fact that
> > vport0 has special meaning to allow legacy SMs to connect.
>
> Hi Jason and Leon,
>
> Is it essential that we figure out which HCAs support MADs for VFs or is it
> perhaps sufficient that I change the description of this patch such that it
> mentions that device management and MAD support is not guaranteed to be
> available?

I have no idea, sorry.

>
> Thanks,
>
> Bart.
>

[PATCH 10/15] RDMA/srpt: Fix handling of iWARP logins

[Bart Van Assche]

The path_rec pointer is NULL set for IB and RoCE logins but not for iWARP
logins. Hence check the path_rec pointer before dereferencing it.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 4f99a5e040c3..fbfadeedc195 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2502,6 +2502,7 @@ static int srpt_rdma_cm_req_recv(struct rdma_cm_id *cm_id,
 	struct srpt_device *sdev;
 	struct srp_login_req req;
 	const struct srp_login_req_rdma *req_rdma;
+	struct sa_path_rec *path_rec = cm_id->route.path_rec;
 	char src_addr[40];
 
 	sdev = ib_get_client_data(cm_id->device, &srpt_client);
@@ -2527,7 +2528,7 @@ static int srpt_rdma_cm_req_recv(struct rdma_cm_id *cm_id,
 		 &cm_id->route.addr.src_addr);
 
 	return srpt_cm_req_recv(sdev, NULL, cm_id, cm_id->port_num,
-				cm_id->route.path_rec->pkey, &req, src_addr);
+				path_rec ? path_rec->pkey : 0, &req, src_addr);
 }
 
 static void srpt_cm_rej_recv(struct srpt_rdma_ch *ch,
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 10/15] RDMA/srpt: Fix handling of iWARP logins

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:17:02PM -0700, Bart Van Assche wrote:
> The path_rec pointer is NULL set for IB and RoCE logins but not for iWARP
> logins. Hence check the path_rec pointer before dereferencing it.

Did you mean it is null set for iWARP logins? I would expect iwarp to
not have a pkey..

Jason

Re: [PATCH 10/15] RDMA/srpt: Fix handling of iWARP logins

[Bart Van Assche]

On 10/2/19 7:16 AM, Jason Gunthorpe wrote:
> On Mon, Sep 30, 2019 at 04:17:02PM -0700, Bart Van Assche wrote:
>> The path_rec pointer is NULL set for IB and RoCE logins but not for iWARP
>> logins. Hence check the path_rec pointer before dereferencing it.
> 
> Did you mean it is null set for iWARP logins? I would expect iwarp to
> not have a pkey..

Yes, I meant that the path_rec pointer is NULL for iWARP logins. The
word "NULL" should be removed from the commit message.

Thanks,

Bart.

[PATCH 11/15] RDMA/srpt: Improve a debug message

[Bart Van Assche]

The ib_srpt driver uses two different identifiers while registering a
session with the LIO core. Report both identifiers if the modified
pr_debug() statement is enabled.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index fbfadeedc195..dabaea301328 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2293,7 +2293,8 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 			be64_to_cpu(*(__be64 *)nexus->i_port_id),
 			be64_to_cpu(*(__be64 *)(nexus->i_port_id + 8)));
 
-	pr_debug("registering session %s\n", ch->sess_name);
+	pr_debug("registering src addr %s or i_port_id %s\n", ch->sess_name,
+		 i_port_id);
 
 	tag_num = ch->rq_size;
 	tag_size = 1; /* ib_srpt does not use se_sess->sess_cmd_map */
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 12/15] RDMA/srpt: Rework the approach for closing an RDMA channel

[Bart Van Assche]

Instead of relying on a waitqueue, report when the identity of an RDMA
channel can be reused through a completion.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 30 +++++++--------------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  3 +++
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index dabaea301328..88e11a250c96 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1928,41 +1928,22 @@ static int srpt_disconnect_ch(struct srpt_rdma_ch *ch)
 	return ret;
 }
 
-static bool srpt_ch_closed(struct srpt_port *sport, struct srpt_rdma_ch *ch)
-{
-	struct srpt_nexus *nexus;
-	struct srpt_rdma_ch *ch2;
-	bool res = true;
-
-	rcu_read_lock();
-	list_for_each_entry(nexus, &sport->nexus_list, entry) {
-		list_for_each_entry(ch2, &nexus->ch_list, list) {
-			if (ch2 == ch) {
-				res = false;
-				goto done;
-			}
-		}
-	}
-done:
-	rcu_read_unlock();
-
-	return res;
-}
-
 /* Send DREQ and wait for DREP. */
 static void srpt_disconnect_ch_sync(struct srpt_rdma_ch *ch)
 {
+	DECLARE_COMPLETION_ONSTACK(closed);
 	struct srpt_port *sport = ch->sport;
 
 	pr_debug("ch %s-%d state %d\n", ch->sess_name, ch->qp->qp_num,
 		 ch->state);
 
+	ch->closed = &closed;
+
 	mutex_lock(&sport->mutex);
 	srpt_disconnect_ch(ch);
 	mutex_unlock(&sport->mutex);
 
-	while (wait_event_timeout(sport->ch_releaseQ, srpt_ch_closed(sport, ch),
-				  5 * HZ) == 0)
+	while (wait_for_completion_timeout(&closed, 5 * HZ) == 0)
 		pr_info("%s(%s-%d state %d): still waiting ...\n", __func__,
 			ch->sess_name, ch->qp->qp_num, ch->state);
 
@@ -2089,6 +2070,9 @@ static void srpt_release_channel_work(struct work_struct *w)
 	list_del_rcu(&ch->list);
 	mutex_unlock(&sport->mutex);
 
+	if (ch->closed)
+		complete(ch->closed);
+
 	srpt_destroy_ch_ib(ch);
 
 	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index ee9f20e9177a..d0955bc0343d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -264,6 +264,8 @@ enum rdma_ch_state {
  * @zw_cqe:	   Zero-length write CQE.
  * @rcu:           RCU head.
  * @kref:	   kref for this channel.
+ * @closed:	   Completion object that will be signaled as soon as a new
+ *		   channel object with the same identity can be created.
  * @rq_size:       IB receive queue size.
  * @max_rsp_size:  Maximum size of an RSP response message in bytes.
  * @sq_wr_avail:   number of work requests available in the send queue.
@@ -306,6 +308,7 @@ struct srpt_rdma_ch {
 	struct ib_cqe		zw_cqe;
 	struct rcu_head		rcu;
 	struct kref		kref;
+	struct completion	*closed;
 	int			rq_size;
 	u32			max_rsp_size;
 	atomic_t		sq_wr_avail;
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 13/15] RDMA/srpt: Rework the code that waits until an RDMA port is no longer in use

[Bart Van Assche]

The current implementation does not wait until srpt_release_channel()
has finished and hence can trigger a use-after-free. Rework
srpt_release_sport() such that it waits until srpt_release_channel()
has finished. This patch fixes the following KASAN complaint:

==================================================================
BUG: KASAN: use-after-free in srpt_free_ioctx.part.23+0x42/0x100 [ib_srpt]
Read of size 8 at addr ffff888115c71100 by task kworker/4:3/807

CPU: 4 PID: 807 Comm: kworker/4:3 Not tainted 5.3.0-dbg+ #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
Workqueue: events srpt_release_channel_work [ib_srpt]
Call Trace:
 dump_stack+0x86/0xca
 print_address_description+0x74/0x32d
 __kasan_report.cold.6+0x1b/0x36
 kasan_report+0x12/0x17
 __asan_load8+0x54/0x90
 srpt_free_ioctx.part.23+0x42/0x100 [ib_srpt]
 srpt_free_ioctx_ring.part.24+0x50/0x80 [ib_srpt]
 srpt_release_channel_work+0x2ad/0x390 [ib_srpt]
 process_one_work+0x51a/0xa60
 worker_thread+0x67/0x5b0
 kthread+0x1dc/0x200
 ret_from_fork+0x24/0x30

Allocated by task 984:
 save_stack+0x21/0x90
 __kasan_kmalloc.constprop.9+0xc7/0xd0
 kasan_kmalloc+0x9/0x10
 __kmalloc+0x153/0x370
 srpt_add_one+0x4f/0x570 [ib_srpt]
 add_client_context+0x251/0x290 [ib_core]
 ib_register_client+0x1da/0x220 [ib_core]
 iblock_get_alignment_offset_lbas+0x6b/0x100 [target_core_iblock]
 do_one_initcall+0xcd/0x43a
 do_init_module+0x103/0x380
 load_module+0x3b77/0x3eb0
 __do_sys_finit_module+0x12d/0x1b0
 __x64_sys_finit_module+0x43/0x50
 do_syscall_64+0x6b/0x2d0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 1128:
 save_stack+0x21/0x90
 __kasan_slab_free+0x139/0x190
 kasan_slab_free+0xe/0x10
 slab_free_freelist_hook+0x67/0x1e0
 kfree+0xcb/0x2a0
 srpt_remove_one+0x569/0x5b0 [ib_srpt]
 remove_client_context+0x9a/0xe0 [ib_core]
 disable_device+0x106/0x1b0 [ib_core]
 __ib_unregister_device+0x5f/0xf0 [ib_core]
 ib_unregister_device_and_put+0x48/0x60 [ib_core]
 nldev_dellink+0x120/0x180 [ib_core]
 rdma_nl_rcv+0x287/0x480 [ib_core]
 netlink_unicast+0x2cc/0x370
 netlink_sendmsg+0x3b1/0x630
 __sys_sendto+0x1db/0x290
 __x64_sys_sendto+0x80/0xa0
 do_syscall_64+0x6b/0x2d0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

The buggy address belongs to the object at ffff888115c71100
 which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 0 bytes inside of
 4096-byte region [ffff888115c71100, ffff888115c72100)
The buggy address belongs to the page:
page:ffffea0004571c00 refcount:1 mapcount:0 mapping:ffff88811ac0de00 index:0xffff888115c70000 compound_mapcount: 0
flags: 0x2fff000000010200(slab|head)
raw: 2fff000000010200 ffffea00045ac408 ffffea0004593208 ffff88811ac0de00
raw: ffff888115c70000 0000000000070002 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff888115c71000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff888115c71080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888115c71100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                   ^
 ffff888115c71180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888115c71200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 42 +++++++++++++--------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  6 ++--
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 88e11a250c96..5e402f7b9692 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2023,10 +2023,17 @@ static void srpt_set_enabled(struct srpt_port *sport, bool enabled)
 		__srpt_close_all_ch(sport);
 }
 
+static void srpt_drop_sport_ref(struct srpt_port *sport)
+{
+	if (atomic_dec_return(&sport->refcount) == 0 && sport->freed_channels)
+		complete(sport->freed_channels);
+}
+
 static void srpt_free_ch(struct kref *kref)
 {
 	struct srpt_rdma_ch *ch = container_of(kref, struct srpt_rdma_ch, kref);
 
+	srpt_drop_sport_ref(ch->sport);
 	kfree_rcu(ch, rcu);
 }
 
@@ -2087,8 +2094,6 @@ static void srpt_release_channel_work(struct work_struct *w)
 
 	kmem_cache_destroy(ch->req_buf_cache);
 
-	wake_up(&sport->ch_releaseQ);
-
 	kref_put(&ch->kref, srpt_free_ch);
 }
 
@@ -2307,6 +2312,12 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 		goto destroy_ib;
 	}
 
+	/*
+	 * Once a session has been created destruction of srpt_rdma_ch objects
+	 * will decrement sport->refcount. Hence increment sport->refcount now.
+	 */
+	atomic_inc(&sport->refcount);
+
 	mutex_lock(&sport->mutex);
 
 	if ((req->req_flags & SRP_MTCH_ACTION) == SRP_MULTICHAN_SINGLE) {
@@ -2889,39 +2900,29 @@ static void srpt_refresh_port_work(struct work_struct *work)
 	srpt_refresh_port(sport);
 }
 
-static bool srpt_ch_list_empty(struct srpt_port *sport)
-{
-	struct srpt_nexus *nexus;
-	bool res = true;
-
-	rcu_read_lock();
-	list_for_each_entry(nexus, &sport->nexus_list, entry)
-		if (!list_empty(&nexus->ch_list))
-			res = false;
-	rcu_read_unlock();
-
-	return res;
-}
-
 /**
  * srpt_release_sport - disable login and wait for associated channels
  * @sport: SRPT HCA port.
  */
 static int srpt_release_sport(struct srpt_port *sport)
 {
+	DECLARE_COMPLETION_ONSTACK(c);
 	struct srpt_nexus *nexus, *next_n;
 	struct srpt_rdma_ch *ch;
 
 	WARN_ON_ONCE(irqs_disabled());
 
+	sport->freed_channels = &c;
+
 	mutex_lock(&sport->mutex);
 	srpt_set_enabled(sport, false);
 	mutex_unlock(&sport->mutex);
 
-	while (wait_event_timeout(sport->ch_releaseQ,
-				  srpt_ch_list_empty(sport), 5 * HZ) <= 0) {
-		pr_info("%s_%d: waiting for session unregistration ...\n",
-			dev_name(&sport->sdev->device->dev), sport->port);
+	while (atomic_read(&sport->refcount) > 0 &&
+	       wait_for_completion_timeout(&c, 5 * HZ) <= 0) {
+		pr_info("%s_%d: waiting for unregistration of %d sessions ...\n",
+			dev_name(&sport->sdev->device->dev), sport->port,
+			atomic_read(&sport->refcount));
 		rcu_read_lock();
 		list_for_each_entry(nexus, &sport->nexus_list, entry) {
 			list_for_each_entry(ch, &nexus->ch_list, list) {
@@ -3130,7 +3131,6 @@ static void srpt_add_one(struct ib_device *device)
 	for (i = 1; i <= sdev->device->phys_port_cnt; i++) {
 		sport = &sdev->port[i - 1];
 		INIT_LIST_HEAD(&sport->nexus_list);
-		init_waitqueue_head(&sport->ch_releaseQ);
 		mutex_init(&sport->mutex);
 		sport->sdev = sdev;
 		sport->port = i;
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index d0955bc0343d..f3df791dd877 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -381,7 +381,8 @@ struct srpt_port_attrib {
  * @port_gid_tpg:  TPG associated with target port GID.
  * @port_gid_wwn:  WWN associated with target port GID.
  * @port_attrib:   Port attributes that can be accessed through configfs.
- * @ch_releaseQ:   Enables waiting for removal from nexus_list.
+ * @refcount:	   Number of objects associated with this port.
+ * @freed_channels: Completion that will be signaled once @refcount becomes 0.
  * @mutex:	   Protects nexus_list.
  * @nexus_list:	   Nexus list. See also srpt_nexus.entry.
  */
@@ -401,7 +402,8 @@ struct srpt_port {
 	struct se_portal_group	port_gid_tpg;
 	struct se_wwn		port_gid_wwn;
 	struct srpt_port_attrib port_attrib;
-	wait_queue_head_t	ch_releaseQ;
+	atomic_t		refcount;
+	struct completion	*freed_channels;
 	struct mutex		mutex;
 	struct list_head	nexus_list;
 };
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 14/15] RDMA/srpt: Make the code for handling port identities more systematic

[Bart Van Assche]

Introduce a new data structure for the information about an RDMA port
name. This patch does not change any functionality.

Cc: Honggang LI <honli@redhat.com>
Cc: Laurence Oberman <loberman@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 64 +++++++++++++++++----------
 drivers/infiniband/ulp/srpt/ib_srpt.h | 25 +++++++----
 2 files changed, 57 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 5e402f7b9692..0582b3d4ec4d 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -567,11 +567,12 @@ static int srpt_refresh_port(struct srpt_port *sport)
 	if (ret)
 		return ret;
 
-	sport->port_guid_wwn.priv = sport;
-	srpt_format_guid(sport->port_guid, sizeof(sport->port_guid),
+	sport->port_guid_id.wwn.priv = sport;
+	srpt_format_guid(sport->port_guid_id.name,
+			 sizeof(sport->port_guid_id.name),
 			 &sport->gid.global.interface_id);
-	sport->port_gid_wwn.priv = sport;
-	snprintf(sport->port_gid, sizeof(sport->port_gid),
+	sport->port_gid_id.wwn.priv = sport;
+	snprintf(sport->port_gid_id.name, sizeof(sport->port_gid_id.name),
 		 "0x%016llx%016llx",
 		 be64_to_cpu(sport->gid.global.subnet_prefix),
 		 be64_to_cpu(sport->gid.global.interface_id));
@@ -2287,17 +2288,17 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev,
 
 	tag_num = ch->rq_size;
 	tag_size = 1; /* ib_srpt does not use se_sess->sess_cmd_map */
-	if (sport->port_guid_tpg.se_tpg_wwn)
-		ch->sess = target_setup_session(&sport->port_guid_tpg, tag_num,
+	if (sport->port_guid_id.tpg.se_tpg_wwn)
+		ch->sess = target_setup_session(&sport->port_guid_id.tpg, tag_num,
 						tag_size, TARGET_PROT_NORMAL,
 						ch->sess_name, ch, NULL);
-	if (sport->port_gid_tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
-		ch->sess = target_setup_session(&sport->port_gid_tpg, tag_num,
+	if (sport->port_gid_id.tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
+		ch->sess = target_setup_session(&sport->port_gid_id.tpg, tag_num,
 					tag_size, TARGET_PROT_NORMAL, i_port_id,
 					ch, NULL);
 	/* Retry without leading "0x" */
-	if (sport->port_gid_tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
-		ch->sess = target_setup_session(&sport->port_gid_tpg, tag_num,
+	if (sport->port_gid_id.tpg.se_tpg_wwn && IS_ERR_OR_NULL(ch->sess))
+		ch->sess = target_setup_session(&sport->port_gid_id.tpg, tag_num,
 						tag_size, TARGET_PROT_NORMAL,
 						i_port_id + 2, ch, NULL);
 	if (IS_ERR_OR_NULL(ch->sess)) {
@@ -2959,10 +2960,10 @@ static struct se_wwn *__srpt_lookup_wwn(const char *name)
 		for (i = 0; i < dev->phys_port_cnt; i++) {
 			sport = &sdev->port[i];
 
-			if (strcmp(sport->port_guid, name) == 0)
-				return &sport->port_guid_wwn;
-			if (strcmp(sport->port_gid, name) == 0)
-				return &sport->port_gid_wwn;
+			if (strcmp(sport->port_guid_id.name, name) == 0)
+				return &sport->port_guid_id.wwn;
+			if (strcmp(sport->port_gid_id.name, name) == 0)
+				return &sport->port_gid_id.wwn;
 		}
 	}
 
@@ -3241,14 +3242,33 @@ static struct srpt_port *srpt_tpg_to_sport(struct se_portal_group *tpg)
 	return tpg->se_tpg_wwn->priv;
 }
 
-static char *srpt_get_fabric_wwn(struct se_portal_group *tpg)
+static struct srpt_port_id *srpt_tpg_to_sport_id(struct se_portal_group *tpg)
 {
 	struct srpt_port *sport = srpt_tpg_to_sport(tpg);
 
-	WARN_ON_ONCE(tpg != &sport->port_guid_tpg &&
-		     tpg != &sport->port_gid_tpg);
-	return tpg == &sport->port_guid_tpg ? sport->port_guid :
-		sport->port_gid;
+	if (tpg == &sport->port_guid_id.tpg)
+		return &sport->port_guid_id;
+	if (tpg == &sport->port_gid_id.tpg)
+		return &sport->port_gid_id;
+	WARN_ON_ONCE(true);
+	return NULL;
+}
+
+static struct srpt_port_id *srpt_wwn_to_sport_id(struct se_wwn *wwn)
+{
+	struct srpt_port *sport = wwn->priv;
+
+	if (wwn == &sport->port_guid_id.wwn)
+		return &sport->port_guid_id;
+	if (wwn == &sport->port_gid_id.wwn)
+		return &sport->port_gid_id;
+	WARN_ON_ONCE(true);
+	return NULL;
+}
+
+static char *srpt_get_fabric_wwn(struct se_portal_group *tpg)
+{
+	return srpt_tpg_to_sport_id(tpg)->name;
 }
 
 static u16 srpt_get_tag(struct se_portal_group *tpg)
@@ -3705,13 +3725,9 @@ static struct se_portal_group *srpt_make_tpg(struct se_wwn *wwn,
 					     const char *name)
 {
 	struct srpt_port *sport = wwn->priv;
-	struct se_portal_group *tpg;
+	struct se_portal_group *tpg = &srpt_wwn_to_sport_id(wwn)->tpg;
 	int res;
 
-	WARN_ON_ONCE(wwn != &sport->port_guid_wwn &&
-		     wwn != &sport->port_gid_wwn);
-	tpg = wwn == &sport->port_guid_wwn ? &sport->port_guid_tpg :
-		&sport->port_gid_tpg;
 	res = core_tpg_register(wwn, tpg, SCSI_PROTOCOL_SRP);
 	if (res)
 		return ERR_PTR(res);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index f3df791dd877..f8bd95302ac0 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -363,13 +363,26 @@ struct srpt_port_attrib {
 	bool			use_srq;
 };
 
+/**
+ * struct srpt_port_id - information about an RDMA port name
+ * @tpg: TPG associated with the RDMA port.
+ * @wwn: WWN associated with the RDMA port.
+ * @name: ASCII representation of the port name.
+ *
+ * Multiple sysfs directories can be associated with a single RDMA port. This
+ * data structure represents a single (port, name) pair.
+ */
+struct srpt_port_id {
+	struct se_portal_group	tpg;
+	struct se_wwn		wwn;
+	char			name[64];
+};
+
 /**
  * struct srpt_port - information associated by SRPT with a single IB port
  * @sdev:      backpointer to the HCA information.
  * @mad_agent: per-port management datagram processing information.
  * @enabled:   Whether or not this target port is enabled.
- * @port_guid: ASCII representation of Port GUID
- * @port_gid:  ASCII representation of Port GID
  * @port:      one-based port number.
  * @sm_lid:    cached value of the port's sm_lid.
  * @lid:       cached value of the port's lid.
@@ -390,17 +403,13 @@ struct srpt_port {
 	struct srpt_device	*sdev;
 	struct ib_mad_agent	*mad_agent;
 	bool			enabled;
-	u8			port_guid[24];
-	u8			port_gid[64];
 	u8			port;
 	u32			sm_lid;
 	u32			lid;
 	union ib_gid		gid;
 	struct work_struct	work;
-	struct se_portal_group	port_guid_tpg;
-	struct se_wwn		port_guid_wwn;
-	struct se_portal_group	port_gid_tpg;
-	struct se_wwn		port_gid_wwn;
+	struct srpt_port_id	port_guid_id;
+	struct srpt_port_id	port_gid_id;
 	struct srpt_port_attrib port_attrib;
 	atomic_t		refcount;
 	struct completion	*freed_channels;
-- 
2.23.0.444.g18eeb5a265-goog

[PATCH 15/15] RDMA/srpt: Postpone HCA removal until after configfs directory removal

[Bart Van Assche]

A shortcoming of the SCSI target core is that it does not have an API
for removing tpg or wwn objects. Wait until these directories have been
removed before allowing HCA removal to finish.

See also Bart Van Assche, "Re: Why using configfs as the only interface is
wrong for a storage target", 2011-02-07
(https://www.spinics.net/lists/linux-scsi/msg50248.html).

This patch fixes the following kernel crash:

==================================================================
BUG: KASAN: use-after-free in __configfs_open_file.isra.4+0x1a8/0x400
Read of size 8 at addr ffff88811880b690 by task restart-lio-srp/1215

CPU: 1 PID: 1215 Comm: restart-lio-srp Not tainted 5.3.0-dbg+ #3
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
Call Trace:
 dump_stack+0x86/0xca
 print_address_description+0x74/0x32d
 __kasan_report.cold.6+0x1b/0x36
 kasan_report+0x12/0x17
 __asan_load8+0x54/0x90
 __configfs_open_file.isra.4+0x1a8/0x400
 configfs_open_file+0x13/0x20
 do_dentry_open+0x2b1/0x770
 vfs_open+0x58/0x60
 path_openat+0x5fa/0x14b0
 do_filp_open+0x115/0x180
 do_sys_open+0x1d4/0x2a0
 __x64_sys_openat+0x59/0x70
 do_syscall_64+0x6b/0x2d0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f2f2bd3fcce
Code: 25 00 00 41 00 3d 00 00 41 00 74 48 48 8d 05 19 d7 0d 00 8b 00 85 c0 75 69 89 f2 b8 01 01 00 00 48 89 fe bf 9c ff ff ff 0f 05 <48> 3d 00 f0 ff ff 0f 87 a6 00 00 00 48 8b 4c 24 28 64 48 33 0c 25
RSP: 002b:00007ffd155f7850 EFLAGS: 00000246 ORIG_RAX: 0000000000000101
RAX: ffffffffffffffda RBX: 0000564609ba88e0 RCX: 00007f2f2bd3fcce
RDX: 0000000000000241 RSI: 0000564609ba8cf0 RDI: 00000000ffffff9c
RBP: 00007ffd155f7950 R08: 0000000000000000 R09: 0000000000000020
R10: 00000000000001b6 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000003 R14: 0000000000000001 R15: 0000564609ba8cf0

Allocated by task 995:
 save_stack+0x21/0x90
 __kasan_kmalloc.constprop.9+0xc7/0xd0
 kasan_kmalloc+0x9/0x10
 __kmalloc+0x153/0x370
 srpt_add_one+0x4f/0x561 [ib_srpt]
 add_client_context+0x251/0x290 [ib_core]
 ib_register_client+0x1da/0x220 [ib_core]
 iblock_get_alignment_offset_lbas+0x6b/0x100 [target_core_iblock]
 do_one_initcall+0xcd/0x43a
 do_init_module+0x103/0x380
 load_module+0x3b77/0x3eb0
 __do_sys_finit_module+0x12d/0x1b0
 __x64_sys_finit_module+0x43/0x50
 do_syscall_64+0x6b/0x2d0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 1221:
 save_stack+0x21/0x90
 __kasan_slab_free+0x139/0x190
 kasan_slab_free+0xe/0x10
 slab_free_freelist_hook+0x67/0x1e0
 kfree+0xcb/0x2a0
 srpt_remove_one+0x596/0x670 [ib_srpt]
 remove_client_context+0x9a/0xe0 [ib_core]
 disable_device+0x106/0x1b0 [ib_core]
 __ib_unregister_device+0x5f/0xf0 [ib_core]
 ib_unregister_driver+0x11a/0x170 [ib_core]
 0xffffffffa087f666
 __x64_sys_delete_module+0x1f8/0x2c0
 do_syscall_64+0x6b/0x2d0
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

The buggy address belongs to the object at ffff88811880b300
 which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 912 bytes inside of
 4096-byte region [ffff88811880b300, ffff88811880c300)
The buggy address belongs to the page:
page:ffffea0004620200 refcount:1 mapcount:0 mapping:ffff88811ac0de00 index:0x0 compound_mapcount: 0
flags: 0x2fff000000010200(slab|head)
raw: 2fff000000010200 dead000000000100 dead000000000122 ffff88811ac0de00
raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88811880b580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88811880b600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff88811880b680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                         ^
 ffff88811880b700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88811880b780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0582b3d4ec4d..daf811abf40a 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2921,7 +2921,7 @@ static int srpt_release_sport(struct srpt_port *sport)
 
 	while (atomic_read(&sport->refcount) > 0 &&
 	       wait_for_completion_timeout(&c, 5 * HZ) <= 0) {
-		pr_info("%s_%d: waiting for unregistration of %d sessions ...\n",
+		pr_info("%s_%d: waiting for unregistration of %d sessions and configfs directories ...\n",
 			dev_name(&sport->sdev->device->dev), sport->port,
 			atomic_read(&sport->refcount));
 		rcu_read_lock();
@@ -3732,6 +3732,8 @@ static struct se_portal_group *srpt_make_tpg(struct se_wwn *wwn,
 	if (res)
 		return ERR_PTR(res);
 
+	atomic_inc(&sport->refcount);
+
 	return tpg;
 }
 
@@ -3745,6 +3747,7 @@ static void srpt_drop_tpg(struct se_portal_group *tpg)
 
 	sport->enabled = false;
 	core_tpg_deregister(tpg);
+	srpt_drop_sport_ref(sport);
 }
 
 /**
-- 
2.23.0.444.g18eeb5a265-goog

Re: [PATCH 03/15] RDMA/siw: Simplify several debug messages

[Bernard Metzler]

-----"Bart Van Assche" <bvanassche@acm.org> wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: "Bart Van Assche" <bvanassche@acm.org>
>Date: 10/01/2019 01:17AM
>Cc: "Leon Romanovsky" <leonro@mellanox.com>, "Doug Ledford"
><dledford@redhat.com>, linux-rdma@vger.kernel.org, "Bart Van Assche"
><bvanassche@acm.org>, "Bernard Metzler" <bmt@zurich.ibm.com>
>Subject: [EXTERNAL] [PATCH 03/15] RDMA/siw: Simplify several debug
>messages
>
>Do not print the remote address if it is not used. Use %pISp instead
>of %pI4 %d.
>
>Cc: Bernard Metzler <bmt@zurich.ibm.com>
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>---
> drivers/infiniband/sw/siw/siw_cm.c | 36
>++++++------------------------
> 1 file changed, 7 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 8c1931a57f4a..5a75deb9870b 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1373,22 +1373,8 @@ int siw_connect(struct iw_cm_id *id, struct
>iw_cm_conn_param *params)
> 		rv = -EINVAL;
> 		goto error;
> 	}
>-	if (v4)
>-		siw_dbg_qp(qp,
>-			   "pd_len %d, laddr %pI4 %d, raddr %pI4 %d\n",
>-			   pd_len,
>-			   &((struct sockaddr_in *)(laddr))->sin_addr,
>-			   ntohs(((struct sockaddr_in *)(laddr))->sin_port),
>-			   &((struct sockaddr_in *)(raddr))->sin_addr,
>-			   ntohs(((struct sockaddr_in *)(raddr))->sin_port));
>-	else
>-		siw_dbg_qp(qp,
>-			   "pd_len %d, laddr %pI6 %d, raddr %pI6 %d\n",
>-			   pd_len,
>-			   &((struct sockaddr_in6 *)(laddr))->sin6_addr,
>-			   ntohs(((struct sockaddr_in6 *)(laddr))->sin6_port),
>-			   &((struct sockaddr_in6 *)(raddr))->sin6_addr,
>-			   ntohs(((struct sockaddr_in6 *)(raddr))->sin6_port));
>+	siw_dbg_qp(qp, "pd_len %d, laddr %pISp, raddr %pISp\n", pd_len,
>laddr,
>+		   raddr);
> 
> 	rv = sock_create(v4 ? AF_INET : AF_INET6, SOCK_STREAM, IPPROTO_TCP,
>&s);
> 	if (rv < 0)
>@@ -1935,7 +1921,7 @@ static void siw_drop_listeners(struct iw_cm_id
>*id)
> /*
>  * siw_create_listen - Create resources for a listener's IWCM ID @id
>  *
>- * Listens on the socket addresses id->local_addr and
>id->remote_addr.
>+ * Listens on the socket address id->local_addr.
>  *
>  * If the listener's @id provides a specific local IP address, at
>most one
>  * listening socket is created and associated with @id.
>@@ -1959,7 +1945,7 @@ int siw_create_listen(struct iw_cm_id *id, int
>backlog)
> 	 */
> 	if (id->local_addr.ss_family == AF_INET) {
> 		struct in_device *in_dev = in_dev_get(dev);
>-		struct sockaddr_in s_laddr, *s_raddr;
>+		struct sockaddr_in s_laddr;
> 		const struct in_ifaddr *ifa;
> 
> 		if (!in_dev) {
>@@ -1967,12 +1953,8 @@ int siw_create_listen(struct iw_cm_id *id, int
>backlog)
> 			goto out;
> 		}
> 		memcpy(&s_laddr, &id->local_addr, sizeof(s_laddr));
>-		s_raddr = (struct sockaddr_in *)&id->remote_addr;
> 
>-		siw_dbg(id->device,
>-			"laddr %pI4:%d, raddr %pI4:%d\n",
>-			&s_laddr.sin_addr, ntohs(s_laddr.sin_port),
>-			&s_raddr->sin_addr, ntohs(s_raddr->sin_port));
>+		siw_dbg(id->device, "laddr %pISp\n", &s_laddr);
> 
> 		rtnl_lock();
> 		in_dev_for_each_ifa_rtnl(ifa, in_dev) {
>@@ -1992,17 +1974,13 @@ int siw_create_listen(struct iw_cm_id *id,
>int backlog)
> 	} else if (id->local_addr.ss_family == AF_INET6) {
> 		struct inet6_dev *in6_dev = in6_dev_get(dev);
> 		struct inet6_ifaddr *ifp;
>-		struct sockaddr_in6 *s_laddr = &to_sockaddr_in6(id->local_addr),
>-			*s_raddr = &to_sockaddr_in6(id->remote_addr);
>+		struct sockaddr_in6 *s_laddr = &to_sockaddr_in6(id->local_addr);
> 
> 		if (!in6_dev) {
> 			rv = -ENODEV;
> 			goto out;
> 		}
>-		siw_dbg(id->device,
>-			"laddr %pI6:%d, raddr %pI6:%d\n",
>-			&s_laddr->sin6_addr, ntohs(s_laddr->sin6_port),
>-			&s_raddr->sin6_addr, ntohs(s_raddr->sin6_port));
>+		siw_dbg(id->device, "laddr %pISp\n", &s_laddr);
> 
> 		rtnl_lock();
> 		list_for_each_entry(ifp, &in6_dev->addr_list, if_list) {
>-- 
>2.23.0.444.g18eeb5a265-goog
>
>

Thanks Bart! Obviously, I wasn't aware of %pISp formatting...
Looks so much prettier!

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 04/15] RDMA/siw: Fix port number endianness in a debug message

[Bernard Metzler]

-----"Bart Van Assche" <bvanassche@acm.org> wrote: -----

>To: "Jason Gunthorpe" <jgg@ziepe.ca>
>From: "Bart Van Assche" <bvanassche@acm.org>
>Date: 10/01/2019 01:17AM
>Cc: "Leon Romanovsky" <leonro@mellanox.com>, "Doug Ledford"
><dledford@redhat.com>, linux-rdma@vger.kernel.org, "Bart Van Assche"
><bvanassche@acm.org>, "Bernard Metzler" <bmt@zurich.ibm.com>
>Subject: [EXTERNAL] [PATCH 04/15] RDMA/siw: Fix port number
>endianness in a debug message
>
>sin_port and sin6_port are big endian member variables. Convert these
>port
>numbers into CPU endianness before printing.
>
>Cc: Bernard Metzler <bmt@zurich.ibm.com>
>Fixes: 6c52fdc244b5 ("rdma/siw: connection management")
>Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>---
> drivers/infiniband/sw/siw/siw_cm.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_cm.c
>b/drivers/infiniband/sw/siw/siw_cm.c
>index 5a75deb9870b..3bccfef40e7e 100644
>--- a/drivers/infiniband/sw/siw/siw_cm.c
>+++ b/drivers/infiniband/sw/siw/siw_cm.c
>@@ -1853,14 +1853,7 @@ static int siw_listen_address(struct iw_cm_id
>*id, int backlog,
> 	list_add_tail(&cep->listenq, (struct list_head
>*)id->provider_data);
> 	cep->state = SIW_EPSTATE_LISTENING;
> 
>-	if (addr_family == AF_INET)
>-		siw_dbg(id->device, "Listen at laddr %pI4 %u\n",
>-			&(((struct sockaddr_in *)laddr)->sin_addr),
>-			((struct sockaddr_in *)laddr)->sin_port);
>-	else
>-		siw_dbg(id->device, "Listen at laddr %pI6 %u\n",
>-			&(((struct sockaddr_in6 *)laddr)->sin6_addr),
>-			((struct sockaddr_in6 *)laddr)->sin6_port);
>+	siw_dbg(id->device, "Listen at laddr %pISp\n", laddr);
> 
> 	return 0;
> 
>-- 
>2.23.0.444.g18eeb5a265-goog
>
>


Thanks again Bart!
Looks much better now. You may even collapse this patch
with PATCH 03/15 you sent just before, but doesn't
really matter of course.

Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>

Re: [PATCH 00/15] RDMA patches for kernel v5.5

[Jason Gunthorpe]

On Mon, Sep 30, 2019 at 04:16:52PM -0700, Bart Van Assche wrote:
> Hi Jason,
> 
> This patch series includes my pending RDMA kernel patches. Please consider
> these for inclusion in Linux kernel v5.5.
> 
> Thanks,
> 
> Bart.
>
> Bart Van Assche (15):
>
>   RDMA/siw: Make node GUIDs valid EUI-64 identifiers
>   RDMA/srp: Remove two casts
>   RDMA/srp: Honor the max_send_sge device attribute
>   RDMA/srp: Make route resolving error messages more informative
>   RDMA/srpt: Fix handling of SR-IOV and iWARP ports
>   RDMA/srpt: Fix handling of iWARP logins
>   RDMA/srpt: Improve a debug message
>   RDMA/srpt: Rework the approach for closing an RDMA channel
>   RDMA/srpt: Rework the code that waits until an RDMA port is no longer
>     in use
>   RDMA/srpt: Make the code for handling port identities more systematic
>   RDMA/srpt: Postpone HCA removal until after configfs directory removal

These are applied to for-next, thanks

Jason