Re: [PATCH v3] net/sched: act_gate: Limit the max value for cycletime

[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.
---
net/sched: act_gate: Limit the max value for cycletime

This changes the act_gate netlink policy so that TCA_GATE_CYCLE_TIME is
validated with NLA_POLICY_FULL_RANGE_SIGNED(NLA_S64) against a range of
0 to S64_MAX, with the intent of preventing an s64 overflow in
gate_timer_func() that led to a soft lockup reported by syzbot.

Closes: https://syzkaller.appspot.com/bug?extid=0054fed3dc9085390f51

A couple of questions on the changelog and on the diff below.

> Modify the cycletime range in the policy to (0, S64_MAX), when parsing
> cycletime, ensuring its value does not exceed S64_MAX guarantees that the
> hrtimer can correctly calculate a valid expiry time.

[Severity: Low]
Should this read [0, S64_MAX] instead of (0, S64_MAX)?

gate_cycle_time_range uses .min = 0 and the range check in
nla_validate_int_range_signed() is inclusive, so a cycletime of 0 is still
accepted, and it keeps its special meaning in tcf_gate_init():

	if (!cycletime) {
		struct tcfg_gate_entry *entry;
		ktime_t cycle = 0;

		list_for_each_entry(entry, &p->entries, list)
			cycle = ktime_add_ns(cycle, entry->interval);
		cycletime = cycle;
	}

As written, the open interval wording suggests 0 is now rejected, which a
backporter might rely on.

> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index fdbfcaa3e2ab9..4c4a0f80dec1f 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> @@ -166,13 +166,19 @@ static const struct nla_policy entry_policy[TCA_GATE_ENTRY_MAX + 1] = {
>  	[TCA_GATE_ENTRY_MAX_OCTETS]	= { .type = NLA_S32 },
>  };
>  
> +static const struct netlink_range_validation_signed gate_cycle_time_range = {
> +	.min = 0,
> +	.max = S64_MAX,
> +};
> +
>  static const struct nla_policy gate_policy[TCA_GATE_MAX + 1] = {
>  	[TCA_GATE_PARMS]		=
>  		NLA_POLICY_EXACT_LEN(sizeof(struct tc_gate)),
>  	[TCA_GATE_PRIORITY]		= { .type = NLA_S32 },
>  	[TCA_GATE_ENTRY_LIST]		= { .type = NLA_NESTED },
>  	[TCA_GATE_BASE_TIME]		= { .type = NLA_U64 },
> -	[TCA_GATE_CYCLE_TIME]		= { .type = NLA_U64 },
> +	[TCA_GATE_CYCLE_TIME]		=
> +		NLA_POLICY_FULL_RANGE_SIGNED(NLA_S64, &gate_cycle_time_range),

[Severity: High]
Does capping cycletime at S64_MAX actually remove the soft lockup?

The expiry is not computed from cycletime alone. In gate_get_start_time():

	base = ns_to_ktime(param->tcfg_basetime);
	now = gate_get_time(gact);

	if (ktime_after(base, now)) {
		*start = base;
		return;
	}

	cycle = param->tcfg_cycletime;

	n = div64_u64(ktime_sub_ns(now, base), cycle);
	*start = ktime_add_ns(base, (n + 1) * cycle);

TCA_GATE_BASE_TIME is left as { .type = NLA_U64 } with no range, so with
basetime = 1 and cycletime = S64_MAX (both accepted by the new policy)
n becomes 0 and the addition is 1 + (2^63 - 1), which wraps to S64_MIN
since ktime_add_ns() is plain 64-bit addition.

gate_start_timer() only clamps downward:

	start = min_t(ktime_t, start, expires);

	hrtimer_start(&gact->hitimer, start, HRTIMER_MODE_ABS_SOFT);

so the wrapped negative value is armed as an absolute expiry in the past.
The catch-up branch of gate_timer_func() then recomputes the same value on
every invocation:

	if (ktime_after(now, close_time)) {
		ktime_t cycle, base;
		u64 n;

		cycle = p->tcfg_cycletime;
		base = ns_to_ktime(p->tcfg_basetime);
		n = div64_u64(ktime_sub_ns(now, base), cycle);
		close_time = ktime_add_ns(base, (n + 1) * cycle);
	}
	...
	hrtimer_set_expires(&gact->hitimer, close_time);
	...
	return HRTIMER_RESTART;

which looks like the same "gate_timer_func in hrtimer_run_softirq" pattern
from the report, reachable via RTM_NEWACTION with basetime = 1,
cycletime = S64_MAX and one entry with a non-zero interval. A wide band of
policy-legal pairs wraps the same way, for example cycletime = S64_MAX - 10
with basetime = 10^12.

Would bounding the whole base + (n + 1) * cycle arithmetic be needed here,
the way sch_taprio.c does it?

	static const struct netlink_range_validation_signed taprio_cycle_time_range = {
		.min = 0,
		.max = INT_MAX,
	};
-- 
pw-bot: cr