[PATCH v2 5/6] sg: tighten handling of struct request objects

[Douglas Gilbert <dgilbert@interlog.com>]

The struct sg_request object is related to a struct request object
in the block layer. So a struct sg_request object holds a pointer
to the other object. And since an active struct request object is
shorter lived, then that pointer is set to NULL when it becomes
inactive. Tighten the handling of that pointer with READ_ONCE()
and WRITE_ONCE(). Also put a single lock around various state
variables in the completion callback (sg_rq_end_io() ) so observers
see either pre-change or post-change and not something in between.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/sg.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c49f87c97dd4..f59ddf809f21 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -218,7 +218,7 @@ struct sg_request {	/* active SCSI command or inactive request */
 	unsigned long frq_bm[1];	/* see SG_FRQ_* defines above */
 	u8 *sense_bp;		/* mempool alloc-ed sense buffer, as needed */
 	struct sg_fd *parentfp;	/* pointer to owning fd, even when on fl */
-	struct request *rq;	/* released in sg_rq_end_io(), bio kept */
+	struct request *rqq;	/* released in sg_rq_end_io(), bio kept */
 	struct bio *bio;	/* kept until this req -->SG_RS_INACTIVE */
 	struct execute_work ew_orph;	/* harvest orphan request */
 };
@@ -1014,6 +1014,7 @@ sg_execute_cmd(struct sg_fd *sfp, struct sg_request *srp)
 {
 	bool at_head, is_v4h, sync;
 	struct sg_device *sdp = sfp->parentdp;
+	struct request *rqq;
 
 	is_v4h = test_bit(SG_FRQ_IS_V4I, srp->frq_bm);
 	sync = test_bit(SG_FRQ_SYNC_INVOC, srp->frq_bm);
@@ -1037,9 +1038,10 @@ sg_execute_cmd(struct sg_fd *sfp, struct sg_request *srp)
 		atomic_inc(&sfp->submitted);
 		set_bit(SG_FRQ_COUNT_ACTIVE, srp->frq_bm);
 	}
+	rqq = READ_ONCE(srp->rqq);
 	if (srp->rq_flags & SGV4_FLAG_HIPRI)
-		srp->rq->cmd_flags |= REQ_HIPRI;
-	blk_execute_rq_nowait(sdp->disk, srp->rq, (int)at_head, sg_rq_end_io);
+		rqq->cmd_flags |= REQ_HIPRI;
+	blk_execute_rq_nowait(sdp->disk, rqq, (int)at_head, sg_rq_end_io);
 	set_bit(SG_FRQ_ISSUED, srp->frq_bm);
 }
 
@@ -1116,7 +1118,7 @@ sg_common_write(struct sg_comm_wr_t *cwrp)
 		goto err_out;
  	}
 
-	if (unlikely(test_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm) || !srp->rq)) {
+	if (unlikely(test_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm) || !srp->rqq)) {
 		res = -EIDRM;	/* this failure unexpected but observed */
 		goto err_out;
 	}
@@ -1125,7 +1127,7 @@ sg_common_write(struct sg_comm_wr_t *cwrp)
 		res = -ENODEV;
 		goto err_out;
 	}
-	srp->rq->timeout = cwrp->timeout;
+	srp->rqq->timeout = cwrp->timeout;
 	sg_execute_cmd(fp, srp);
 	return srp;
 err_out:
