[PATCH AUTOSEL 6.19-6.6] ipv4: igmp: annotate data-races around idev->mr_maxdelay

[Sasha Levin <sashal@kernel.org>]

From: Eric Dumazet <edumazet@google.com>

[ Upstream commit e4faaf65a75f650ac4366ddff5dabb826029ca5a ]

idev->mr_maxdelay is read and written locklessly,
add READ_ONCE()/WRITE_ONCE() annotations.

While we are at it, make this field an u32.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
Link: https://patch.msgid.link/20260122172247.2429403-1-edumazet@google.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Analysis of commit: "ipv4: igmp: annotate data-races around
idev->mr_maxdelay"

### 1. COMMIT MESSAGE ANALYSIS

The commit message is straightforward: it adds READ_ONCE()/WRITE_ONCE()
annotations to `idev->mr_maxdelay` because the field is read and written
locklessly (i.e., without holding a lock that protects both the reader
and writer). Additionally, the field type is changed from `unsigned
long` to `u32`.

**Author**: Eric Dumazet — a highly prolific and respected networking
developer at Google, known for data-race annotation work across the
networking stack.

**Reviewed-by**: David Ahern — another senior networking developer.

### 2. CODE CHANGE ANALYSIS

The changes are minimal and contained:

**In `include/linux/inetdevice.h`:**
- `mr_maxdelay` field type changed from `unsigned long` to `u32`
- Field repositioned in the struct (moved after `mr_gq_running` which is
  `unsigned char`, before `mr_ifc_count` which is `u32`) — this is
  likely for better packing/alignment

**In `net/ipv4/igmp.c`:**
- **Line ~230 (igmp_gq_start_timer)**: `in_dev->mr_maxdelay` →
  `READ_ONCE(in_dev->mr_maxdelay)` — the reader side
- **Line ~1012 (igmp_heard_query)**: `in_dev->mr_maxdelay = max_delay` →
  `WRITE_ONCE(in_dev->mr_maxdelay, max_delay)` — the writer side

### 3. DATA RACE ANALYSIS

Let me examine the concurrency situation:

- **Writer**: `igmp_heard_query()` writes `mr_maxdelay` when processing
  an incoming IGMPv3 query. This runs in softirq/BH context when
  receiving network packets.
- **Reader**: `igmp_gq_start_timer()` reads `mr_maxdelay` to calculate a
  random timer delay. This is called from `igmp_heard_query()` itself,
  but also potentially from other contexts.

The key question: can the read and write happen concurrently? Looking at
the code, `igmp_heard_query()` writes `mr_maxdelay` and then calls
`igmp_gq_start_timer()` which reads it. However, on different CPUs
processing different network packets, one CPU could be writing while
another reads. Without proper annotations, the compiler could optimize
these accesses in ways that cause tearing or stale reads.

This is a real KCSAN-detectable data race. While the consequences may
not be catastrophic (the value is used for a random timer delay), it is
technically undefined behavior in the C memory model, and on
architectures where `unsigned long` is 64-bit but atomicity is only
guaranteed for 32-bit, there could be torn reads producing garbage
values that get passed to `get_random_u32_below()`.

### 4. TYPE CHANGE: unsigned long → u32

The change from `unsigned long` to `u32` is notable:
- `max_delay` in `igmp_heard_query()` is computed as
  `IGMPV3_MRC(ih3->code)*(HZ/IGMP_TIMER_SCALE)` — the MRC field is from
  a network packet and is bounded, so the value fits in u32.
- `get_random_u32_below()` takes a `u32` argument, so this makes the
  types consistent.
- On 64-bit systems, this also ensures atomic read/write since u32
  accesses are atomic on all Linux-supported architectures, which
  complements the READ_ONCE/WRITE_ONCE annotations.

### 5. RISK ASSESSMENT

**Risk**: Very low.
- The READ_ONCE/WRITE_ONCE annotations are purely compiler directives
  that don't change runtime behavior on most architectures
