netdev - Re: [PATCH] tcp: Add TCP_INFO counter for packets received out-of-order

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJ5wANqhpR28y5AYf6GTBgzTau+u0N0ogG690C71LbxaA@mail.gmail.com>
Date:   Mon, 9 Sep 2019 17:01:46 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Thomas Higdon <tph@...com>
Cc:     netdev <netdev@...r.kernel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Dave Jones <dsj@...com>
Subject: Re: [PATCH] tcp: Add TCP_INFO counter for packets received out-of-order

On Mon, Sep 9, 2019 at 4:30 PM Thomas Higdon <tph@...com> wrote:
>
> For receive-heavy cases on the server-side, we want to track the
> connection quality for individual client IPs. This counter, similar to
> the existing system-wide TCPOFOQueue counter in /proc/net/netstat,
> tracks out-of-order packet reception. By providing this counter in
> TCP_INFO, it will allow understanding to what degree receive-heavy
> sockets are experiencing out-of-order delivery and packet drops
> indicating congestion.
>
> Please note that this is similar to the counter in NetBSD TCP_INFO, and
> has the same name.
>
> Signed-off-by: Thomas Higdon <tph@...com>
> ---
>  include/linux/tcp.h      | 2 ++
>  include/uapi/linux/tcp.h | 2 ++
>  net/ipv4/tcp.c           | 1 +
>  net/ipv4/tcp_input.c     | 1 +
>  4 files changed, 6 insertions(+)
>
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index f3a85a7fb4b1..a01dc78218f1 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -393,6 +393,8 @@ struct tcp_sock {
>          */
>         struct request_sock *fastopen_rsk;
>         u32     *saved_syn;
> +
> +       u32 rcv_ooopack; /* Received out-of-order packets, for tcpinfo */
>  };
>
>  enum tsq_enum {
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index b3564f85a762..20237987ccc8 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -270,6 +270,8 @@ struct tcp_info {
>         __u64   tcpi_bytes_retrans;  /* RFC4898 tcpEStatsPerfOctetsRetrans */
>         __u32   tcpi_dsack_dups;     /* RFC4898 tcpEStatsStackDSACKDups */
>         __u32   tcpi_reord_seen;     /* reordering events seen */
> +
> +       __u32   tcpi_rcv_ooopack;    /* Out-of-order packets received */

This is problematic : you create a 32bit hole in this structure that
we will never be able to fill.

We need to add another metric here so that the whole 64bit space is used.

>  };
>
>  /* netlink attributes types for SCM_TIMESTAMPING_OPT_STATS */
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 94df48bcecc2..d4386f054f18 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3295,6 +3295,7 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info)
>         info->tcpi_bytes_retrans = tp->bytes_retrans;
>         info->tcpi_dsack_dups = tp->dsack_dups;
>         info->tcpi_reord_seen = tp->reord_seen;
> +       info->tcpi_rcv_ooopack = tp->rcv_ooopack;
>         unlock_sock_fast(sk, slow);
>  }
>  EXPORT_SYMBOL_GPL(tcp_get_info);
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 706cbb3b2986..2774680c5d05 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4555,6 +4555,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
>         tp->pred_flags = 0;
>         inet_csk_schedule_ack(sk);
>
> +       tp->rcv_ooopack++;

We count skbs or we count segments ?

(GRO might have aggregated multiple segments)


>         NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFOQUEUE);
>         seq = TCP_SKB_CB(skb)->seq;
>         end_seq = TCP_SKB_CB(skb)->end_seq;


You forgot to clear the field in tcp_disconnect()