IB/srp: fix race condition on srp_target_port.req_lim

PATCH In review Bart Van Assche 2 participants 3 messages Jul 25, 2010 – Jul 26, 2010 2 messages not in the archive

Subsystems (2)

InfiniBand SCSI RDMA protocol (SRP) initiator

lore

Share & tools

lore

Bart Van Assche

author

2010-07-25 16:12 UTC #0 lore raw

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

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 <[email protected]>

Cc Roland Dreier <[email protected]>

drivers/infiniband/ulp/srp/ib_srp.c +8 -6

index ed3f9eb..3d334b5 100644
@@ -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);

drivers/infiniband/ulp/srp/ib_srp.h +1 -1

index 5a80eac..048f213 100644
@@ -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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Dillow

2010-07-25 18:36 UTC #2 lore raw

Quoted 8 lines · 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.

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1280082992.10629.44.camel-1q1vX8mYZiGLUyTwlgNVppKKF0rrzTr+@public.gmane.org

Bart Van Assche

author

2010-07-26 07:55 UTC #4 lore raw

Quoted 11 lines · David Dillow

Quoted 8 lines · 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.

Quoted 4 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html