Re: [PATCH v3 81/88] scsi: qla2xxx: Unlink NVMe unsol ctx before freeing on LS reject error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless list manipulation of `fcport->unsol_ctx_head` leads to data races and list corruption.
- [High] `fcport` is freed without canceling or synchronizing background purex tasks, leading to Use-After-Free when those tasks access `uctx->fcport`.
--

--- Patch [81]: [PATCH v3 81/88] scsi: qla2xxx: Unlink NVMe unsol ctx before freeing on LS reject error ---
commit f4f1b05ef4c1210be894b2dc013ef3b077996e9e
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Unlink NVMe unsol ctx before freeing on LS reject error

This patch addresses a potential list corruption issue in the NVMe
unsolicited context handling. It unlinks the context from the list
before freeing it on the error path. This ensures that subsequent operations
on the fcport list do not interact with freed memory.

> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 28a04e0ff660b..36b742f73abf3 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
[ ... ]
> @@ -446,6 +446,7 @@ static int qla_nvme_xmt_ls_rsp(struct nvme_fc_local_port *lport,
>  		qla_nvme_ls_reject_iocb(vha, ha->base_qpair, &a, true);
>  		spin_unlock_irqrestore(ha->base_qpair->qp_lock_ptr, flags);
>  	}
> +	list_del(&uctx->elem);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to manipulate
the list here without any locking?

Elements are added to the list without synchronization in the hardware
response interrupt context:

drivers/scsi/qla2xxx/qla_nvme.c:qla2xxx_process_purls_iocb() {
    ...
    INIT_LIST_HEAD(&uctx->elem);
    list_add_tail(&uctx->elem, &fcport->unsol_ctx_head);
    ...
}

Items are also removed from a workqueue context without locking:

drivers/scsi/qla2xxx/qla_nvme.c:qla_nvme_release_lsrsp_cmd_kref() {
    ...
    list_del(&uctx->elem);
    ...
}

Since operations can execute concurrently across multiple CPUs asynchronously,
will this unprotected list_del() modification lead to further list
corruption and kernel oops from non-atomic pointer updates?

[Severity: High]
This is a pre-existing issue, but does this code correctly handle the
lifetime of the fcport pointer?

When an fcport logs out, it can be freed directly:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_free_fcport() {
    ...
    qla_edif_list_del(fcport);
    kfree(fcport);
    ...
}

If a background task is simultaneously processing queued items, it might
access the freed structure:

drivers/scsi/qla2xxx/qla_nvme.c:qla2xxx_process_purls_pkt() {
    ...
    ret = nvme_fc_rcv_ls_req(uctx->fcport->nvme_remote_port, &uctx->lsrsp,
                 &item->iocb, item->size);
    ...
}

Since qla2x00_free_fcport() doesn't appear to cancel pending work or clear
vha->purex_list, could this sequence cause a use-after-free when the target
drops connection after sending an unsolicited NVMe LS packet?

>  	kfree(uctx);
>  	return rval;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707055435.2680300-1-njavali@marvell.com?part=81