Re: [PATCH v4] s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware activates.

[Nihar Ranjan Panda <niharp@linux.ibm.com>]

On 09/09/26 8:07 pm, Heiko Carstens wrote:
> [full quote below since additional people are added to cc]
> 
> On Wed, Sep 09, 2026 at 11:27:25AM +0530, Nihar Ranjan Panda wrote:
>> On 07/09/26 10:51 am, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>>>
>>> New issues:
>>> - [High] Lockless execution of cio_update_schib() causes a data race on the shared sch->schib structure.
>>> - [High] The timeout polling loop is vulnerable to a preemption bug resulting in a spurious -ETIMEDOUT failure.
>>> - [High] Missing I/O cancellation on timeout leaves a dangling channel program.
>>>
>>> Pre-existing issues:
>>> - [High] qdio_set_state() is called locklessly, potentially overwriting a STOPPED/ERR state.
>>> --
>>>
>> The above reported issues have been reviewed and determined to be invalid. 
>> Please find the clarification for each item detailed below.
> 
> I do disagree for at least two points (the other ones are up to other folks).
>  
>>> commit f5d84619d7d33842d3cc7f120e8e2f08b030d3f4
>>> Author: Nihar Panda <niharp@linux.ibm.com>
>>>
>>> s390/qdio: Ensure QDIO_IRQ_STATE_ACTIVE is set only after firmware activates.
>>>     
>>> This commit modifies the activation sequence for QDIO queues to poll the
>>> Subchannel Status Word (SCSW) instead of relying on an intermediate interrupt.
>>> It checks for both SCSW_ACTL_SCHACT and qact to become set within a loop before
>>> transitioning the state to QDIO_IRQ_STATE_ACTIVE.
>>>
>>>> diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c
>>>> index c1e09fa34e774..821d501efdccf 100644
>>>> --- a/drivers/s390/cio/qdio_main.c
>>>> +++ b/drivers/s390/cio/qdio_main.c
>>>
>>> [ ... ]
>>>
>>>> @@ -1178,23 +1197,32 @@ int qdio_activate(struct ccw_device *cdev)
>>>>  			      0, DOIO_DENY_PREFETCH);
>>>>  	spin_unlock_irq(get_ccwdev_lock(cdev));
>>>>  	if (rc) {
>>>> -		DBF_ERROR("%4x act IO ERR", irq_ptr->schid.sch_no);
>>>> -		DBF_ERROR("rc:%4x", rc);
>>>> +		DBF_ERROR("%4x act IE:%d", irq_ptr->schid.sch_no, rc);
>>>>  		goto out;
>>>>  	}
>>>>  
>>>> -	/* wait for subchannel to become active */
>>>> -	msleep(5);
>>>> +	rc = -ETIMEDOUT;
>>>> +	timeout = jiffies + HZ;
>>>>  
>>>> -	switch (irq_ptr->state) {
>>>> -	case QDIO_IRQ_STATE_STOPPED:
>>>> -	case QDIO_IRQ_STATE_ERR:
>>>> -		rc = -EIO;
>>>> -		break;
>>>> -	default:
>>>> -		qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
>>>> -		rc = 0;
>>>> +	while (time_before(jiffies, timeout)) {
>>>
>>> [Severity: High]
>>> Does this loop risk spurious timeouts under preemption or high CPU load?
>>>
>>> By checking the timeout condition before evaluating the hardware state, if
>>> preemption occurs right after the timeout is set and lasts longer than HZ,
>>> the loop will terminate immediately.
>>>
>>> This causes a spurious -ETIMEDOUT failure without ever checking the hardware
>>> status.
>>>
>> NO, the loop is safe because msleep + time_before(jiffies,…) is immune to preemption delay; 
>> jiffies advances independently
> 
> This is not correct. It the task that is doing
> 
> 	rc = -ETIMEDOUT;
> 	timeout = jiffies + HZ;
> 
> is preempted right after assigning the above to timeout, and is scheduled back
> in _after_ timeout then
> 
> 	while (time_before(jiffies, timeout)) {
> 
> evaluates to false. Which means that -ETIMEDOUT is returned, even though state
> has not been checked even once. This is a regression to before and needs to be
> fixed. Similar things can happen if the loop itself is preempted. It might
> have waited only 1, 2, 3, or 4ms, and might then be preempted - without
> checking again. Before it was a minimum of 5ms and a guaranteed check
> afterwards.
> 
> The 5ms might have been a random number, but it is a change and potential
> regression as well.
> 
Agreed. To prevent spurious -ETIMEDOUT errors caused by preemption prior to the initial loop evaluation, I will modify the logic to use a do .. while loop. This guarantees the hardware state is validated at least once.

>>>> +		msleep(1);
>>>> +		if (irq_ptr->state != QDIO_IRQ_STATE_ESTABLISHED) {
>>>> +			rc = -EIO;
>>>> +			DBF_ERROR("%4x act WS:%d", irq_ptr->schid.sch_no, irq_ptr->state);
>>>> +			break;
>>>> +		}
>>>> +		/* Query hardware */
>>>> +		if (cio_update_schib(sch) == 0) {
>>>
>>> [Severity: High]
>>> Is it safe to call cio_update_schib() without holding the subchannel lock here?
>>>
>>> The lock get_ccwdev_lock(cdev) was released earlier, and cio_update_schib()
>>> performs a lockless memcpy():
>>>
>>> drivers/s390/cio/cio.c:cio_update_schib() {
>>>     ...
>>> 	if (stsch(sch->schid, &schib))
>>> 		return -ENODEV;
>>>
>>> 	memcpy(&sch->schib, &schib, sizeof(schib));
>>>     ...
>>> }
>>>
>>> This results in a data race with concurrent interrupt handlers that might be
>>> updating sch->schib, potentially causing torn writes and state corruption.
>>>
>> Yes, it is safe to call cio_update_schib() without holding the subchannel lock. 
>> The implementation is secure for the following reasons:
>>
>> Subsystem Consistency: The lockless call is consistent with all other invocations of 
>> cio_update_schib() across the driver subsystem.
> 
> This is not true. All other places (except one) call cio_update_schib() with
> sch->lock being held.
> 
> The only other "offender" seems to be vfio_ccw_schib_region_read(), which
> looks like a bug.
> 
> I leave the rest below to other folks who have been on cc on your original
> email (+ adding Eric and Matthew for vfio).
> 

I will wrap the cio_update_schib() call with spin_lock_irq(get_ccwdev_lock(cdev)) to ensure the subchannel 
lock is held and prevent the data race.
>> Authoritative Data: Both concurrent writers are applying identical, hardware-authoritative state, 
>> eliminating the risk of conflicting data.
>> Writer A — the interrupt path (do_cio_interrupt()): memcpy(&sch->schib.scsw, &irb->scsw, …);
>> Writer B — the poll path (cio_update_schib()):memcpy(&sch->schib, &schib, …);
>>
>> Fault-Tolerant Design: The polling loop naturally tolerates a missed read and will simply retry, 
>> preventing state corruption.
>>
>>>> +			if ((sch->schib.scsw.cmd.actl & SCSW_ACTL_SCHACT)
>>>> +					&& sch->schib.scsw.cmd.qact) {
>>>> +				qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
>>>
>>> [Severity: High]
>>> This is a pre-existing issue, but is it safe for qdio_set_state() to assign
>>> the state locklessly?
>>>
>>> By directly assigning irq_ptr->state without serialization, it can overwrite a
>>> QDIO_IRQ_STATE_STOPPED or QDIO_IRQ_STATE_ERR state set concurrently by an
>>> interrupt handler.
>>>
>>> This could mask concurrent hardware errors and leave the device in a
>>> false-active state.
>>>
>>
>> Yes overwrite can happen between do_cio_interrupt()  and cio_update_schib().
>> The mb() inside qdio_set_state() ensures the write is ordered and visible, not exclusive.
>> The race is not new and is not worsened by the change.
>> The race window is smaller, the guard is tighter, and the hardware confirmation adds a second condition. 
>>
>>
>>>> +				rc = 0;
>>>> +				break;
>>>> +			}
>>>> +		}
>>>>  	}
>>>> +	if (rc == -ETIMEDOUT)
>>>> +		DBF_ERROR("%4x act TIMEOUT", irq_ptr->schid.sch_no);
>>>>  out:
>>>>  	mutex_unlock(&irq_ptr->setup_mutex);
>>>>  	return rc;
>>>
>>> [Severity: High]
>>> If this returns -ETIMEDOUT, is the pending asynchronous I/O left uncancelled?
>>>
>>> When the timeout expires, the function returns the error while the channel
>>> program started by ccw_device_start() may still be active in hardware.
>>>
>>> This could lead to use-after-free and memory corruption if the uncancelled CCW
>>> eventually completes and performs DMA writes to memory that has since been
>>> freed or repurposed.
>>>
>> No, as qdio_shutdown() → qdio_cancel_ccw() → ccw_device_halt/clear() is the 
>> mandatory error path all callers must invoke, so the channel program will be closed.
>>