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

Eric, Matthew,

not sure if you are aware of the below (nothing I would address). Please handle :)

On Thu, Aug 06, 2026 at 03:52:23PM +0000, [email protected] wrote:
> 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 <[email protected]>
> 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/[email protected]?part=1