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

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

[Nihar Panda]

Set QDIO_IRQ_STATE_ACTIVE only if both the subchannel-active bit and
the QDIO-active bit are set in the Subchannel Status Word (SCSW).

The channel subsystem sets the SCSW_ACTL_SCHACT bit in scsw.actl and
scsw.qact = 1 in the SCHIB to indicate that the activate-QDIO-queues
CCW program is running and the queues are ready.

An interrupt-driven approach is not applicable here.
Using CCW_FLAG_PCI on the activate CCW generates an intermediate interrupt
too early, before the firmware sets qact=1.
Therefore, polling the SCHIB via cio_update_schib() is the only way to
reliably detect when the queues are ready.

Signed-off-by: Nihar Panda <niharp@linux.ibm.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Nagamani PV <nagamani@linux.ibm.com>
---
 arch/s390/include/asm/scsw.h |  4 +--
 drivers/s390/cio/qdio_main.c | 54 +++++++++++++++++++++++++++---------
 2 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/arch/s390/include/asm/scsw.h b/arch/s390/include/asm/scsw.h
index 56003e26cdbf..bf00d827d72b 100644
--- a/arch/s390/include/asm/scsw.h
+++ b/arch/s390/include/asm/scsw.h
@@ -28,7 +28,7 @@
  * @zcc: zero condition code
  * @ectl: extended control
  * @pno: path not operational
- * @res: reserved
+ * @qact: qdio active
  * @fctl: function control
  * @actl: activity control
  * @stctl: status control
@@ -50,7 +50,7 @@ struct cmd_scsw {
 	__u32 zcc  : 1;
 	__u32 ectl : 1;
 	__u32 pno  : 1;
-	__u32 res  : 1;
+	__u32 qact : 1;
 	__u32 fctl : 3;
 	__u32 actl : 7;
 	__u32 stctl : 5;
diff --git a/drivers/s390/cio/qdio_main.c b/drivers/s390/cio/qdio_main.c
index c1e09fa34e77..821d501efdcc 100644
--- a/drivers/s390/cio/qdio_main.c
+++ b/drivers/s390/cio/qdio_main.c
@@ -1140,11 +1140,29 @@ EXPORT_SYMBOL_GPL(qdio_establish);
 /**
  * qdio_activate - activate queues on a qdio subchannel
  * @cdev: associated cdev
+ *
+ * This function must only be called when the QDIO subchannel is in
+ * QDIO_IRQ_STATE_ESTABLISHED state (i.e., after successful qdio_establish()).
+ * Any other state indicates either the subchannel is not ready or an error
+ * condition that requires proper recovery through qdio_shutdown() and
+ * qdio_establish() before activation can be attempted.
+ *
+ * Return:
+ * * 0		- success
+ * * -ENODEV	- device is not initialized
+ * * -EIO	- adapter lacks QDIO activation support, or
+ *                the IRQ state changed unexpectedly during activation
+ * * -EBUSY	- subchannel state is not QDIO_IRQ_STATE_ESTABLISHED
+ *                at call time
+ * * -ETIMEDOUT - subchannel failed to become active within the timeout
+ * * other	- standard error code forwarded from ccw_device_start()
  */
 int qdio_activate(struct ccw_device *cdev)
 {
+	struct subchannel *sch = to_subchannel(cdev->dev.parent);
 	struct qdio_irq *irq_ptr = cdev->private->qdio_data;
 	struct subchannel_id schid;
+	unsigned long timeout;
 	struct ciw *ciw;
 	int rc;
 
@@ -1161,7 +1179,8 @@ int qdio_activate(struct ccw_device *cdev)
 	}
 
 	mutex_lock(&irq_ptr->setup_mutex);
-	if (irq_ptr->state == QDIO_IRQ_STATE_INACTIVE) {
+	if (irq_ptr->state != QDIO_IRQ_STATE_ESTABLISHED) {
+		DBF_ERROR("%4x act WS:%d", schid.sch_no, irq_ptr->state);
 		rc = -EBUSY;
 		goto out;
 	}
@@ -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)) {
+		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) {
+			if ((sch->schib.scsw.cmd.actl & SCSW_ACTL_SCHACT)
+					&& sch->schib.scsw.cmd.qact) {
+				qdio_set_state(irq_ptr, QDIO_IRQ_STATE_ACTIVE);
+				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;
-- 
2.53.0

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

[sashiko-bot]

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.
--

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.

> +		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.

> +			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.

> +				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.

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

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

[Nihar Ranjan Panda]

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.

> 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
>> +		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.

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.

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

[Heiko Carstens]

[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.

> >> +		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).

> 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.
> 

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

[Nihar Ranjan Panda]

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.
>>