Re: [PATCH net v2 1/2] net/sched: taprio: reject software schedules that overflow their timestamps

[Vinicius Costa Gomes]

Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx> writes:

> taprio takes base-time as an unbounded signed 64-bit value. A base time
> in the future is used as the schedule start unchanged, and
> setup_first_end_time() then adds the cycle time, the first interval and
> the gate durations of the first entry to it. With a start close to
> KTIME_MAX these sums overflow, and the software schedule starts with end
> and gate close times that lie far in the past.
>
> If this is the first schedule, the qdisc timer is armed for its future
> start. With an operational schedule running, taprio_start_sched() keeps
> the earlier operational expiry instead. A large cycle-time-extension
> can then trigger an early handover to the pending admin schedule.
> advance_sched() uses the invalid entry end as the next expiry and can
> keep restarting inside the same timer interrupt.
>
> Before a software schedule is initialized and published, check that the
> computed start is not negative and leaves room for every timestamp
> initialized from it, and reject the schedule with -ERANGE otherwise. Full
> offload and txtime-assist do not use the software timer and are not
> affected. Schedules with a reasonable base time behave as before.
>
> Fixes: 5a781ccbd19e ("tc: Add support for configuring the taprio scheduler")
> Assisted-by: Codex:gpt-6.1-sol
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Krystian Kaniewski <krystianmkaniewski@xxxxxxxxx>
> ---
> v2: no change
> v1: https://lore.kernel.org/all/20261006113335.241564-2-krystianmkaniewski@xxxxxxxxx/
>
>  net/sched/sch_taprio.c | 46 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 299234a5f0fe6..1f753911cdfec 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1238,6 +1238,48 @@ static int taprio_get_start_time(struct Qdisc *sch,
>  	return 0;
>  }
>  
> +static int taprio_validate_start_time(struct taprio_sched *q,
> +				      const struct sched_gate_list *sched,
> +				      ktime_t start,
> +				      struct netlink_ext_ack *extack)
> +{
> +	int num_tc = netdev_get_num_tc(qdisc_dev(q->root));
> +	const struct sched_entry *first, *entry;
> +	u64 offset = 0, span;
> +	int tc;
> +
> +	if (TXTIME_ASSIST_IS_ENABLED(q->flags) ||
> +	    FULL_OFFLOAD_IS_ENABLED(q->flags))
> +		return 0;
> +
> +	first = list_first_entry(&sched->entries, struct sched_entry, list);
> +
> +	/* setup_first_end_time() adds the cycle time, the first interval and
> +	 * the finite gate durations of the first entry to the start, and
> +	 * setup_txtime() adds the offset of every entry. None of these sums
> +	 * may overflow.
> +	 */

nitpick: In the commit message there's "Full offload and txtime-assist
do not use the software timer and are not affected", and here you say
something about setup_txtime(), I think it's better if you don't mention
anything about those other "modes" here.

> +	span = max_t(u64, sched->cycle_time, first->interval);
> +	list_for_each_entry(entry, &sched->entries, list) {
> +		span = max(span, offset);
> +		offset += entry->interval;
> +	}
> +
> +	for (tc = 0; tc < num_tc; tc++) {
> +		if (first->gate_duration[tc] == sched->cycle_time)
> +			continue;
> +		span = max(span, first->gate_duration[tc]);
> +	}
> +
> +	if (start < 0 || span > KTIME_MAX ||
> +	    (u64)start > KTIME_MAX - span) {
> +		NL_SET_ERR_MSG(extack, "Schedule timing is out of range");
> +		return -ERANGE;
> +	}
> +
> +	return 0;
> +}
> +
>  static void setup_first_end_time(struct taprio_sched *q,
>  				 struct sched_gate_list *sched, ktime_t base)
>  {
> @@ -1958,6 +2000,10 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>  		goto unlock;
>  	}
>  
> +	err = taprio_validate_start_time(q, new_admin, start, extack);
> +	if (err)
> +		goto unlock;
> +
>  	setup_txtime(q, new_admin, start);
>  
>  	if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
> -- 
> 2.53.0
>


Cheers,
-- 
Vinicius