Commit 31e6cdbe authored by Saurav Kashyap's avatar Saurav Kashyap Committed by Martin K. Petersen
Browse files

scsi: qla2xxx: Implement ref count for SRB 

The timeout handler and the done function are racing. When
qla2x00_async_iocb_timeout() starts to run it can be preempted by the
normal response path (via the firmware?). qla24xx_async_gpsc_sp_done()
releases the SRB unconditionally. When scheduling back to
qla2x00_async_iocb_timeout() qla24xx_async_abort_cmd() will access an freed
sp->qpair pointer:

  qla2xxx [0000:83:00.0]-2871:0: Async-gpsc timeout - hdl=63d portid=234500 50:06:0e:80:08:77:b6:21.
  qla2xxx [0000:83:00.0]-2853:0: Async done-gpsc res 0, WWPN 50:06:0e:80:08:77:b6:21
  qla2xxx [0000:83:00.0]-2854:0: Async-gpsc OUT WWPN 20:45:00:27:f8:75:33:00 speeds=2c00 speed=0400.
  qla2xxx [0000:83:00.0]-28d8:0: qla24xx_handle_gpsc_event 50:06:0e:80:08:77:b6:21 DS 7 LS 6 rc 0 login 1|1 rscn 1|0 lid 5
  BUG: unable to handle kernel NULL pointer dereference at 0000000000000004
  IP: qla24xx_async_abort_cmd+0x1b/0x1c0 [qla2xxx]

Obvious solution to this is to introduce a reference counter. One reference
is taken for the normal code path (the 'good' case) and one for the timeout
path. As we always race between the normal good case and the timeout/abort
handler we need to serialize it. Also we cannot assume any order between
the handlers. Since this is slow path we can use proper synchronization via
locks.

When we are able to cancel a timer (del_timer returns 1) we know there
can't be any error handling in progress because the timeout handler hasn't
expired yet, thus we can safely decrement the refcounter by one.

If we are not able to cancel the timer, we know an abort handler is
running. We have to make sure we call sp->done() in the abort handlers
before calling kref_put().

Link: https://lore.kernel.org/r/20220110050218.3958-3-njavali@marvell.com


Cc: stable@vger.kernel.org
Reviewed-by: default avatarHimanshu Madhani <himanshu.madhani@oracle.com>
Co-developed-by: default avatarDaniel Wagner <dwagner@suse.de>
Signed-off-by: default avatarDaniel Wagner <dwagner@suse.de>
Signed-off-by: default avatarSaurav Kashyap <skashyap@marvell.com>
Signed-off-by: default avatarNilesh Javali <njavali@marvell.com>
Signed-off-by: default avatarMartin K. Petersen <martin.petersen@oracle.com>
parent d4523bd6

drivers/scsi/qla2xxx/qla_bsg.c

+4 −2
Original line number Diff line number Diff line
@@ -29,7 +29,8 @@ void qla2x00_bsg_job_done(srb_t *sp, int res) 
	    "%s: sp hdl %x, result=%x bsg ptr %p\n",

	    __func__, sp->handle, res, bsg_job);



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);



	bsg_reply->result = res;

	bsg_job_done(bsg_job, bsg_reply->result,
@@ -3013,7 +3014,8 @@ qla24xx_bsg_timeout(struct bsg_job *bsg_job) 


done:

	spin_unlock_irqrestore(&ha->hardware_lock, flags);

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

	return 0;

}

drivers/scsi/qla2xxx/qla_def.h

+5 −0
Original line number Diff line number Diff line
@@ -726,6 +726,11 @@ typedef struct srb { 
	 * code.

	 */

	void (*put_fn)(struct kref *kref);



	/*

	 * Report completion for asynchronous commands.

	 */

	void (*async_done)(struct srb *sp, int res);

} srb_t;



#define GET_CMD_SP(sp) (sp->u.scmd.cmd)

drivers/scsi/qla2xxx/qla_edif.c

+2 −1
Original line number Diff line number Diff line
@@ -2146,7 +2146,8 @@ edif_doorbell_show(struct device *dev, struct device_attribute *attr, 


static void qla_noop_sp_done(srb_t *sp, int res)

{

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

}



/*

drivers/scsi/qla2xxx/qla_gbl.h

+1 −0
Original line number Diff line number Diff line
@@ -333,6 +333,7 @@ extern int qla24xx_get_one_block_sg(uint32_t, struct qla2_sgx *, uint32_t *); 
extern int qla24xx_configure_prot_mode(srb_t *, uint16_t *);

extern int qla24xx_issue_sa_replace_iocb(scsi_qla_host_t *vha,

	struct qla_work_evt *e);

void qla2x00_sp_release(struct kref *kref);



/*

 * Global Function Prototypes in qla_mbx.c source file.

drivers/scsi/qla2xxx/qla_gs.c

+57 −28
Original line number Diff line number Diff line
@@ -529,7 +529,6 @@ static void qla2x00_async_sns_sp_done(srb_t *sp, int rc) 
		if (!e)

			goto err2;



		del_timer(&sp->u.iocb_cmd.timer);

		e->u.iosb.sp = sp;

		qla2x00_post_work(vha, e);

		return;
@@ -556,8 +555,8 @@ static void qla2x00_async_sns_sp_done(srb_t *sp, int rc) 
			sp->u.iocb_cmd.u.ctarg.rsp = NULL;

		}



		sp->free(sp);



		/* ref: INIT */

		kref_put(&sp->cmd_kref, qla2x00_sp_release);

		return;

	}
