Re: [PATCH v1 6/6] s390/vfio_ccw: lock I/O resources alongside I/O regions

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces 'sleeping in atomic context' bugs by adding mutex_lock() calls in paths where a spinlock is held and interrupts are disabled.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but concurrent execution of `cp_free` can lead to a double free because `vfio_ccw_sch_io_todo` fails to hold `io_mutex`.
--

--- Patch [6]: [PATCH v1 6/6] s390/vfio_ccw: lock I/O resources alongside I/O regions ---
commit 60e83e6e496f63d77b199d14299ecacbd61f8fb1
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: lock I/O resources alongside I/O regions
    
The commit expands the scope of the I/O mutex from protecting just the
memory regions to also protecting related resources like channel programs
and subchannel events. It adds mutex locking around CRW queuing and
channel program freeing.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc728..385af7daca3b4 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -292,7 +292,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  	crw->crw.erc = erc;
>  	crw->crw.rsid = rsid;
>  
> +	mutex_lock(&private->io_mutex);

[Severity: High]
Does this cause a sleep-in-atomic bug?

The CIO core invokes the chp_event callback with the subchannel lock held
and interrupts disabled:

drivers/s390/cio/chsc.c:__s390_subchannel_vary_chpid() {
    spin_lock_irqsave(&sch->lock, flags);
    if (sch->driver && sch->driver->chp_event)
        sch->driver->chp_event(sch, &link, on ? CHP_VARY_ON : CHP_VARY_OFF);
    ...
}

This eventually calls vfio_ccw_queue_crw(), which now attempts to acquire
the sleeping io_mutex while the spinlock is held.

>  	list_add_tail(&crw->next, &private->crw);
> +	mutex_unlock(&private->io_mutex);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef0..96f23da88a396 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -171,7 +171,9 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
>  
>  	/* This is usually handled during CLOSE event */
> +	mutex_lock(&private->io_mutex);

[Severity: High]
Can this also trigger a sleep-in-atomic panic?

Similar to the CRW queuing path, fsm_notoper() is reached from
vfio_ccw_sch_event(), which holds the same subchannel spinlock:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
    spin_lock_irqsave(&sch->lock, flags);
    ...
    vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of locking in
vfio_ccw_sch_io_todo() allow a double-free race with this cp_free() call?

While this patch adds io_mutex protection here in fsm_notoper(), the
I/O completion workqueue conditionally calls cp_free() without acquiring
io_mutex:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
        cp_free(&private->cp);
    ...
}

Because cp_free() checks and modifies cp->initialized non-atomically:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    if (!cp->initialized)
        return;
    cp->initialized = false;
    ...
}

could these two paths execute the freeing logic simultaneously if a
subchannel becomes not operational exactly as its I/O completes?

>  	cp_free(&private->cp);
> +	mutex_unlock(&private->io_mutex);
>  }

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