Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Premature loop termination in `vfio_ccw_sch_quiesce` due to re-used completion without re-initialization.
- [High] Unconditional NULL pointer dereference in asynchronous subchannel callbacks.
- [High] Leaked workqueue items (`io_work` and `crw_work`) during device teardown leading to Use-After-Free.
- [High] Lockless state access in `vfio_ccw_sch_io_todo` causes a double-free of channel program (`cp_free`).
--

--- Patch [1]: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce() ---
commit c7b4889a75b6cf11527a57bff8438c3c08dac9c4
Author: Heiko Carstens <hca@linux.ibm.com>
Subject: s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()

This patch adds the __must_hold attribute to vfio_ccw_sch_quiesce() to
inform context analysis that sch->lock must be held on function entry.
This resolves a thread-safety-analysis warning during compilation.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc728..ed9ca77c5b23f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -35,6 +35,7 @@ debug_info_t *vfio_ccw_debug_trace_id;
>   * Helpers
>   */
>  int vfio_ccw_sch_quiesce(struct subchannel *sch)
> +	__must_hold(&sch->lock);
>  {
>  	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
>  	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);

[Severity: High]
This is a pre-existing issue, but does the completion handling in this loop
allow premature termination?

If wait_for_completion_timeout() times out, but a late interrupt arrives
and calls complete(), the completion's done counter becomes greater than 0.

On the next iteration of the do/while (ret == -EBUSY) loop in 
vfio_ccw_sch_quiesce():

do {
        ret = cio_cancel_halt_clear(sch, &iretry);
        ...
        if (ret == -EBUSY)
                wait_for_completion_timeout(&completion, 3*HZ);
        ...
} while (ret == -EBUSY);

Could wait_for_completion_timeout() see done > 0 and return immediately
without waiting, causing a tight spin loop calling cio_disable_subchannel()
since there is no reinit_completion() inside the loop?

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of a NULL
pointer dereference when fetching the parent pointer?

During device removal, vfio_ccw_sch_remove() sets the drvdata to NULL:
dev_set_drvdata(&sch->dev, NULL);

If an asynchronous event like an interrupt triggers concurrently before the
CSS core fully detaches the driver, could vfio_ccw_sch_irq() or
vfio_ccw_sch_event() fetch a NULL parent and immediately dereference it
in this sequence?

struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);

[ ... ]
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 0501d4bbcdbd6..8db29519dbfde 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -127,7 +127,8 @@ struct vfio_ccw_private {
>  	struct work_struct	crw_work;
>  } __aligned(8);

[Severity: High]
This is a pre-existing issue, but does the device teardown path leak these
pending work items?

When a mediated device is removed, vfio_ccw_mdev_release_dev() frees the
private structure without calling cancel_work_sync() on io_work or crw_work:

static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
{
        ...
        kfree(private);
}

Additionally, fsm_close() can skip the global flush_workqueue() if
cio_disable_subchannel() immediately returns 0:

ret = cio_disable_subchannel(sch);
if (ret == -EBUSY)
        ret = vfio_ccw_sch_quiesce(sch);

If a work item is left pending on the workqueue, could it execute after
the private structure has been freed, resulting in a use-after-free?

> -int vfio_ccw_sch_quiesce(struct subchannel *sch);
> +int vfio_ccw_sch_quiesce(struct subchannel *sch)
> +	__must_hold(&sch->lock);
>  void vfio_ccw_sch_io_todo(struct work_struct *work);
>  void vfio_ccw_crw_todo(struct work_struct *work);

[Severity: High]
This isn't a bug introduced by this patch, but does the lockless state check
in vfio_ccw_sch_io_todo() allow a double-free of the channel program?

The workqueue function vfio_ccw_sch_io_todo() reads private->state
locklessly:

if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
        cp_free(&private->cp);
        cp_is_finished = true;
}

Concurrently, fsm_close() holds sch->lock, changes the state to STANDBY,
unlocks, and also calls cp_free():

spin_unlock_irq(&sch->lock);
cp_free(&private->cp);

Since vfio_ccw_sch_io_todo() operates outside sch->lock, could both threads
evaluate their conditions simultaneously and concurrently execute cp_free()
on the same channel program?

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