Re: [PATCH v3 51/57] scsi: iSCSI transport: Prepare for enabling lock context analysis

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dropping a lock without holding a reference allows concurrent teardown to free the connection object, leading to a Use-After-Free.
- [High] Pre-existing memory leak of `iscsi_endpoint` if userspace sends `ISCSI_UEVENT_STOP_CONN` before `ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT` during connection recovery.
--

--- Patch [51]: [PATCH v3 51/57] scsi: iSCSI transport: Prepare for enabling lock context analysis ---
commit f51c8db89420b5a402dacd38c1b68b13f9794c32
Author: Bart Van Assche <bvanassche@acm.org>

scsi: iSCSI transport: Prepare for enabling lock context analysis

Document locking requirements with __must_hold().

> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 8aa76f813bcdb..ade69c7b65b47 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -2243,6 +2243,7 @@ static void iscsi_ep_disconnect(struct iscsi_cls_conn *conn, bool is_active)
>  static void iscsi_if_disconnect_bound_ep(struct iscsi_cls_conn *conn,
>  					 struct iscsi_endpoint *ep,
>  					 bool is_active)
> +	__must_hold(&conn->ep_mutex)

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a memory leak
of the iscsi_endpoint?

If userspace sends ISCSI_UEVENT_STOP_CONN during connection recovery,
iscsi_if_stop_conn() unconditionally clears ISCSI_CLS_CONN_BIT_CLEANUP:

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_stop_conn()
    ...
    clear_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags);
    ...

Then, when ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT is handled here in
iscsi_if_disconnect_bound_ep(), the bit check fails:

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_disconnect_bound_ep()
    ...
    if (!test_bit(ISCSI_CLS_CONN_BIT_CLEANUP, &conn->flags)) {
        spin_unlock_irq(&conn->lock);
        iscsi_ep_disconnect(conn, is_active);
    } else {
    ...

This bypasses the iscsi_put_endpoint() call in the else block, which appears
to permanently leak the endpoint if the cleanup work previously took a
reference.


[Severity: Critical]
This is a pre-existing issue, but does dropping this mutex allow a
use-after-free of the connection object?

When the else block is taken in this function:

drivers/scsi/scsi_transport_iscsi.c:iscsi_if_disconnect_bound_ep()
    ...
    } else {
        spin_unlock_irq(&conn->lock);
        ISCSI_DBG_TRANS_CONN(conn, "flush kernel conn cleanup.\n");
        mutex_unlock(&conn->ep_mutex);

        flush_work(&conn->cleanup_work);
        /* ... */
        iscsi_put_endpoint(ep);
        mutex_lock(&conn->ep_mutex);
    }

iscsi_if_ep_disconnect() does not take a reference on conn (which is not
refcounted by the endpoint). If a concurrent hardware teardown destroys the
connection and drops its last reference during flush_work(), wouldn't
mutex_lock(&conn->ep_mutex) re-acquire the lock on freed memory?

>  {
>  	/* Check if this was a conn error and the kernel took ownership */
>  	spin_lock_irq(&conn->lock);

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