linux-kernel - Re: [PATCH v3] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o88favd9.ffs@tglx>
Date:   Sun, 26 Sep 2021 23:18:42 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Iain Hunter <drhunter95@...il.com>
Cc:     lothar.felten@...il.com, iain@...terembedded.co.uk,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        Gwendal Grignou <gwendal@...omium.org>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        Arnd Bergmann <arnd@...db.de>,
        Zeng Tao <prime.zeng@...ilicon.com>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] workaround regression in ina2xx introduced by
 cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")

On Sun, Sep 26 2021 at 18:16, Iain Hunter wrote:
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -817,10 +817,10 @@ static int ina2xx_capture_thread(void *data)
>  		 */
>  		do {
>  			timespec64_add_ns(&next, 1000 * sampling_us);
> -			delta = timespec64_sub(next, now);
> -			delay_us = div_s64(timespec64_to_ns(&delta), 1000);
> -		} while (delay_us <= 0);
> +		} while (timespec64_compare(&next, &now) < 0);
>  
> +		delta = timespec64_sub(next, now);
> +		delay_us = div_s64(timespec64_to_ns(&delta), 1000);

This whole timespec dance does not make any sense and can be completely
avoided by using just scalar nanoseconds. Untested patch below.

Thanks,

        tglx
---
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -775,7 +775,7 @@ static int ina2xx_capture_thread(void *d
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
 	int sampling_us = SAMPLING_PERIOD(chip);
 	int ret;
-	struct timespec64 next, now, delta;
+	ktime_t next, now, delta;
 	s64 delay_us;
 
 	/*
@@ -785,7 +785,7 @@ static int ina2xx_capture_thread(void *d
 	if (!chip->allow_async_readout)
 		sampling_us -= 200;
 
-	ktime_get_ts64(&next);
+	next = ktime_get();
 
 	do {
 		while (!chip->allow_async_readout) {
@@ -798,7 +798,7 @@ static int ina2xx_capture_thread(void *d
 			 * reset the reference timestamp.
 			 */
 			if (ret == 0)
-				ktime_get_ts64(&next);
+				next = ktime_get();
 			else
 				break;
 		}
@@ -807,7 +807,7 @@ static int ina2xx_capture_thread(void *d
 		if (ret < 0)
 			return ret;
 
-		ktime_get_ts64(&now);
+		now = ktime_get();
 
 		/*
 		 * Advance the timestamp for the next poll by one sampling
@@ -816,11 +816,10 @@ static int ina2xx_capture_thread(void *d
 		 * multiple times, i.e. samples are dropped.
 		 */
 		do {
-			timespec64_add_ns(&next, 1000 * sampling_us);
-			delta = timespec64_sub(next, now);
-			delay_us = div_s64(timespec64_to_ns(&delta), 1000);
-		} while (delay_us <= 0);
+			next = ktime_add_us(next, sampling_us);
+		} while (next <= now);
 
+		delay_us = ktime_to_us(ktime_sub(next, now));
 		usleep_range(delay_us, (delay_us * 3) >> 1);
 
 	} while (!kthread_should_stop());