* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-06 15:38 ` [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce() Heiko Carstens
@ 2026-08-06 15:52 ` sashiko-bot
2026-08-06 16:02 ` Heiko Carstens
2026-08-06 17:34 ` Matthew Rosato
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: sashiko-bot @ 2026-08-06 15:52 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-s390, Heiko Carstens, Alexander Gordeev, Vasily Gorbik
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-06 15:52 ` sashiko-bot
@ 2026-08-06 16:02 ` Heiko Carstens
2026-08-06 17:26 ` Matthew Rosato
0 siblings, 1 reply; 16+ messages in thread
From: Heiko Carstens @ 2026-08-06 16:02 UTC (permalink / raw)
To: Eric Farman, Matthew Rosato; +Cc: linux-s390, Alexander Gordeev, Vasily Gorbik
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, sashiko-bot@kernel.org 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 <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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-06 15:38 ` [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce() Heiko Carstens
2026-08-06 15:52 ` sashiko-bot
@ 2026-08-06 17:34 ` Matthew Rosato
2026-08-07 10:55 ` Christian Borntraeger
2026-08-07 12:14 ` Christian Borntraeger
3 siblings, 0 replies; 16+ messages in thread
From: Matthew Rosato @ 2026-08-06 17:34 UTC (permalink / raw)
To: Heiko Carstens, Christian Borntraeger, Janosch Frank,
Claudio Imbrenda, Eric Farman, Vineeth Vijayan,
Peter Oberparleiter
Cc: linux-s390
On 8/6/26 11:38 AM, Heiko Carstens wrote:
> Add __must_hold() attribute to vfio_ccw_sch_quiesce() in order to let
> clang's context analysis know that sch->lock must be held on function
> entry. This can also be easily verified when inspecting the function.
>
> Without this annotation this leads to a valid warning when context
> analysis is enabled:
>
> drivers/s390/cio/vfio_ccw_drv.c:55:9: warning:
> expecting spinlock 'sch->lock' to be held at start of each loop [-Wthread-safety-analysis]
> 55 | ret = cio_cancel_halt_clear(sch, &iretry);
> | ^
>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-06 15:38 ` [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce() Heiko Carstens
2026-08-06 15:52 ` sashiko-bot
2026-08-06 17:34 ` Matthew Rosato
@ 2026-08-07 10:55 ` Christian Borntraeger
2026-08-07 11:15 ` Heiko Carstens
2026-08-07 12:14 ` Christian Borntraeger
3 siblings, 1 reply; 16+ messages in thread
From: Christian Borntraeger @ 2026-08-07 10:55 UTC (permalink / raw)
To: Heiko Carstens, Janosch Frank, Claudio Imbrenda, Eric Farman,
Matthew Rosato, Vineeth Vijayan, Peter Oberparleiter
Cc: linux-s390
Am 06.08.26 um 17:38 schrieb Heiko Carstens:
> Add __must_hold() attribute to vfio_ccw_sch_quiesce() in order to let
> clang's context analysis know that sch->lock must be held on function
> entry. This can also be easily verified when inspecting the function.
>
> Without this annotation this leads to a valid warning when context
> analysis is enabled:
>
> drivers/s390/cio/vfio_ccw_drv.c:55:9: warning:
> expecting spinlock 'sch->lock' to be held at start of each loop [-Wthread-safety-analysis]
> 55 | ret = cio_cancel_halt_clear(sch, &iretry);
> | ^
>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
I assume this series should go via the s390 tree with the others?
> ---
> drivers/s390/cio/vfio_ccw_drv.c | 1 +
> drivers/s390/cio/vfio_ccw_private.h | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc72..ed9ca77c5b23 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);
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 0501d4bbcdbd..8db29519dbfd 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);
>
> -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);
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-07 10:55 ` Christian Borntraeger
@ 2026-08-07 11:15 ` Heiko Carstens
0 siblings, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2026-08-07 11:15 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Janosch Frank, Claudio Imbrenda, Eric Farman, Matthew Rosato,
Vineeth Vijayan, Peter Oberparleiter, linux-s390
On Fri, Aug 07, 2026 at 12:55:28PM +0200, Christian Borntraeger wrote:
>
>
> Am 06.08.26 um 17:38 schrieb Heiko Carstens:
> > Add __must_hold() attribute to vfio_ccw_sch_quiesce() in order to let
> > clang's context analysis know that sch->lock must be held on function
> > entry. This can also be easily verified when inspecting the function.
> >
> > Without this annotation this leads to a valid warning when context
> > analysis is enabled:
> >
> > drivers/s390/cio/vfio_ccw_drv.c:55:9: warning:
> > expecting spinlock 'sch->lock' to be held at start of each loop [-Wthread-safety-analysis]
> > 55 | ret = cio_cancel_halt_clear(sch, &iretry);
> > | ^
> >
> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
>
> Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>
> I assume this series should go via the s390 tree with the others?
As written in the cover-letter:
"Christian, Claudio, Janosch, I think it would make most sense if this
goes via kvms390."
:)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-06 15:38 ` [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce() Heiko Carstens
` (2 preceding siblings ...)
2026-08-07 10:55 ` Christian Borntraeger
@ 2026-08-07 12:14 ` Christian Borntraeger
2026-08-07 12:29 ` Heiko Carstens
3 siblings, 1 reply; 16+ messages in thread
From: Christian Borntraeger @ 2026-08-07 12:14 UTC (permalink / raw)
To: Heiko Carstens, Janosch Frank, Claudio Imbrenda, Eric Farman,
Matthew Rosato, Vineeth Vijayan, Peter Oberparleiter
Cc: linux-s390
Am 06.08.26 um 17:38 schrieb Heiko Carstens:
> Add __must_hold() attribute to vfio_ccw_sch_quiesce() in order to let
> clang's context analysis know that sch->lock must be held on function
> entry. This can also be easily verified when inspecting the function.
>
> Without this annotation this leads to a valid warning when context
> analysis is enabled:
>
> drivers/s390/cio/vfio_ccw_drv.c:55:9: warning:
> expecting spinlock 'sch->lock' to be held at start of each loop [-Wthread-safety-analysis]
> 55 | ret = cio_cancel_halt_clear(sch, &iretry);
> | ^
>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
> drivers/s390/cio/vfio_ccw_drv.c | 1 +
> drivers/s390/cio/vfio_ccw_private.h | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc72..ed9ca77c5b23 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);
Looks like this ; is wrong ^
will fixup when applying.
> {
> 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 0501d4bbcdbd..8db29519dbfd 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);
>
> -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);
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/3] s390/vfio_ccw: Add __must_hold() attribute to vfio_ccw_sch_quiesce()
2026-08-07 12:14 ` Christian Borntraeger
@ 2026-08-07 12:29 ` Heiko Carstens
0 siblings, 0 replies; 16+ messages in thread
From: Heiko Carstens @ 2026-08-07 12:29 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Janosch Frank, Claudio Imbrenda, Eric Farman, Matthew Rosato,
Vineeth Vijayan, Peter Oberparleiter, linux-s390
On Fri, Aug 07, 2026 at 02:14:27PM +0200, Christian Borntraeger wrote:
> > int vfio_ccw_sch_quiesce(struct subchannel *sch)
> > + __must_hold(&sch->lock);
> Looks like this ; is wrong ^
>
> will fixup when applying.
Right. Thanks!
^ permalink raw reply [flat|nested] 16+ messages in thread