@@ -2299,7 +2301,7 @@ sg_sfp_blk_poll(struct sg_fd *sfp, int loop_count)
 		return -EINVAL;
 	xa_lock_irqsave(xafp, iflags);
 	xa_for_each(xafp, idx, srp) {
-		rqq = srp->rq;
+		rqq = READ_ONCE(srp->rqq);
 		if (rqq && (srp->rq_flags & SGV4_FLAG_HIPRI) &&
 		    atomic_read(&srp->rq_st) == SG_RS_INFLIGHT &&
 		    test_bit(SG_FRQ_ISSUED, srp->frq_bm)) {
@@ -2320,8 +2322,8 @@ sg_srp_blk_poll(struct sg_request *srp, int loop_count)
 {
 	if (!test_bit(SG_FRQ_ISSUED, srp->frq_bm))
 		return 0;	/* blk_execute_rq_nowait() may not have issued request */
-	return sg_srp_q_blk_poll(srp, srp->rq, srp->parentfp->parentdp->device->request_queue,
-				 loop_count);
+	return sg_srp_q_blk_poll(srp, READ_ONCE(srp->rqq),
+				 srp->parentfp->parentdp->device->request_queue, loop_count);
 }
 
 /*
@@ -2553,6 +2555,7 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 	enum sg_rq_state rqq_state = SG_RS_AWAIT_RCV;
 	int a_resid, slen;
 	u32 rq_result;
+	unsigned long iflags;
 	struct sg_request *srp = rq->end_io_data;
 	struct scsi_request *scsi_rp = scsi_req(rq);
 	struct sg_device *sdp;
@@ -2624,7 +2627,10 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 			set_bit(SG_FRQ_DEACT_ORPHAN, srp->frq_bm);
 		}
 	}
-	set_bit(SG_FRQ_ISSUED, srp->frq_bm);
+	xa_lock_irqsave(&sfp->srp_arr, iflags);
+	__set_bit(SG_FRQ_ISSUED, srp->frq_bm);
+	sg_rq_chg_state_force_ulck(srp, rqq_state);
+	WRITE_ONCE(srp->rqq, NULL);
 	if (test_bit(SG_FRQ_COUNT_ACTIVE, srp->frq_bm)) {
 		int num = atomic_inc_return(&sfp->waiting);
 
@@ -2638,12 +2644,11 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
 				WRITE_ONCE(sfp->low_await_idx, srp->rq_idx);
 		}
 	}
-	sg_rq_chg_state_force(srp, rqq_state);
+	xa_unlock_irqrestore(&sfp->srp_arr, iflags);
 	/*
 	 * Free the mid-level resources apart from the bio (if any). The bio's
 	 * blk_rq_unmap_user() can be called later from user context.
 	 */
-	srp->rq = NULL;
 	scsi_req_free_cmd(scsi_rp);
 	blk_put_request(rq);
 
@@ -3092,7 +3097,7 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 	}
 	/* current sg_request protected by SG_RS_BUSY state */
 	scsi_rp = scsi_req(rq);
-	srp->rq = rq;
+	WRITE_ONCE(srp->rqq, rq);
 	if (rq_flags & SGV4_FLAG_HIPRI)
 		set_bit(SG_FFD_HIPRI_SEEN, sfp->ffd_bm);
 
@@ -3178,7 +3183,7 @@ sg_start_req(struct sg_request *srp, struct sg_comm_wr_t *cwrp, int dxfer_dir)
 	if (unlikely(res)) {		/* failure, free up resources */
 		if (likely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ,
 					     srp->frq_bm))) {
-			srp->rq = NULL;
+			WRITE_ONCE(srp->rqq, NULL);
 			blk_put_request(rq);
 		}
 	} else {
@@ -3214,9 +3219,9 @@ sg_finish_scsi_blk_rq(struct sg_request *srp)
 
 	/* Expect blk_put_request(rq) already called in sg_rq_end_io() */
 	if (unlikely(!test_and_set_bit(SG_FRQ_BLK_PUT_REQ, srp->frq_bm))) {
-		struct request *rq = srp->rq;
+		struct request *rq = READ_ONCE(srp->rqq);
 
-		srp->rq = NULL;
+		WRITE_ONCE(srp->rqq, NULL);
 		if (rq) {       /* blk_get_request() may have failed */
 			if (scsi_req(rq))
 				scsi_req_free_cmd(scsi_req(rq));
-- 
2.25.1