IB/srp: Fix possible use-after-free

Commit Message

Bart Van Assche Aug. 11, 2015, 3:58 p.m. UTC

On 08/11/2015 07:42 AM, Sagi Grimberg wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
> b/drivers/infiniband/ulp/srp/ib_srp.c
> index 3a1514c..b220856 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -546,6 +546,17 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>           if (ret)
>                   goto err_qp;
> 
> +       if (ch->qp)
> +               srp_destroy_qp(ch);
> +       if (ch->recv_cq)
> +               ib_destroy_cq(ch->recv_cq);
> +       if (ch->send_cq)
> +               ib_destroy_cq(ch->send_cq);
> +
> +       ch->qp = qp;
> +       ch->recv_cq = recv_cq;
> +       ch->send_cq = send_cq;
> +
>           if (dev->use_fast_reg && dev->has_fr) {
>                   fr_pool = srp_alloc_fr_pool(target);
>                   if (IS_ERR(fr_pool)) {
> @@ -570,17 +581,6 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>                   ch->fmr_pool = fmr_pool;
>           }
> 
> -       if (ch->qp)
> -               srp_destroy_qp(ch);
> -       if (ch->recv_cq)
> -               ib_destroy_cq(ch->recv_cq);
> -
> -       ch->qp = qp;
> -       ch->recv_cq = recv_cq;
> -       ch->send_cq = send_cq;
> -
>           kfree(init_attr);
>           return 0;
> 
> Sorry for the mixup. Does this patch make more sense?

On second thought ... with your patch, if the "goto err_qp" branch in
srp_create_ch_ib() is taken upon return ch->qp, ch->recv_cq and
ch->send_cq will be dangling pointers. That will have bad consequences
in the subsequent srp_free_ch_ib() call. How about replacing the above
patch with the (untested) patch below ?

Thanks,

Bart.

[PATCH] IB/srp: Avoid that a completion during reconnect causes a crash

Untested.
---
 drivers/infiniband/ulp/srp/ib_srp.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Sagi Grimberg Aug. 12, 2015, 6:31 a.m. UTC | #1

> On second thought ... with your patch, if the "goto err_qp" branch in
> srp_create_ch_ib() is taken upon return ch->qp, ch->recv_cq and
> ch->send_cq will be dangling pointers. That will have bad consequences
> in the subsequent srp_free_ch_ib() call. How about replacing the above
> patch with the (untested) patch below ?
>
> Thanks,
>
> Bart.
>
> [PATCH] IB/srp: Avoid that a completion during reconnect causes a crash
>
> Untested.
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 2968b7b..1f9ed68 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -554,9 +554,6 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>   				     "FR pool allocation failed (%d)\n", ret);
>   			goto err_qp;
>   		}
> -		if (ch->fr_pool)
> -			srp_destroy_fr_pool(ch->fr_pool);
> -		ch->fr_pool = fr_pool;
>   	} else if (!dev->use_fast_reg && dev->has_fmr) {
>   		fmr_pool = srp_alloc_fmr_pool(target);
>   		if (IS_ERR(fmr_pool)) {
> @@ -565,9 +562,6 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>   				     "FMR pool allocation failed (%d)\n", ret);
>   			goto err_qp;
>   		}
> -		if (ch->fmr_pool)
> -			ib_destroy_fmr_pool(ch->fmr_pool);
> -		ch->fmr_pool = fmr_pool;
>   	}
>
>   	if (ch->qp)
> @@ -577,6 +571,16 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
>   	if (ch->send_cq)
>   		ib_destroy_cq(ch->send_cq);
>
> +	if (dev->use_fast_reg && dev->has_fr) {
> +		if (ch->fr_pool)
> +			srp_destroy_fr_pool(ch->fr_pool);
> +		ch->fr_pool = fr_pool;
> +	} else if (!dev->use_fast_reg && dev->has_fmr) {
> +		if (ch->fmr_pool)
> +			ib_destroy_fmr_pool(ch->fmr_pool);
> +		ch->fmr_pool = fmr_pool;
> +	}
> +
>   	ch->qp = qp;
>   	ch->recv_cq = recv_cq;
>   	ch->send_cq = send_cq;
>

Looks better, I'll resubmit.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Ledford Sept. 3, 2015, 8 p.m. UTC | #2

On 08/12/2015 02:31 AM, Sagi Grimberg wrote:

> 
> Looks better, I'll resubmit.

For some reason I don't have the email for the v1 of this patchset in my
mail.  It must have gotten accidentally deleted.  In any case, v1 was
applied.

Patch

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 2968b7b..1f9ed68 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -554,9 +554,6 @@  static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 				     "FR pool allocation failed (%d)\n", ret);
 			goto err_qp;
 		}
-		if (ch->fr_pool)
-			srp_destroy_fr_pool(ch->fr_pool);
-		ch->fr_pool = fr_pool;
 	} else if (!dev->use_fast_reg && dev->has_fmr) {
 		fmr_pool = srp_alloc_fmr_pool(target);
 		if (IS_ERR(fmr_pool)) {
@@ -565,9 +562,6 @@  static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 				     "FMR pool allocation failed (%d)\n", ret);
 			goto err_qp;
 		}
-		if (ch->fmr_pool)
-			ib_destroy_fmr_pool(ch->fmr_pool);
-		ch->fmr_pool = fmr_pool;
 	}
 
 	if (ch->qp)
@@ -577,6 +571,16 @@  static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	if (ch->send_cq)
 		ib_destroy_cq(ch->send_cq);
 
+	if (dev->use_fast_reg && dev->has_fr) {
+		if (ch->fr_pool)
+			srp_destroy_fr_pool(ch->fr_pool);
+		ch->fr_pool = fr_pool;
+	} else if (!dev->use_fast_reg && dev->has_fmr) {
+		if (ch->fmr_pool)
+			ib_destroy_fmr_pool(ch->fmr_pool);
+		ch->fmr_pool = fmr_pool;
+	}
+
 	ch->qp = qp;
 	ch->recv_cq = recv_cq;
 	ch->send_cq = send_cq;