Re: [PATCH v2 3/3] scsi: libiscsi: validate the task named by a management response

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

Pre-existing issues:
- [High] iscsi_complete_pdu() acquires session->lock with spin_lock(), but can be called from process context, leading to a deadlock against softirqs.
--

--- Patch [3]: [PATCH v2 3/3] scsi: libiscsi: validate the task named by a management response ---
commit 5135af06e5540d3c52932cc2310608b06242c034
Author: Yehyeong Lee <[email protected]>

scsi: libiscsi: validate the task named by a management response

__iscsi_complete_pdu() fetches the task for five response types from one
place and checks only that the index resolved. This commit fixes an issue
where task->conn could be NULL by requiring the task to be in flight and
not a SCSI command.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index f6e7facf296b2..b686cd2aa4dd2 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -1180,7 +1180,7 @@ int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
>  	case ISCSI_OP_SCSI_TMFUNC_RSP:
>  	case ISCSI_OP_NOOP_IN:
>  		task = iscsi_itt_to_task(conn, hdr->itt);
> -		if (!task)
> +		if (!task || task->state == ISCSI_TASK_FREE || task->sc)

[Severity: High]
This is a pre-existing issue, but could the lock usage in the caller lead
to a deadlock?

Looking at iscsi_complete_pdu() (which calls __iscsi_complete_pdu()), it uses
spin_lock(&conn->session->lock) instead of spin_lock_bh().

If qla4xxx_task_work() in drivers/scsi/qla4xxx/ql4_os.c is called from
a workqueue (process context), it calls iscsi_complete_pdu() without disabling
softirqs:

drivers/scsi/qla4xxx/ql4_os.c:qla4xxx_task_work() {
    ...
		iscsi_complete_pdu(conn, hdr, data, data_len);
    ...
}

If a timer interrupt fires on the same CPU while the lock is held, the timer
softirq callback iscsi_check_transport_timeouts() will attempt to acquire the
same lock, resulting in a lock inversion deadlock:

drivers/scsi/libiscsi.c:iscsi_check_transport_timeouts() {
    ...
	spin_lock(&session->lock);
    ...
}

Should iscsi_complete_pdu() use spin_lock_bh() to prevent this?

>  			return ISCSI_ERR_BAD_ITT;
>  		break;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3