| Message ID | 20240428113404.12522-1-hyperlyzcs@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] scsi: qla2xxx: Fix double free of fcport in error handling path | expand |
… > Fix this by cleaning up the redundant qla2x00_free_fcport() and > replacing error handling with a goto chain. … Can the following wording approach be a bit nicer? Thus clean duplicate qla2x00_free_fcport() calls up and use more common error handling code instead. > --- > drivers/scsi/qla2xxx/qla_iocb.c | 13 +++++-------- … Unfortunately, you overlooked to add a patch version description behind the marker line. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n713 … > +++ b/drivers/scsi/qla2xxx/qla_iocb.c … > @@ -2787,6 +2783,7 @@ qla24xx_els_dcmd_iocb(scsi_qla_host_t *vha, int els_opcode, > > wait_for_completion(&elsio->u.els_logo.comp); > > +free_sp: * I suggest to omit a blank line here. * How do you think about to use the label “put_ref”? > /* ref: INIT */ > kref_put(&sp->cmd_kref, qla2x00_sp_release); > return rval; Regards, Markus
…> Fix this by cleaning up the redundant qla2x00_free_fcport() and
> replacing error handling with a goto chain.
I imagine that there can be a need to provide the desired software adjustment
as a patch series with two separate update steps.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n81
* Deletion of inappropriate function calls
* Optimisation of exception handling
How do you think about to refer to the affected function
(instead of the hint “error handling path”) in the summary phrase?
Regards,
Markus
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 0b41e8a06602..7b6a1db55672 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -2749,10 +2749,8 @@ qla24xx_els_dcmd_iocb(scsi_qla_host_t *vha, int els_opcode, GFP_KERNEL); if (!elsio->u.els_logo.els_logo_pyld) { - /* ref: INIT */ - kref_put(&sp->cmd_kref, qla2x00_sp_release); - qla2x00_free_fcport(fcport); - return QLA_FUNCTION_FAILED; + rval = QLA_FUNCTION_FAILED; + goto free_sp; } memset(&logo_pyld, 0, sizeof(struct els_logo_payload)); @@ -2774,10 +2772,8 @@ qla24xx_els_dcmd_iocb(scsi_qla_host_t *vha, int els_opcode, rval = qla2x00_start_sp(sp); if (rval != QLA_SUCCESS) { - /* ref: INIT */ - kref_put(&sp->cmd_kref, qla2x00_sp_release); - qla2x00_free_fcport(fcport); - return QLA_FUNCTION_FAILED; + rval = QLA_FUNCTION_FAILED; + goto free_sp; } ql_dbg(ql_dbg_io, vha, 0x3074, @@ -2787,6 +2783,7 @@ qla24xx_els_dcmd_iocb(scsi_qla_host_t *vha, int els_opcode, wait_for_completion(&elsio->u.els_logo.comp); +free_sp: /* ref: INIT */ kref_put(&sp->cmd_kref, qla2x00_sp_release); return rval;
When dma_alloc_coherent() or qla2x00_start_sp() return an error, the callback function qla2x00_els_dcmd_sp_free in qla2x00_sp_release will call qla2x00_free_fcport() to kfree fcport. We shouldn't call qla2x00_free_fcport() again in the error handling path. Fix this by cleaning up the redundant qla2x00_free_fcport() and replacing error handling with a goto chain. Fixes: 82f522ae0d97 ("scsi: qla2xxx: Fix double free of fcport") Signed-off-by: Yongzhi Liu <hyperlyzcs@gmail.com> --- drivers/scsi/qla2xxx/qla_iocb.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)