Re: [PATCH net v2] tcp: check skb is non-NULL in tcp_rto_delta_us()

[Josh Hunt]

On 9/8/24 10:50 AM, Neal Cardwell wrote:
> Since this is targeted to the net branch to fix crashes at least as
> far back as 5.4, AFAICT it would be good to have a Fixes: footer, so
> maintainers know how far back in stable release history to apply the
> fix.
>
> I'd suggest pointing to this linux/v4.13 commit:
>
> Fixes: e1a10ef7fa87 ("tcp: introduce tcp_rto_delta_us() helper for
> xmit timer fix")
>
> The bug actually predates that commit (the code before that already
> assumed tcp_write_queue_head() was non-NULL in tcp_rearm_rto() if
> packets_out is non-zero). But that commit is the first point at which
> tcp_rto_delta_us() exists as a function and so it's straightforward to
> apply the patch (albeit with some conflicts in earlier kernels). And
> that commit is far enough back to imply that the fix should be
> backported to all "longterm" releases listed at

Thanks Neal. I'll add this fixes tag.

> IMHO it would be nice to have the WARN_ONCE print more information, to
> help debug these cases. This seems like some sort of packet counting
> bug, so IMHO it would be nice to have more information about packet
> counts and MTU/MSS (since MTU/MSS changes force recalculation of
> packet counts for skbs and the scoreboard, and have thus been a
> traditional source of packet-counting bugs). Perhaps something like
> the following (compiled but not tested):
>
> +               WARN_ONCE(1,
> +                         "rtx queue empty: "
> +                         "out:%u sacked:%u lost:%u retrans:%u "
> +                         "tlp_high_seq:%u sk_state:%u ca_state:%u "
> +                         "advmss:%u mss_cache:%u pmtu:%u\n",
> +                         tcp_sk(sk)->packets_out, tcp_sk(sk)->sacked_out,
> +                         tcp_sk(sk)->lost_out, tcp_sk(sk)->retrans_out,
> +                         tcp_sk(sk)->tlp_high_seq, sk->sk_state,
> +                         inet_csk(sk)->icsk_ca_state,
> +                         tcp_sk(sk)->advmss, tcp_sk(sk)->mss_cache,
> +                         inet_csk(sk)->icsk_pmtu_cookie);

Makes sense. I agree more info to help debug is better. I'll review the 
suggested additions and spin a v3.

Thanks!
Josh