drivers/hwmon/max16065.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
Simply copying shared data to a local variable cannot prevent data
races. The compiler is allowed to optimize away the local copy and
re-read the shared memory, causing a Time-of-Check Time-of-Use (TOCTOU)
issue if the data changes between the check and the usage.
To enforce the use of the local variable, use READ_ONCE() when reading
the shared data and WRITE_ONCE() when updating it. Apply these macros to
the three identified locations (curr_sense, adc, and fault) where local
variables are used for error validation, ensuring the value remains
consistent.
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Closes: https://lore.kernel.org/all/6fe17868327207e8b850cf9f88b7dc58b2021f73.camel@decadent.org.uk/
Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
Fixes: b8d5acdcf525 ("hwmon: (max16065) Use local variable to avoid TOCTOU")
Cc: stable@vger.kernel.org
Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
---
drivers/hwmon/max16065.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/max16065.c b/drivers/hwmon/max16065.c
index 4c9e7892a73c..43fbb9b26b10 100644
--- a/drivers/hwmon/max16065.c
+++ b/drivers/hwmon/max16065.c
@@ -151,27 +151,27 @@ static struct max16065_data *max16065_update_device(struct device *dev)
int i;
for (i = 0; i < data->num_adc; i++)
- data->adc[i]
- = max16065_read_adc(client, MAX16065_ADC(i));
+ WRITE_ONCE(data->adc[i],
+ max16065_read_adc(client, MAX16065_ADC(i)));
if (data->have_current) {
- data->adc[MAX16065_NUM_ADC]
- = max16065_read_adc(client, MAX16065_CSP_ADC);
- data->curr_sense
- = i2c_smbus_read_byte_data(client,
- MAX16065_CURR_SENSE);
+ WRITE_ONCE(data->adc[MAX16065_NUM_ADC],
+ max16065_read_adc(client, MAX16065_CSP_ADC));
+ WRITE_ONCE(data->curr_sense,
+ i2c_smbus_read_byte_data(client, MAX16065_CURR_SENSE));
}
for (i = 0; i < 2; i++)
- data->fault[i]
- = i2c_smbus_read_byte_data(client, MAX16065_FAULT(i));
+ WRITE_ONCE(data->fault[i],
+ i2c_smbus_read_byte_data(client, MAX16065_FAULT(i)));
/*
* MAX16067 and MAX16068 have separate undervoltage and
* overvoltage alarm bits. Squash them together.
*/
if (data->chip == max16067 || data->chip == max16068)
- data->fault[0] |= data->fault[1];
+ WRITE_ONCE(data->fault[0],
+ data->fault[0] | data->fault[1]);
data->last_updated = jiffies;
data->valid = true;
@@ -185,7 +185,7 @@ static ssize_t max16065_alarm_show(struct device *dev,
{
struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(da);
struct max16065_data *data = max16065_update_device(dev);
- int val = data->fault[attr2->nr];
+ int val = READ_ONCE(data->fault[attr2->nr]);
if (val < 0)
return val;
@@ -203,7 +203,7 @@ static ssize_t max16065_input_show(struct device *dev,
{
struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
struct max16065_data *data = max16065_update_device(dev);
- int adc = data->adc[attr->index];
+ int adc = READ_ONCE(data->adc[attr->index]);
if (unlikely(adc < 0))
return adc;
@@ -216,7 +216,7 @@ static ssize_t max16065_current_show(struct device *dev,
struct device_attribute *da, char *buf)
{
struct max16065_data *data = max16065_update_device(dev);
- int curr_sense = data->curr_sense;
+ int curr_sense = READ_ONCE(data->curr_sense);
if (unlikely(curr_sense < 0))
return curr_sense;
--
2.43.0On Tue, 3 Feb 2026 20:14:43 +0800
Gui-Dong Han <hanguidong02@gmail.com> wrote:
> Simply copying shared data to a local variable cannot prevent data
> races. The compiler is allowed to optimize away the local copy and
> re-read the shared memory, causing a Time-of-Check Time-of-Use (TOCTOU)
> issue if the data changes between the check and the usage.
While the compiler is allowed to do this, is there any indication
that either gcc or clang have ever done it?
ISTR someone saying that they never did - although I thought that
was the original justification for adding ACCESS_ONCE().
READ_ONCE() also includes barriers to guarantee ordering between cpu.
These are empty on x86 but add code to architectures where the cpu
can (IIRC) re-order writes.
This is worst on alpha but affects arm and probably ppc.
For these cases is it enough to add the compile-time barrier() after
reading the variable to a local.
That will also generate better code on x86.
The WRITE_ONCE() aren't needed at all, the compilers definitely
guarantee to do a single memory access for aligned accesses that are
less than the size of a word.
This all stinks of being an AI generated patch.
David
>
> To enforce the use of the local variable, use READ_ONCE() when reading
> the shared data and WRITE_ONCE() when updating it. Apply these macros to
> the three identified locations (curr_sense, adc, and fault) where local
> variables are used for error validation, ensuring the value remains
> consistent.
>
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Closes: https://lore.kernel.org/all/6fe17868327207e8b850cf9f88b7dc58b2021f73.camel@decadent.org.uk/
> Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
> Fixes: b8d5acdcf525 ("hwmon: (max16065) Use local variable to avoid TOCTOU")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
> ---
> drivers/hwmon/max16065.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwmon/max16065.c b/drivers/hwmon/max16065.c
> index 4c9e7892a73c..43fbb9b26b10 100644
> --- a/drivers/hwmon/max16065.c
> +++ b/drivers/hwmon/max16065.c
> @@ -151,27 +151,27 @@ static struct max16065_data *max16065_update_device(struct device *dev)
> int i;
>
> for (i = 0; i < data->num_adc; i++)
> - data->adc[i]
> - = max16065_read_adc(client, MAX16065_ADC(i));
> + WRITE_ONCE(data->adc[i],
> + max16065_read_adc(client, MAX16065_ADC(i)));
>
> if (data->have_current) {
> - data->adc[MAX16065_NUM_ADC]
> - = max16065_read_adc(client, MAX16065_CSP_ADC);
> - data->curr_sense
> - = i2c_smbus_read_byte_data(client,
> - MAX16065_CURR_SENSE);
> + WRITE_ONCE(data->adc[MAX16065_NUM_ADC],
> + max16065_read_adc(client, MAX16065_CSP_ADC));
> + WRITE_ONCE(data->curr_sense,
> + i2c_smbus_read_byte_data(client, MAX16065_CURR_SENSE));
> }
>
> for (i = 0; i < 2; i++)
> - data->fault[i]
> - = i2c_smbus_read_byte_data(client, MAX16065_FAULT(i));
> + WRITE_ONCE(data->fault[i],
> + i2c_smbus_read_byte_data(client, MAX16065_FAULT(i)));
>
> /*
> * MAX16067 and MAX16068 have separate undervoltage and
> * overvoltage alarm bits. Squash them together.
> */
> if (data->chip == max16067 || data->chip == max16068)
> - data->fault[0] |= data->fault[1];
> + WRITE_ONCE(data->fault[0],
> + data->fault[0] | data->fault[1]);
>
> data->last_updated = jiffies;
> data->valid = true;
> @@ -185,7 +185,7 @@ static ssize_t max16065_alarm_show(struct device *dev,
> {
> struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(da);
> struct max16065_data *data = max16065_update_device(dev);
> - int val = data->fault[attr2->nr];
> + int val = READ_ONCE(data->fault[attr2->nr]);
>
> if (val < 0)
> return val;
> @@ -203,7 +203,7 @@ static ssize_t max16065_input_show(struct device *dev,
> {
> struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> struct max16065_data *data = max16065_update_device(dev);
> - int adc = data->adc[attr->index];
> + int adc = READ_ONCE(data->adc[attr->index]);
>
> if (unlikely(adc < 0))
> return adc;
> @@ -216,7 +216,7 @@ static ssize_t max16065_current_show(struct device *dev,
> struct device_attribute *da, char *buf)
> {
> struct max16065_data *data = max16065_update_device(dev);
> - int curr_sense = data->curr_sense;
> + int curr_sense = READ_ONCE(data->curr_sense);
>
> if (unlikely(curr_sense < 0))
> return curr_sense;
On Sat, Feb 7, 2026 at 6:43 PM David Laight <david.laight.linux@gmail.com> wrote: > > On Tue, 3 Feb 2026 20:14:43 +0800 > Gui-Dong Han <hanguidong02@gmail.com> wrote: > > > Simply copying shared data to a local variable cannot prevent data > > races. The compiler is allowed to optimize away the local copy and > > re-read the shared memory, causing a Time-of-Check Time-of-Use (TOCTOU) > > issue if the data changes between the check and the usage. > > While the compiler is allowed to do this, is there any indication > that either gcc or clang have ever done it? > ISTR someone saying that they never did - although I thought that > was the original justification for adding ACCESS_ONCE(). This patch addresses an issue originally reported by Ben Hutchings during his review of the 5.10 stable queue. Ben explicitly pointed out the potential race and suggested using READ/WRITE_ONCE to enforce local variable usage [1]. Many of his recent suggestions on stable patches have been adopted by maintainers like Greg KH. > > READ_ONCE() also includes barriers to guarantee ordering between cpu. > These are empty on x86 but add code to architectures where the cpu > can (IIRC) re-order writes. > This is worst on alpha but affects arm and probably ppc. > > For these cases is it enough to add the compile-time barrier() after > reading the variable to a local. > That will also generate better code on x86. > > The WRITE_ONCE() aren't needed at all, the compilers definitely > guarantee to do a single memory access for aligned accesses that are > less than the size of a word. Following his report, I consulted the LKMM documentation. The access pattern here fits the definition of a data race, and the documentation recommends annotating these accesses to eliminate the data race [2]. > > This all stinks of being an AI generated patch. I assure you this patch was not generated by AI. It was created based on feedback from an experienced developer and kernel documentation. Thanks. [1] https://lore.kernel.org/all/6fe17868327207e8b850cf9f88b7dc58b2021f73.camel@decadent.org.uk/ [2] https://elixir.bootlin.com/linux/v6.19-rc5/source/tools/memory-model/Documentation/explanation.txt#L2231
On Sat, 2026-02-07 at 10:43 +0000, David Laight wrote:
> On Tue, 3 Feb 2026 20:14:43 +0800
> Gui-Dong Han <hanguidong02@gmail.com> wrote:
>
> > Simply copying shared data to a local variable cannot prevent data
> > races. The compiler is allowed to optimize away the local copy and
> > re-read the shared memory, causing a Time-of-Check Time-of-Use (TOCTOU)
> > issue if the data changes between the check and the usage.
>
> While the compiler is allowed to do this, is there any indication
> that either gcc or clang have ever done it?
> ISTR someone saying that they never did - although I thought that
> was the original justification for adding ACCESS_ONCE().
They do it sometimes and it's precisely why these maros were added. It
makes no sense to me to look at what these compilers currrently do (for
some particular versions, optimisation settings, and targets) and
extrapolate that to the assertion that they will never optimise away a
copy.
> READ_ONCE() also includes barriers to guarantee ordering between cpu.
> These are empty on x86 but add code to architectures where the cpu
> can (IIRC) re-order writes.
> This is worst on alpha but affects arm and probably ppc.
No, READ_ONCE() and WRITE_ONCE() don't include any CPU memory barriers.
> For these cases is it enough to add the compile-time barrier() after
> reading the variable to a local.
> That will also generate better code on x86.
>
> The WRITE_ONCE() aren't needed at all, the compilers definitely
> guarantee to do a single memory access for aligned accesses that are
> less than the size of a word.
I think in this case WRITE_ONCE() might not be needed, but it's also
harmless and it's much easier to reason about {READ,WRITE}_ONCE() being
paired up.
> This all stinks of being an AI generated patch.
This is a follow-up to an earlier patch that claimed to fix the TOCTOU
issue. I objected to that because in the absense of READ_ONCE() it was
not guaranteed to do so.
Ben.
--
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.
On Tue, Feb 03, 2026 at 08:14:43PM +0800, Gui-Dong Han wrote:
> Simply copying shared data to a local variable cannot prevent data
> races. The compiler is allowed to optimize away the local copy and
> re-read the shared memory, causing a Time-of-Check Time-of-Use (TOCTOU)
> issue if the data changes between the check and the usage.
>
> To enforce the use of the local variable, use READ_ONCE() when reading
> the shared data and WRITE_ONCE() when updating it. Apply these macros to
> the three identified locations (curr_sense, adc, and fault) where local
> variables are used for error validation, ensuring the value remains
> consistent.
>
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Closes: https://lore.kernel.org/all/6fe17868327207e8b850cf9f88b7dc58b2021f73.camel@decadent.org.uk/
> Fixes: f5bae2642e3d ("hwmon: Driver for MAX16065 System Manager and compatibles")
> Fixes: b8d5acdcf525 ("hwmon: (max16065) Use local variable to avoid TOCTOU")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@gmail.com>
I don't claim to be an expert in this area, so I ran the patch through
an AI review and asked it if it is both necessary and complete. It claims
that this is the case, so I am going to apply this patch.
For those interested, I attached the AI review results below.
Thanks,
Guenter
---
[AI generated]
Summary:
The patch addresses a theoretical data race and TOCTOU (Time-of-Check
Time-of-Use) issue where compiler optimizations could reload shared data
(data->adc, data->curr_sense, data->fault) after a validity check but
before usage. By employing READ_ONCE in the reader functions
(max16065_alarm_show, max16065_input_show, max16065_current_show) and
WRITE_ONCE in the updater (max16065_update_device), the change ensures
that the compiler treats these accesses as volatile, preventing reloads
and ensuring that the value checked is the same as the value used.
The logic correctly handles the "lockless reader / locked updater"
pattern common in hwmon drivers.
Verification:
* Concurrency: The max16065_update_device function holds data->update_lock
during updates, ensuring only one thread updates the cache. The
READ_ONCE/WRITE_ONCE macros correctly protect against compiler
tearing/reordering for the readers that run outside this lock.
* Error Handling: Error codes from i2c_smbus_read_byte_data and
max16065_read_adc are correctly stored in the data structure and
checked in the show functions (via unlikely(val < 0)).
* Code Style: The changes follow kernel coding style and best practices
for concurrency annotations.
* Regressions: No regressions were found.
False Positives Eliminated:
* Self-read in `max16065_update_device`: The line WRITE_ONCE(data->fault[0],
data->fault[0] | data->fault[1]); reads data->fault[0] without READ_ONCE.
This is safe because max16065_update_device holds the update_lock,
so no other thread can be modifying data->fault concurrently.
© 2016 - 2026 Red Hat, Inc.