Re: [PATCH 23/29] ibmvfc: declare global function definitions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Objects allocated via mempool_alloc are incorrectly freed using kfree, breaking mempool semantics and causing deadlock under memory pressure.
- [High] Non-synchronous timer cancellation during host removal leads to Use-After-Free on `vhost` in the timer callback.
- [High] Recursive deadlock in ibmvfc_send_event() error path when sending management (MAD format) events.
--

commit 433fbb97fc83e37f1339b6c6598956ee0f1ae0a5
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: declare global function definitions

Some common functions will require visibility by both SCSI and NVMe
protocols. Make common ibmvfc helper routines available to the NVMe
support code.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 7e6912fba899d..177d341ce7bc6 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -1410,7 +1403,7 @@ static void ibmvfc_set_rport_dev_loss_tmo(struct fc_rport *rport, u32 timeout)
>   * @kref:		kref struct
>   *
>   **/
> -static void ibmvfc_release_tgt(struct kref *kref)
> +void ibmvfc_release_tgt(struct kref *kref)
>  {
>  	struct ibmvfc_target *tgt = container_of(kref, struct ibmvfc_target, kref);
>  	kfree(tgt);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this leak reserved objects
to the slab allocator and risk deadlock?

Since the target is allocated from tgt_pool (a mempool), shouldn't it be freed
with mempool_free() rather than kfree()?

Under memory pressure, if mempool allocations fail, mempool_alloc() sleeps
waiting for objects to return. Freeing directly to the slab bypasses the
mempool and its waitqueue is never woken.

[ ... ]

> @@ -1639,7 +1629,7 @@ static void ibmvfc_locked_done(struct ibmvfc_event *evt)

[Severity: High]
This isn't a bug introduced by this patch, but does this code path contain a
recursive deadlock when sending management (MAD format) events?

Management commands are often issued from a context that already holds
vhost->host->host_lock. If ibmvfc_send_event() encounters a local error, it
synchronously calls evt->done(evt):

ibmvfc_send_event() {
    ...
    if (rc) {
        ...
        evt->done(evt);
    }
}

For MAD format events, evt->done is often assigned to ibmvfc_locked_done(),
which unconditionally attempts to lock the same spinlock again:

ibmvfc_locked_done() {
    ...
    spin_lock_irqsave(evt->vhost->host->host_lock, flags);
    ...
}

Will this cause a fatal recursive deadlock?

>   * @done:	Routine to call when the event is responded to
>   * @format:	SRP or MAD format
>   **/
> -static void ibmvfc_init_event(struct ibmvfc_event *evt,
> +void ibmvfc_init_event(struct ibmvfc_event *evt,

[ ... ]

> @@ -1764,7 +1754,7 @@ static void ibmvfc_timeout(struct timer_list *t)

[Severity: High]
This isn't a bug introduced by this patch, but can ibmvfc_timeout() execute
concurrently and cause a use-after-free on vhost?

During teardown, ibmvfc_purge_requests() cancels each event's timer using the
non-synchronous timer_delete() while holding host_lock.

If the timer callback ibmvfc_timeout() is concurrently firing on another CPU,
it blocks waiting for that lock:

ibmvfc_timeout() {
    ...
    ibmvfc_reset_host(vhost);
}

ibmvfc_reset_host() then calls spin_lock_irqsave(vhost->host->host_lock, flags).

Once the teardown drops the lock and frees vhost, the timer callback acquires
the lock and operates on the freed vhost object. Could this lead to a
use-after-free?

>   *
>   * Returns the value returned from ibmvfc_send_crq(). (Zero for success)
>   **/
> -static int ibmvfc_send_event(struct ibmvfc_event *evt,
> +int ibmvfc_send_event(struct ibmvfc_event *evt,
>  			     struct ibmvfc_host *vhost, unsigned long timeout)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623013035.3436640-1-tyreld@linux.ibm.com?part=23