Re: [PATCH v3 35/57] scsi: libiscsi: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing __must_hold() annotations for core locking functions.
- [Low] Missing __must_hold annotation on __iscsi_complete_pdu() definition.

Pre-existing issues:
- [High] Deadlock due to `iscsi_complete_pdu()` acquiring `session->frwd_lock` without disabling softirqs.
- [High] Hard deadlock due to `qla4xxx` acquiring `session->back_lock` in a hardware interrupt context while `libiscsi` core leaves hardirqs enabled.
--

--- Patch [35]: [PATCH v3 35/57] scsi: libiscsi: Prepare for enabling lock context analysis ---
commit 052b7ed83fc5a572597a78bb52886b250ac80aa2
Author: Bart Van Assche <bvanassche@acm.org>

scsi: libiscsi: Prepare for enabling lock context analysis
    
This commit prepares libiscsi for lock context analysis by documenting
locking requirements using __must_hold().

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
[ ... ]
> @@ -1962,6 +1965,7 @@ static int iscsi_exec_task_mgmt_fn(struct iscsi_conn *conn,
>   * Fail commands. session frwd lock held and xmit thread flushed.
>   */
>  static void fail_scsi_tasks(struct iscsi_conn *conn, u64 lun, int error)
> +	__must_hold(&conn->session->frwd_lock)
>  {

[Severity: Low]
Should we also add these annotations to the core locking functions like
fail_scsi_task(), __fail_scsi_task(), and iscsi_send_nopout()?

Functions like __fail_scsi_task() manipulate sequence numbers and queues
that explicitly require both back_lock and frwd_lock to be held. Similarly,
iscsi_send_nopout() requires frwd_lock. Leaving these unannotated leaves
portions of the subsystem's logic unanalyzed by the context checker.

[ ... ]
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -474,8 +474,9 @@ extern int iscsi_conn_send_pdu(struct iscsi_cls_conn *, struct iscsi_hdr *,
>  				char *, uint32_t);
>  extern int iscsi_complete_pdu(struct iscsi_conn *, struct iscsi_hdr *,
>  			      char *, int);
> -extern int __iscsi_complete_pdu(struct iscsi_conn *, struct iscsi_hdr *,
> -				char *, int);
> +extern int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *,
> +				char *, int)
> +		__must_hold(&conn->session->back_lock);

[Severity: Low]
Does the function definition in drivers/scsi/libiscsi.c also need this
annotation?

The __must_hold() attribute was added to the declaration here in the header,
but it seems omitted from the definition:

int __iscsi_complete_pdu(struct iscsi_conn *conn, struct iscsi_hdr *hdr,
                         char *data, int datalen)
{
    ...

While some static analyzers inherit attributes from the header, ensuring
locking annotations on declarations mirror their definitions provides
local consistency across translation units.

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential softirq
deadlock when acquiring frwd_lock during pdu completion?

The generic API iscsi_complete_pdu() uses spin_lock() to acquire the
back_lock, which leaves softirqs enabled. Inside __iscsi_complete_pdu(),
processing an ISCSI_OP_NOOP_IN temporarily drops back_lock and acquires
frwd_lock using spin_lock():

__iscsi_complete_pdu()
    spin_unlock(&session->back_lock);
    spin_lock(&session->frwd_lock);
    iscsi_send_nopout(conn, (struct iscsi_nopin*)hdr);

When called from a process context, such as a workqueue handler, softirqs
remain enabled:

qla4xxx_task_work()
  -> iscsi_complete_pdu()
     spin_lock(&conn->session->back_lock);

If a timer softirq interrupts this process and attempts to acquire frwd_lock,
it could spin forever waiting for the interrupted process to release it.

[Severity: High]
This is a pre-existing issue, but could this back_lock usage lead to a
hardirq deadlock with the qla4xxx driver?

The libiscsi core routinely acquires back_lock using spin_lock_bh() or
spin_lock() in functions like fail_scsi_tasks(), leaving hardware interrupts
enabled:

fail_scsi_tasks()
    spin_lock_bh(&session->back_lock);

However, the qla4xxx driver acquires back_lock from a hardware interrupt
context:

qla4_83xx_interrupt_service_routine()
  -> qla4xxx_process_response_queue()
    -> qla4xxx_passthru_status_entry()
       spin_lock(&conn->session->back_lock);

If a hardware interrupt fires on a CPU currently executing a libiscsi core
function that is holding back_lock, the interrupt handler will attempt to
acquire the lock and cause a hard deadlock.

>  extern int iscsi_verify_itt(struct iscsi_conn *, itt_t);
>  extern struct iscsi_task *iscsi_itt_to_ctask(struct iscsi_conn *, itt_t);
>  extern struct iscsi_task *iscsi_itt_to_task(struct iscsi_conn *, itt_t);

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