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

On Tue, Mar 3, 2026 at 3:33=E2=80=AFAM Paolo Abeni <[email protected]> wrot=
e:
>
> 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-loca=
l.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 <[email protected]>
> > + */
> > +
> > +#include "socket.h"
>
> [ ... ]
>
> > +static void quic_timer_sack_timeout(struct timer_list *t)
> > +{
> > +     struct quic_sock *qs =3D container_of(t, struct quic_sock, timers=
[QUIC_TIMER_SACK].t);
> > +     struct sock *sk =3D &qs->inet.sk;
> > +
> > +     bh_lock_sock(sk);
> > +     if (sock_owned_by_user(sk)) {
> > +             if (!test_and_set_bit(QUIC_SACK_DEFERRED, &sk->sk_tsq_fla=
gs))
> > +                     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 =3D container_of(t, struct quic_sock, timers=
[QUIC_TIMER_LOSS].t);
> > +     struct sock *sk =3D &qs->inet.sk;
> > +
> > +     bh_lock_sock(sk);
> > +     if (sock_owned_by_user(sk)) {
> > +             if (!test_and_set_bit(QUIC_LOSS_DEFERRED, &sk->sk_tsq_fla=
gs))
> > +                     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 =3D container_of(t, struct quic_sock, timers=
[QUIC_TIMER_PATH].t);
> > +     struct sock *sk =3D &qs->inet.sk;
> > +
> > +     bh_lock_sock(sk);
> > +     if (sock_owned_by_user(sk)) {
> > +             if (!test_and_set_bit(QUIC_PATH_DEFERRED, &sk->sk_tsq_fla=
gs))
> > +                     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 =3D container_of(t, struct quic_sock, timers=
[QUIC_TIMER_PMTU].t);
> > +     struct sock *sk =3D &qs->inet.sk;
> > +
> > +     bh_lock_sock(sk);
> > +     if (sock_owned_by_user(sk)) {
> > +             if (!test_and_set_bit(QUIC_PMTU_DEFERRED, &sk->sk_tsq_fla=
gs))
> > +                     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_timeou=
t,
> 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 hand=
ler
> function pointer, reducing around 60 lines of near-duplicate code to
> roughly 15 lines. The TCP subsystem uses a similar approach for some of i=
ts
> 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.
>
This is a good one. May not add a data table, but a simple helper like this=
:

static void quic_timer_timeout(struct timer_list *t, int type, int defer,
                               void (*handler)(struct sock *sk))
{
        struct quic_sock *qs =3D container_of(t, struct quic_sock,
timers[type].t);
        struct sock *sk =3D &qs->inet.sk;

        bh_lock_sock(sk);
        if (sock_owned_by_user(sk)) {
                if (!test_and_set_bit(defer, &sk->sk_tsq_flags))
                        sock_hold(sk);
                goto out;
        }

        handler(sk);
out:
        bh_unlock_sock(sk);
        sock_put(sk);
}

will reduce quite some dup code.

> > +void quic_timer_reset(struct sock *sk, u8 type, u64 timeout)
> > +{
> > +     struct timer_list *t =3D 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_PA=
CE
> guards that dispatch to hrtimer APIs:
>
> quic_timer_start():
>         if (type =3D=3D QUIC_TIMER_PACE) {
>                 hr =3D 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 =3D=3D QUIC_TIMER_PACE) {
>                 if (hrtimer_try_to_cancel(quic_timer(sk, type)) =3D=3D 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 w=
ith
> quic_timer_start()/quic_timer_stop(), or document that this function must
> only be used with timer_list-based timers?
>
There will be no callers to quic_timer_reset(QUIC_TIMER_PACE), so no need
to add a check for QUIC_TIMER_PACE, I will leave a comment there for this.

Thanks.

> > +void quic_timer_start(struct sock *sk, u8 type, u64 timeout)
> > +{
> > +     struct timer_list *t;
> > +     struct hrtimer *hr;
> > +
> > +     if (type =3D=3D QUIC_TIMER_PACE) {
> > +             hr =3D quic_timer(sk, type);
> > +
> > +             if (!hrtimer_is_queued(hr)) {
> > +                     hrtimer_start(hr, ns_to_ktime(timeout), HRTIMER_M=
ODE_ABS_PINNED_SOFT);
> > +                     sock_hold(sk);
> > +             }
> > +             return;
> > +     }
> > +
> > +     t =3D 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 =3D=3D QUIC_TIMER_PACE) {
> > +             if (hrtimer_try_to_cancel(quic_timer(sk, type)) =3D=3D 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
>
> [ ... ]
>