Re: [net-next,v10,13/15] quic: add timer management

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
quic: add timer management

This patch introduces 'quic_timer' to unify and manage the five main
timers used in QUIC: loss detection, delayed ACK, path validation,
PMTU probing, and pacing. These timers are critical for driving
retransmissions, connection liveness, and flow control.

> diff --git a/net/quic/Makefile b/net/quic/Makefile
> index 58bb18f7926d..2ccf01ad9e22 100644
> --- a/net/quic/Makefile
> +++ b/net/quic/Makefile

[ ... ]

> diff --git a/net/quic/socket.c b/net/quic/socket.c
> index 74e3e3939c61..93a7abefc226 100644
> --- a/net/quic/socket.c
> +++ b/net/quic/socket.c

[ ... ]

> diff --git a/net/quic/socket.h b/net/quic/socket.h
> index d7811391cc8b..c5654fdc06b5 100644
> --- a/net/quic/socket.h
> +++ b/net/quic/socket.h

[ ... ]

> diff --git a/net/quic/timer.c b/net/quic/timer.c
> new file mode 100644
> index 000000000000..6f957385a341
> --- /dev/null
> +++ b/net/quic/timer.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* QUIC kernel implementation
> + * (C) Copyright Red Hat Corp. 2023
> + *
> + * This file is part of the QUIC kernel implementation
> + *
> + * Initialization/cleanup for QUIC protocol support.
> + *
> + * Written or modified by:
> + *    Xin Long <lucien.xin@gmail.com>
> + */
> +
> +#include "socket.h"

[ ... ]

> +static void quic_timer_sack_timeout(struct timer_list *t)
> +{
> +	struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_SACK].t);
> +	struct sock *sk = &qs->inet.sk;
> +
> +	bh_lock_sock(sk);
> +	if (sock_owned_by_user(sk)) {
> +		if (!test_and_set_bit(QUIC_SACK_DEFERRED, &sk->sk_tsq_flags))
> +			sock_hold(sk);
> +		goto out;
> +	}
> +
> +	quic_timer_sack_handler(sk);
> +out:
> +	bh_unlock_sock(sk);
> +	sock_put(sk);
> +}

[ ... ]

> +static void quic_timer_loss_timeout(struct timer_list *t)
> +{
> +	struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_LOSS].t);
> +	struct sock *sk = &qs->inet.sk;
> +
> +	bh_lock_sock(sk);
> +	if (sock_owned_by_user(sk)) {
> +		if (!test_and_set_bit(QUIC_LOSS_DEFERRED, &sk->sk_tsq_flags))
> +			sock_hold(sk);
> +		goto out;
> +	}
> +
> +	quic_timer_loss_handler(sk);
> +out:
> +	bh_unlock_sock(sk);
> +	sock_put(sk);
> +}

[ ... ]

> +static void quic_timer_path_timeout(struct timer_list *t)
> +{
> +	struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_PATH].t);
> +	struct sock *sk = &qs->inet.sk;
> +
> +	bh_lock_sock(sk);
> +	if (sock_owned_by_user(sk)) {
> +		if (!test_and_set_bit(QUIC_PATH_DEFERRED, &sk->sk_tsq_flags))
> +			sock_hold(sk);
> +		goto out;
> +	}
> +
> +	quic_timer_path_handler(sk);
> +out:
> +	bh_unlock_sock(sk);
> +	sock_put(sk);
> +}

[ ... ]

> +static void quic_timer_pmtu_timeout(struct timer_list *t)
> +{
> +	struct quic_sock *qs = container_of(t, struct quic_sock, timers[QUIC_TIMER_PMTU].t);
> +	struct sock *sk = &qs->inet.sk;
> +
> +	bh_lock_sock(sk);
> +	if (sock_owned_by_user(sk)) {
> +		if (!test_and_set_bit(QUIC_PMTU_DEFERRED, &sk->sk_tsq_flags))
> +			sock_hold(sk);
> +		goto out;
> +	}
> +
> +	quic_timer_pmtu_handler(sk);
> +out:
> +	bh_unlock_sock(sk);
> +	sock_put(sk);
> +}

