[PATCH] IB/srp: fix race condition on srp_target_port.req_lim

[PATCH] IB/srp: fix race condition on srp_target_port.req_lim

[Bart Van Assche]

In the current implementation of ib_srp the req_lim field of
struct srp_target_port can be manipulated in a non-atomic way by
more than one CPU at a time: one CPU can be modifying req_lim in
function srp_process_rsp() while another CPU can concurrently be
decrementing req_lim in function __srp_get_tx_iu(). This is a
race condition which can result in incorrect manipulation of the
req_lim field. The patch below fixes this race condition by
converting all manipulations of req_lim into atomic operations.

Signed-off-by: Bart Van Assche <bart.vanassche-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index ed3f9eb..3d334b5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -822,7 +822,7 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 
 	spin_lock_irqsave(target->scsi_host->host_lock, flags);
 
-	target->req_lim += delta;
+	atomic_add(delta, &target->req_lim);
 
 	req = &target->req_ring[rsp->tag & ~SRP_TAG_TSK_MGMT];
 
@@ -1008,7 +1008,7 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
 		return NULL;
 
-	if (target->req_lim < min) {
+	if (atomic_read(&target->req_lim) < min) {
 		++target->zero_req_lim;
 		return NULL;
 	}
@@ -1042,7 +1042,7 @@ static int __srp_post_send(struct srp_target_port *target,
 
 	if (!ret) {
 		++target->tx_head;
-		--target->req_lim;
+		atomic_dec(&target->req_lim);
 	}
 
 	return ret;
@@ -1266,10 +1266,12 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 			struct srp_login_rsp *rsp = event->private_data;
 
 			target->max_ti_iu_len = be32_to_cpu(rsp->max_ti_iu_len);
-			target->req_lim       = be32_to_cpu(rsp->req_lim_delta);
+			atomic_set(&target->req_lim,
+				   be32_to_cpu(rsp->req_lim_delta));
 
-			target->scsi_host->can_queue = min(target->req_lim,
-							   target->scsi_host->can_queue);
+			target->scsi_host->can_queue
+				= min(atomic_read(&target->req_lim),
+				      target->scsi_host->can_queue);
 		} else {
 			shost_printk(KERN_WARNING, target->scsi_host,
 				    PFX "Unhandled RSP opcode %#x\n", opcode);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 5a80eac..048f213 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -135,7 +135,7 @@ struct srp_target_port {
 	struct ib_qp	       *qp;
 
 	int			max_ti_iu_len;
-	s32			req_lim;
+	atomic_t		req_lim;
 
 	int			zero_req_lim;
 
--
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] IB/srp: fix race condition on srp_target_port.req_lim

[David Dillow]

On Sun, 2010-07-25 at 18:12 +0200, Bart Van Assche wrote:
> In the current implementation of ib_srp the req_lim field of
> struct srp_target_port can be manipulated in a non-atomic way by
> more than one CPU at a time: one CPU can be modifying req_lim in
> function srp_process_rsp() while another CPU can concurrently be
> decrementing req_lim in function __srp_get_tx_iu(). This is a
> race condition which can result in incorrect manipulation of the
> req_lim field. The patch below fixes this race condition by
> converting all manipulations of req_lim into atomic operations.

This is not needed -- all modifications of target->req_lim are protected
by scsi_dev->host_lock. It is held implicitly in srp_queuecommand() by
the SCSI mid-layer, and we take that lock before modifying req_lim
elsewhere -- or we're initializing and there won't be concurrent access.

--
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] IB/srp: fix race condition on srp_target_port.req_lim

[Bart Van Assche]

On Sun, Jul 25, 2010 at 8:36 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>
> On Sun, 2010-07-25 at 18:12 +0200, Bart Van Assche wrote:
> > In the current implementation of ib_srp the req_lim field of
> > struct srp_target_port can be manipulated in a non-atomic way by
> > more than one CPU at a time: one CPU can be modifying req_lim in
> > function srp_process_rsp() while another CPU can concurrently be
> > decrementing req_lim in function __srp_get_tx_iu(). This is a
> > race condition which can result in incorrect manipulation of the
> > req_lim field. The patch below fixes this race condition by
> > converting all manipulations of req_lim into atomic operations.
>
> This is not needed -- all modifications of target->req_lim are protected
> by scsi_dev->host_lock. It is held implicitly in srp_queuecommand() by
> the SCSI mid-layer, and we take that lock before modifying req_lim
> elsewhere -- or we're initializing and there won't be concurrent access.

It would be helpful for anyone who is reviewing or modifying the SRP
initiator source code if the locking policy was documented. As an
example, in kernels 2.6.33 and before there was a single notification
processing function srp_completion() that incremented target->tx_tail
non-atomically and without protection of scsi_host->host_lock. It was
not possible to tell from the source code comments whether or not this
access pattern was intentional.

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