diff mbox

[1/5] ib_srp: free memory correctly in srp_free_iu()

Message ID 1346443241-24844-2-git-send-email-dongsu.park@profitbricks.com (mailing list archive)
State Awaiting Upstream
Headers show

Commit Message

Dongsu Park Aug. 31, 2012, 8 p.m. UTC 
From: Dongsu Park <dongsu.park@profitbricks.com>

As a potential fix for a race condition in srp_free_iu(),
hold a mutex in srp_free_target_ib() before calling srp_free_iu().

In addition, also clear rx/tx ring after freeing memory.
Both rx_ring[] and tx_ring[] should be reinitialized to NULL,
to prevent other tasks from accessing the freed memory.

Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Bart Van Assche Sept. 1, 2012, 7:32 a.m. UTC | #1 
On 08/31/12 20:00, dongsu.park@profitbricks.com wrote:
> From: Dongsu Park <dongsu.park@profitbricks.com>
> 
> As a potential fix for a race condition in srp_free_iu(),
> hold a mutex in srp_free_target_ib() before calling srp_free_iu().
> 
> In addition, also clear rx/tx ring after freeing memory.
> Both rx_ring[] and tx_ring[] should be reinitialized to NULL,
> to prevent other tasks from accessing the freed memory.
> 
> Signed-off-by: Dongsu Park <dongsu.park@profitbricks.com>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 7ae5a00..a0d0ca2 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -291,10 +291,16 @@ static void srp_free_target_ib(struct srp_target_port *target)
>  	ib_destroy_cq(target->send_cq);
>  	ib_destroy_cq(target->recv_cq);
>  
> -	for (i = 0; i < SRP_RQ_SIZE; ++i)
> +	mutex_lock(&target->mutex);
> +	for (i = 0; i < SRP_RQ_SIZE; ++i) {
>  		srp_free_iu(target->srp_host, target->rx_ring[i]);
> -	for (i = 0; i < SRP_SQ_SIZE; ++i)
> +		target->rx_ring[i] = NULL;
> +	}
> +	for (i = 0; i < SRP_SQ_SIZE; ++i) {
>  		srp_free_iu(target->srp_host, target->tx_ring[i]);
> +		target->tx_ring[i] = NULL;
> +	}
> +	mutex_unlock(&target->mutex);
>  }
>  
>  static void srp_path_rec_completion(int status,

It would be interesting if you could provide a little more background
information about this patch. srp_remove_target() waits until all users
of rx_ring[] and tx_ring[] have finished before invoking
srp_free_target_ib(). Or at least, that's how it should work. Did you
come up with this patch after source reading or does this patch address
an issue you hit during testing ? In the last case we have to dig deeper
and we have to find out why it's possible that rx_ring[] and/or
tx_ring[] were accessed while srp_free_target_ib() was in progress.

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

Patch

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7ae5a00..a0d0ca2 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -291,10 +291,16 @@  static void srp_free_target_ib(struct srp_target_port *target)
 	ib_destroy_cq(target->send_cq);
 	ib_destroy_cq(target->recv_cq);
 
-	for (i = 0; i < SRP_RQ_SIZE; ++i)
+	mutex_lock(&target->mutex);
+	for (i = 0; i < SRP_RQ_SIZE; ++i) {
 		srp_free_iu(target->srp_host, target->rx_ring[i]);
-	for (i = 0; i < SRP_SQ_SIZE; ++i)
+		target->rx_ring[i] = NULL;
+	}
+	for (i = 0; i < SRP_SQ_SIZE; ++i) {
 		srp_free_iu(target->srp_host, target->tx_ring[i]);
+		target->tx_ring[i] = NULL;
+	}
+	mutex_unlock(&target->mutex);
 }
 
 static void srp_path_rec_completion(int status,