[PATCH] Input: libps2 - annotate lockless command wait conditions

[PATCH] Input: libps2 - annotate lockless command wait conditions

[Zhu Jun]

PS/2 command waiters release the serio receive lock before waiting for
the interrupt handler to clear the command flags. Their wait conditions,
and the intermediate cmdcnt check, consequently race with the handler's
updates.

Sample these fields once and mark the intentionally lockless checks.
Keep the receive lock around response handling and flag updates, so
the final command result is still checked under the existing lock.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Signed-off-by: Zhu Jun <zhujun2@cmss.chinamobile.com>
---
 drivers/input/serio/libps2.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index 05b64277aecd..a60912224bfa 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -41,6 +41,12 @@
 #define PS2_FLAG_NAK		BIT(4)	/* Last transmission was NAKed */
 #define PS2_FLAG_PASS_NOACK	BIT(5)	/* Pass non-ACK byte to receive handler */
 
+static bool ps2_is_waiting(struct ps2dev *ps2dev, unsigned long flag)
+{
+	/* Waiters sample flags while the receive interrupt updates them. */
+	return data_race(READ_ONCE(ps2dev->flags)) & flag;
+}
+
 static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte,
 			   unsigned int timeout, unsigned int max_attempts)
 	__releases(&ps2dev->serio->lock) __acquires(&ps2dev->serio->lock)
@@ -62,7 +68,7 @@ static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte,
 				"failed to write %#02x: %d\n", byte, error);
 		else
 			wait_event_timeout(ps2dev->wait,
-					   !(ps2dev->flags & PS2_FLAG_ACK),
+					   !ps2_is_waiting(ps2dev, PS2_FLAG_ACK),
 					   msecs_to_jiffies(timeout));
 
 		serio_pause_rx(ps2dev->serio);
@@ -338,13 +344,17 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 	timeout = msecs_to_jiffies(command == PS2_CMD_RESET_BAT ? 4000 : 500);
 
 	timeout = wait_event_timeout(ps2dev->wait,
-				     !(ps2dev->flags & PS2_FLAG_CMD1), timeout);
+				     !ps2_is_waiting(ps2dev, PS2_FLAG_CMD1),
+				     timeout);
 
-	if (ps2dev->cmdcnt && !(ps2dev->flags & PS2_FLAG_CMD1)) {
+	/* The interrupt may advance cmdcnt between the two waits. */
+	if (data_race(READ_ONCE(ps2dev->cmdcnt)) &&
+	    !ps2_is_waiting(ps2dev, PS2_FLAG_CMD1)) {
 
 		timeout = ps2_adjust_timeout(ps2dev, command, timeout);
 		wait_event_timeout(ps2dev->wait,
-				   !(ps2dev->flags & PS2_FLAG_CMD), timeout);
+				   !ps2_is_waiting(ps2dev, PS2_FLAG_CMD),
+				   timeout);
 	}
 
 	serio_pause_rx(ps2dev->serio);
-- 
2.51.0

Re: [PATCH] Input: libps2 - annotate lockless command wait conditions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] Input: libps2 - unannotated lockless wait condition in
  ps2_drain()
--

commit 9801ff26658df02a74329e6bb2b403d12fd15880
Author: Zhu Jun <zhujun2@cmss.chinamobile.com>

Input: libps2 - annotate lockless command wait conditions

This commit introduces ps2_is_waiting() to annotate lockless wait conditions
on ps2dev->flags and ps2dev->cmdcnt with data_race(READ_ONCE(...)).

> diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
> index 05b64277aecd9..a60912224bfac 100644
> --- a/drivers/input/serio/libps2.c
> +++ b/drivers/input/serio/libps2.c
> @@ -41,6 +41,12 @@
>  #define PS2_FLAG_NAK		BIT(4)	/* Last transmission was NAKed */
>  #define PS2_FLAG_PASS_NOACK	BIT(5)	/* Pass non-ACK byte to receive handler */
>  
> +static bool ps2_is_waiting(struct ps2dev *ps2dev, unsigned long flag)
> +{
> +	/* Waiters sample flags while the receive interrupt updates them. */
> +	return data_race(READ_ONCE(ps2dev->flags)) & flag;
> +}
> +
>  static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte,
>  			   unsigned int timeout, unsigned int max_attempts)
>  	__releases(&ps2dev->serio->lock) __acquires(&ps2dev->serio->lock)
> @@ -62,7 +68,7 @@ static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte,
>  				"failed to write %#02x: %d\n", byte, error);
>  		else
>  			wait_event_timeout(ps2dev->wait,
> -					   !(ps2dev->flags & PS2_FLAG_ACK),
> +					   !ps2_is_waiting(ps2dev, PS2_FLAG_ACK),
>  					   msecs_to_jiffies(timeout));
>  
>  		serio_pause_rx(ps2dev->serio);

[Severity: Low]
Should ps2_drain() also use !ps2_is_waiting(ps2dev, PS2_FLAG_CMD) for its
wait_event_timeout() condition?

drivers/input/serio/libps2.c:ps2_drain() {
	...
	scoped_guard(serio_pause_rx, ps2dev->serio) {
		ps2dev->flags = PS2_FLAG_CMD;
		ps2dev->cmdcnt = maxbytes;
	}

	wait_event_timeout(ps2dev->wait,
			   !(ps2dev->flags & PS2_FLAG_CMD),
			   msecs_to_jiffies(timeout));
}

Like ps2_do_sendbyte() and __ps2_command(), ps2_drain() drops
ps2dev->serio->lock before waiting for the receive interrupt handler to
clear PS2_FLAG_CMD, leaving an unannotated data race on ps2dev->flags.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111084142.80954-1-zhujun2@cmss.chinamobile.com?part=1