@@ -592,6 +591,7 @@ static int qla_async_rftid(scsi_qla_host_t *vha, port_id_t *d_id) 
	if (!vha->flags.online)

		goto done;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL);

	if (!sp)

		goto done;
@@ -652,7 +652,8 @@ static int qla_async_rftid(scsi_qla_host_t *vha, port_id_t *d_id) 
	}

	return rval;

done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}
@@ -687,6 +688,7 @@ static int qla_async_rffid(scsi_qla_host_t *vha, port_id_t *d_id, 
	srb_t *sp;

	struct ct_sns_pkt *ct_sns;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL);

	if (!sp)

		goto done;
@@ -747,7 +749,8 @@ static int qla_async_rffid(scsi_qla_host_t *vha, port_id_t *d_id, 
	return rval;



done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}
@@ -777,6 +780,7 @@ static int qla_async_rnnid(scsi_qla_host_t *vha, port_id_t *d_id, 
	srb_t *sp;

	struct ct_sns_pkt *ct_sns;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL);

	if (!sp)

		goto done;
@@ -836,7 +840,8 @@ static int qla_async_rnnid(scsi_qla_host_t *vha, port_id_t *d_id, 
	return rval;



done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}
@@ -882,6 +887,7 @@ static int qla_async_rsnn_nn(scsi_qla_host_t *vha) 
	srb_t *sp;

	struct ct_sns_pkt *ct_sns;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL);

	if (!sp)

		goto done;
@@ -947,7 +953,8 @@ static int qla_async_rsnn_nn(scsi_qla_host_t *vha) 
	return rval;



done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}
@@ -2887,7 +2894,8 @@ static void qla24xx_async_gpsc_sp_done(srb_t *sp, int res) 
	qla24xx_handle_gpsc_event(vha, &ea);



done:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

}



int qla24xx_async_gpsc(scsi_qla_host_t *vha, fc_port_t *fcport)
@@ -2899,6 +2907,7 @@ int qla24xx_async_gpsc(scsi_qla_host_t *vha, fc_port_t *fcport) 
	if (!vha->flags.online || (fcport->flags & FCF_ASYNC_SENT))

		return rval;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, fcport, GFP_KERNEL);

	if (!sp)

		goto done;
@@ -2938,7 +2947,8 @@ int qla24xx_async_gpsc(scsi_qla_host_t *vha, fc_port_t *fcport) 
	return rval;



done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}
@@ -2987,7 +2997,8 @@ void qla24xx_sp_unmap(scsi_qla_host_t *vha, srb_t *sp) 
		break;

	}



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

}



void qla24xx_handle_gpnid_event(scsi_qla_host_t *vha, struct event_arg *ea)
@@ -3126,13 +3137,15 @@ static void qla2x00_async_gpnid_sp_done(srb_t *sp, int res) 
	if (res) {

		if (res == QLA_FUNCTION_TIMEOUT) {

			qla24xx_post_gpnid_work(sp->vha, &ea.id);

			sp->free(sp);

			/* ref: INIT */

			kref_put(&sp->cmd_kref, qla2x00_sp_release);

			return;

		}

	} else if (sp->gen1) {

		/* There was another RSCN for this Nport ID */

		qla24xx_post_gpnid_work(sp->vha, &ea.id);

		sp->free(sp);

		/* ref: INIT */

		kref_put(&sp->cmd_kref, qla2x00_sp_release);

		return;

	}
@@ -3153,7 +3166,8 @@ static void qla2x00_async_gpnid_sp_done(srb_t *sp, int res) 
				  sp->u.iocb_cmd.u.ctarg.rsp_dma);

		sp->u.iocb_cmd.u.ctarg.rsp = NULL;



		sp->free(sp);

		/* ref: INIT */

		kref_put(&sp->cmd_kref, qla2x00_sp_release);

		return;

	}
@@ -3173,6 +3187,7 @@ int qla24xx_async_gpnid(scsi_qla_host_t *vha, port_id_t *id) 
	if (!vha->flags.online)

		goto done;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL);

	if (!sp)

		goto done;
