Re: [PATCH v3 46/57] scsi: qedi: Enable lock context analysis

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Deadlock vulnerability due to acquiring `session->back_lock` without disabling softirqs in CQE processing paths.
--

--- Patch [46]: [PATCH v3 46/57] scsi: qedi: Enable lock context analysis ---
commit c592a3970ebc316b3aad74beecefeffd2a24b31b
Author: Bart Van Assche <bvanassche@acm.org>

scsi: qedi: Enable lock context analysis

Annotate qedi_cleanup_all_io() with __no_context_analysis since it
performs conditional locking.

[ ... ]

> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
> index 854efa4f61d87..e162061ca49c9 100644
> --- a/drivers/scsi/qedi/qedi_fw.c
> +++ b/drivers/scsi/qedi/qedi_fw.c
> @@ -1134,6 +1134,7 @@ int qedi_send_iscsi_logout(struct qedi_conn *qedi_conn,
>  
>  int qedi_cleanup_all_io(struct qedi_ctx *qedi, struct qedi_conn *qedi_conn,
>  			struct iscsi_task *task, bool in_recovery)
> +	__context_unsafe(conditional locking)
>  {
>  	int rval;
>  	struct iscsi_task *ctask;

[Severity: High]
This is a pre-existing issue, but could the locking elsewhere in this file
lead to a deadlock?

Looking at qedi_process_tmf_resp() (and similar response handlers like
qedi_process_text_resp(), qedi_process_logout_resp(), and
qedi_process_login_resp()), they acquire session->back_lock without
disabling softirqs:

qedi_process_tmf_resp()
    spin_lock(&session->back_lock);

These handlers execute asynchronously in a kernel thread
(qedi_percpu_io_thread) with softirqs enabled.

If a timer interrupt fires on the same CPU while this lock is held, the
libiscsi transport timer (iscsi_check_transport_timeouts) could preempt
the thread in TIMER_SOFTIRQ context.

If that timer logic attempts to send a ping (iscsi_send_nopout()) and
fails due to an allocation failure or full queue, its cleanup path calls
iscsi_put_task().

iscsi_put_task() unconditionally attempts to acquire the lock using
spin_lock_bh(&session->back_lock).

Would this cause a softirq-preemption deadlock where the timer spins forever
waiting for the preempted thread to release session->back_lock? Should these
CQE processing paths use spin_lock_bh() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785451174.git.bvanassche@acm.org?part=46