Re: [PATCH net 3/5] ipv6: mcast: fix delay calculation in igmp6_join_group()

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipv6: mcast: fix delay calculation in igmp6_join_group()

igmp6_join_group() reused the remaining expiry of an already scheduled
report work as the new delay without bounding it. The patch initializes
delay to unsolicited_report_interval(ma->idev) and re-randomizes it with
get_random_u32_below(interval) whenever delay >= interval, matching
igmp6_group_queried().

No Link: tags present in the commit header.

Only the changelog wording looks questionable to me; the code change itself
reads correctly, so the diff hunks are omitted.

> 1. If the timer has already expired (timer.expires <= jiffies),
>    ma->mca_work.timer.expires - jiffies underflows to a very large
>    unsigned long value (~ULONG_MAX), causing mod_delayed_work() to
>    schedule the report weeks/months into the future.

[Severity: Medium]
Can mod_delayed_work() really push the report weeks or months out in this
case?

The delayed work layer converts the delay straight back into an absolute
expiry in __queue_delayed_work():

kernel/workqueue.c:__queue_delayed_work() {
	...
	timer->expires = jiffies + delay;
	...
}

With delay == expires_old - jiffies_old, that modular arithmetic gives back
expires_old, i.e. the same already-past expiry rather than a near-ULONG_MAX
value.

__queue_delayed_work() then calls add_timer_global()/add_timer_on(), which
end in __mod_timer(timer, timer->expires, ...) and calc_wheel_index():

kernel/time/timer.c:calc_wheel_index() {
	...
	} else if ((long) delta < 0) {
		idx = clk & LVL_MASK;
		*bucket_expiry = clk;
	} else {
		/*
		 * Force expire obscene large timeouts to expire at the
		 * capacity limit of the wheel.
		 */
		if (delta >= WHEEL_TIMEOUT_CUTOFF)
			expires = clk + WHEEL_TIMEOUT_MAX;
	...
}

A past expiry takes the (long) delta < 0 branch and lands in the current
bucket, so it fires at the next tick; the WHEEL_TIMEOUT_CUTOFF capping
branch is not reachable for a delta that is negative when read as signed.
add_timer()'s kerneldoc says the same thing:

kernel/time/timer.c:add_timer() {
 * If @timer->expires is already in the past @timer will be queued to
 * expire at the next timer tick.
}

So for case 1, is the actual symptom that the randomized delay is lost and
the second unsolicited report goes out on the next tick, right behind the
first one, rather than a report deferred for weeks or months? The stated
symptom implies dropped membership reports and multicast traffic loss, which
reads differently for anyone triaging this for stable.

Case 2 (a delay inherited from a query with a large maximum response delay
exceeding unsolicited_report_interval()) matches the code, and the clamp
added here covers both cases either way. Could the first paragraph be
reworded to describe the lost randomization and the reuse of the stale
expiry instead?