This isn't a bug, but the four timer_list-based timeout callbacks
(quic_timer_sack_timeout, quic_timer_loss_timeout, quic_timer_path_timeout,
quic_timer_pmtu_timeout) are structurally identical.

Each extracts the sock via container_of, takes bh_lock_sock, checks
sock_owned_by_user to conditionally defer via test_and_set_bit/sock_hold,
otherwise calls the handler directly, then bh_unlock_sock/sock_put. They
differ only in the timer index constant, the deferred bit name, and the
handler function.

This pattern could be consolidated into a single parameterized timeout
callback using a data table that maps timer type to deferred bit and handler
function pointer, reducing around 60 lines of near-duplicate code to
roughly 15 lines. The TCP subsystem uses a similar approach for some of its
timer infrastructure.

That said, this follows the existing per-timer-callback pattern used
elsewhere in the kernel networking stack (for example, tcp_write_timer,
tcp_delack_timer are also separate functions), so this is a minor style
observation rather than a defect.

> +void quic_timer_reset(struct sock *sk, u8 type, u64 timeout)
> +{
> +	struct timer_list *t = quic_timer(sk, type);
> +
> +	if (timeout && !mod_timer(t, jiffies + usecs_to_jiffies(timeout)))
> +		sock_hold(sk);
> +}

Can this function handle QUIC_TIMER_PACE safely? It unconditionally casts
the result of quic_timer() to 'struct timer_list *' and calls mod_timer().

Looking at the timer API in this patch:

Both quic_timer_start() and quic_timer_stop() have explicit QUIC_TIMER_PACE
guards that dispatch to hrtimer APIs:

quic_timer_start():
	if (type == QUIC_TIMER_PACE) {
		hr = quic_timer(sk, type);
		if (!hrtimer_is_queued(hr)) {
			hrtimer_start(hr, ns_to_ktime(timeout), ...);
			sock_hold(sk);
		}
		return;
	}

quic_timer_stop():
	if (type == QUIC_TIMER_PACE) {
		if (hrtimer_try_to_cancel(quic_timer(sk, type)) == 1)
			sock_put(sk);
		return;
	}

However, quic_timer_reset() does not have a similar guard. If a future
caller invokes quic_timer_reset(sk, QUIC_TIMER_PACE, timeout), mod_timer()
will operate on memory that is actually a 'struct hrtimer', potentially
causing memory corruption.

Should quic_timer_reset() either add a QUIC_TIMER_PACE guard consistent with
quic_timer_start()/quic_timer_stop(), or document that this function must
only be used with timer_list-based timers?

> +void quic_timer_start(struct sock *sk, u8 type, u64 timeout)
> +{
> +	struct timer_list *t;
> +	struct hrtimer *hr;
> +
> +	if (type == QUIC_TIMER_PACE) {
> +		hr = quic_timer(sk, type);
> +
> +		if (!hrtimer_is_queued(hr)) {
> +			hrtimer_start(hr, ns_to_ktime(timeout), HRTIMER_MODE_ABS_PINNED_SOFT);
> +			sock_hold(sk);
> +		}
> +		return;
> +	}
> +
> +	t = quic_timer(sk, type);
> +	if (timeout && !timer_pending(t)) {
> +		if (!mod_timer(t, jiffies + usecs_to_jiffies(timeout)))
> +			sock_hold(sk);
> +	}
> +}
> +
> +void quic_timer_stop(struct sock *sk, u8 type)
> +{
> +	if (type == QUIC_TIMER_PACE) {
> +		if (hrtimer_try_to_cancel(quic_timer(sk, type)) == 1)
> +			sock_put(sk);
> +		return;
> +	}
> +	if (timer_delete(quic_timer(sk, type)))
> +		sock_put(sk);
> +}

[ ... ]

> diff --git a/net/quic/timer.h b/net/quic/timer.h
> new file mode 100644
> index 000000000000..61b094325334
> --- /dev/null
> +++ b/net/quic/timer.h

[ ... ]