@@ -3189,7 +3204,8 @@ int qla24xx_async_gpnid(scsi_qla_host_t *vha, port_id_t *id) 
		if (tsp->u.iocb_cmd.u.ctarg.id.b24 == id->b24) {

			tsp->gen1++;

			spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);

			sp->free(sp);

			/* ref: INIT */

			kref_put(&sp->cmd_kref, qla2x00_sp_release);

			goto done;

		}

	}
@@ -3259,8 +3275,8 @@ int qla24xx_async_gpnid(scsi_qla_host_t *vha, port_id_t *id) 
			sp->u.iocb_cmd.u.ctarg.rsp_dma);

		sp->u.iocb_cmd.u.ctarg.rsp = NULL;

	}



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}
@@ -3315,7 +3331,8 @@ void qla24xx_async_gffid_sp_done(srb_t *sp, int res) 
	ea.rc = res;



	qla24xx_handle_gffid_event(vha, &ea);

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

}



/* Get FC4 Feature with Nport ID. */
@@ -3328,6 +3345,7 @@ int qla24xx_async_gffid(scsi_qla_host_t *vha, fc_port_t *fcport) 
	if (!vha->flags.online || (fcport->flags & FCF_ASYNC_SENT))

		return rval;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, fcport, GFP_KERNEL);

	if (!sp)

		return rval;
@@ -3366,7 +3384,8 @@ int qla24xx_async_gffid(scsi_qla_host_t *vha, fc_port_t *fcport) 


	return rval;

done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

	fcport->flags &= ~FCF_ASYNC_SENT;

	return rval;

}
@@ -3753,7 +3772,6 @@ static void qla2x00_async_gpnft_gnnft_sp_done(srb_t *sp, int res) 
	    "Async done-%s res %x FC4Type %x\n",

	    sp->name, res, sp->gen2);



	del_timer(&sp->u.iocb_cmd.timer);

	sp->rc = res;

	if (res) {

		unsigned long flags;
@@ -3921,8 +3939,8 @@ static int qla24xx_async_gnnft(scsi_qla_host_t *vha, struct srb *sp, 
		    sp->u.iocb_cmd.u.ctarg.rsp_dma);

		sp->u.iocb_cmd.u.ctarg.rsp = NULL;

	}



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);



	spin_lock_irqsave(&vha->work_lock, flags);

	vha->scan.scan_flags &= ~SF_SCANNING;
@@ -3974,9 +3992,12 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp) 
		ql_dbg(ql_dbg_disc + ql_dbg_verbose, vha, 0xffff,

		    "%s: Performing FCP Scan\n", __func__);



		if (sp)

			sp->free(sp); /* should not happen */

		if (sp) {

			/* ref: INIT */

			kref_put(&sp->cmd_kref, qla2x00_sp_release);

		}



		/* ref: INIT */

		sp = qla2x00_get_sp(vha, NULL, GFP_KERNEL);

		if (!sp) {

			spin_lock_irqsave(&vha->work_lock, flags);
@@ -4021,6 +4042,7 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp) 
			    sp->u.iocb_cmd.u.ctarg.req,

			    sp->u.iocb_cmd.u.ctarg.req_dma);

			sp->u.iocb_cmd.u.ctarg.req = NULL;

			/* ref: INIT */

			qla2x00_rel_sp(sp);

			return rval;

		}
@@ -4083,7 +4105,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type, srb_t *sp) 
		sp->u.iocb_cmd.u.ctarg.rsp = NULL;

	}



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);



	spin_lock_irqsave(&vha->work_lock, flags);

	vha->scan.scan_flags &= ~SF_SCANNING;
@@ -4147,7 +4170,8 @@ static void qla2x00_async_gnnid_sp_done(srb_t *sp, int res) 


	qla24xx_handle_gnnid_event(vha, &ea);



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

}



int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport)
@@ -4160,6 +4184,7 @@ int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport) 
		return rval;



	qla2x00_set_fcport_disc_state(fcport, DSC_GNN_ID);

	/* ref: INIT */

	sp = qla2x00_get_sp(vha, fcport, GFP_ATOMIC);

	if (!sp)

		goto done;
@@ -4200,7 +4225,8 @@ int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport) 
	return rval;



done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

	fcport->flags &= ~FCF_ASYNC_SENT;

done:

	return rval;
@@ -4274,7 +4300,8 @@ static void qla2x00_async_gfpnid_sp_done(srb_t *sp, int res) 


	qla24xx_handle_gfpnid_event(vha, &ea);



	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

}



int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport)
@@ -4286,6 +4313,7 @@ int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport) 
	if (!vha->flags.online || (fcport->flags & FCF_ASYNC_SENT))

		return rval;



	/* ref: INIT */

	sp = qla2x00_get_sp(vha, fcport, GFP_ATOMIC);

	if (!sp)

		goto done;
@@ -4326,7 +4354,8 @@ int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport) 
	return rval;



done_free_sp:

	sp->free(sp);

	/* ref: INIT */

	kref_put(&sp->cmd_kref, qla2x00_sp_release);

done:

	return rval;

}