- The type change from `unsigned long` to `u32` is safe because the
  values stored are always small (timer delays in jiffies, derived from
  IGMP protocol fields)
- The struct field reordering doesn't affect functionality
- Only 2 files changed, only 3 lines of actual code logic changed

**Benefit**: Fixes a data race that could theoretically cause:
- Torn reads on 32-bit architectures (if `unsigned long` were involved
  in non-atomic access — though on 32-bit it's 32-bit anyway)
- Compiler-induced issues where the compiler might reload or optimize
  the value in unexpected ways
- Silences KCSAN warnings, which is important for finding real races

### 6. STABLE SUITABILITY

**Meets stable criteria?**
- **Obviously correct**: Yes — this is a textbook data-race annotation
  pattern from a top networking developer, reviewed by another expert
- **Fixes a real bug**: Yes — data races are real bugs (KCSAN reports
  them), even if the consequences are subtle
- **Small and contained**: Yes — minimal changes across 2 files
- **No new features**: Correct — purely a bug fix
- **No new APIs**: Correct

**Concerns:**
- The type change (`unsigned long` → `u32`) and struct field reordering
  are slightly beyond "pure annotation" and could theoretically conflict
  with other patches. However, the type change is necessary for
  correctness (ensuring atomicity on all architectures).
- The struct layout change might cause minor backport conflicts if other
  fields were added/modified in stable trees.

### 7. PRECEDENT

Eric Dumazet has authored hundreds of similar READ_ONCE/WRITE_ONCE
annotation patches in the networking stack, and many of them have been
backported to stable. These are considered important for correctness and
for enabling KCSAN to find real bugs by eliminating false positives.

### 8. CONCLUSION

This is a small, well-reviewed fix for a real data race in the IPv4 IGMP
code path. The IGMP code handles multicast group membership and is used
on any system with multicast networking. The fix is from a trusted
author, reviewed by a trusted reviewer, and follows established
patterns. The type change is safe and actually improves correctness. The
risk of regression is negligible.

**YES**

 include/linux/inetdevice.h | 2 +-
 net/ipv4/igmp.c            | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 5730ba6b1cfaf..dccbeb25f7014 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -38,11 +38,11 @@ struct in_device {
 	struct ip_mc_list	*mc_tomb;
 	unsigned long		mr_v1_seen;
 	unsigned long		mr_v2_seen;
-	unsigned long		mr_maxdelay;
 	unsigned long		mr_qi;		/* Query Interval */
 	unsigned long		mr_qri;		/* Query Response Interval */
 	unsigned char		mr_qrv;		/* Query Robustness Variable */
 	unsigned char		mr_gq_running;
+	u32			mr_maxdelay;
 	u32			mr_ifc_count;
 	struct timer_list	mr_gq_timer;	/* general query timer */
 	struct timer_list	mr_ifc_timer;	/* interface change timer */
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 7182f1419c2a4..0adc993c211d7 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -227,7 +227,7 @@ static void igmp_start_timer(struct ip_mc_list *im, int max_delay)
 
 static void igmp_gq_start_timer(struct in_device *in_dev)
 {
-	int tv = get_random_u32_below(in_dev->mr_maxdelay);
+	int tv = get_random_u32_below(READ_ONCE(in_dev->mr_maxdelay));
 	unsigned long exp = jiffies + tv + 2;
 
 	if (in_dev->mr_gq_running &&
@@ -1009,7 +1009,7 @@ static bool igmp_heard_query(struct in_device *in_dev, struct sk_buff *skb,
 		max_delay = IGMPV3_MRC(ih3->code)*(HZ/IGMP_TIMER_SCALE);
 		if (!max_delay)
 			max_delay = 1;	/* can't mod w/ 0 */
-		in_dev->mr_maxdelay = max_delay;
+		WRITE_ONCE(in_dev->mr_maxdelay, max_delay);
 
 		/* RFC3376, 4.1.6. QRV and 4.1.7. QQIC, when the most recently
 		 * received value was zero, use the default or statically
-- 
2.51.0