Message ID | 20221124112229.789975-1-leitao@debian.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp3335975wrr; Thu, 24 Nov 2022 03:26:12 -0800 (PST) X-Google-Smtp-Source: AA0mqf7gCXjU3wPa9MOF4jGGVmtKBuck5x9YGUlWOby63gunlg51dU+2nfh4qKkRTYJfd6J5jwmG X-Received: by 2002:a17:903:22c4:b0:187:4ace:e1fd with SMTP id y4-20020a17090322c400b001874acee1fdmr13522433plg.54.1669289172304; Thu, 24 Nov 2022 03:26:12 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669289172; cv=none; d=google.com; s=arc-20160816; b=nHUSyzFiyg5QwhDZLm5DPT+4b/CqjvVXMOAaSzj9IREGEJ0YtXL4xVq9zuxOvAs/YP +qbWG2sXqJT2LmOs0qj8HoXYV78DhTeGv7i9/QZWdCmEa44UagJcdN25eMF+IZWOZB/r DhxFvg9AFQlxYCTHs7hkWb2v6+XfhiDRlFo6bXPkdlYULpLhyCd4OhsYwwg+5CA92YMR CsS/NCH/r9dOY8KuJK/Qh0crmJQ9szJDTBsDxqWww8BrMqYaZp6Tjs+bv2DDX2alsFKH l9eI+YEblEhtIC2Y8Xeqw+EvHkodNdu/HX9l57u4qM1kc6HQsDv4sWiqIEYtZJXFK7KW SN6g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=TQJAcBaQ4iWBQtMTkg02XF1PTNjJUIHH8kGGgI0Jy94=; b=jtqvbqJgoLfebHFkDclQqa6ASifPP5GFp3VRY+i1wArR+4srLdd8NZHBU79WVHLbiH gTp35v3g4TsQQNk1fTStZ/jWcSDkxrJV4TuQDrIvsvZvgl3nZpNrcqOmcaxzIQ7gbd9+ DAnqKFCDX+LexbHnQq9/0QcAK2+4XTDgfaqL+C2cSgLCtUzwJ+dC9etlSIhBByslRdQa 5tnhqQc2tVvUvfQwEZ8pgDXdMQHv6+8f9olIxE+ro/8ik+iKs2XXYUP2/V6Q7vgtzBWr jorSoOlKAr9LDVcL0GLCI2gOEGAAC9JO7Nen5SrHrNrkO/hi1PO0kH7G9emBw0h+BVR0 PHzQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s135-20020a632c8d000000b00477b29bb008si1044860pgs.631.2022.11.24.03.25.59; Thu, 24 Nov 2022 03:26:12 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229666AbiKXLWs (ORCPT <rfc822;fengqi706@gmail.com> + 99 others); Thu, 24 Nov 2022 06:22:48 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54134 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229606AbiKXLWo (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 24 Nov 2022 06:22:44 -0500 Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4EAA011478; Thu, 24 Nov 2022 03:22:42 -0800 (PST) Received: by mail-ej1-f52.google.com with SMTP id e27so3416616ejc.12; Thu, 24 Nov 2022 03:22:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=TQJAcBaQ4iWBQtMTkg02XF1PTNjJUIHH8kGGgI0Jy94=; b=I1mxj9FyD0I1zXVD4592+OKpvxOiaMFhOAkNCetf1ZXYlMEa1foyN2p/cDGypaacMM iXBoEjbGbDhE9g/65VJjxABVO94M2+vZ2kwhz96xnkBDlN/zvbTSNec/zQto7Aqdoa7i TDrinTWdc7JIEV07F1tfp9ZdER5w3XBmVPw22+eO1UGWnuzBHfsvOarD/8FQwTd67i+n yI+OifTARGIKWtV4/KC+2bE6p+3N78STIc2y9AKR0jT2CdepGd2pvJLP4LtY98ZZ7XK7 4Z3+cYJ2V2nDzzyT6/7D0+0INBbZsEPizEFuNFLzyIRa8lbdvlTV7miprzwjTHwN4jpE 8GSQ== X-Gm-Message-State: ANoB5pm5THL5p6jd626az8yhCTZDsdFl1MKvgNLt+u3mOCNSPSKbaDm9 WOofMyBtmnXwlQp91T1IFo8= X-Received: by 2002:a17:906:e2cb:b0:7ad:c35a:ad76 with SMTP id gr11-20020a170906e2cb00b007adc35aad76mr27578205ejb.705.1669288960912; Thu, 24 Nov 2022 03:22:40 -0800 (PST) Received: from localhost (fwdproxy-cln-017.fbsv.net. [2a03:2880:31ff:11::face:b00c]) by smtp.gmail.com with ESMTPSA id e19-20020a170906315300b007803083a36asm318771eje.115.2022.11.24.03.22.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 24 Nov 2022 03:22:40 -0800 (PST) From: Breno Leitao <leitao@debian.org> To: edumazet@google.com, davem@davemloft.net, kuba@kernel.org Cc: netdev@vger.kernel.org, leit@fb.com, yoshfuji@linux-ipv6.org, pabeni@redhat.com, dsahern@kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH RESEND net-next] tcp: socket-specific version of WARN_ON_ONCE() Date: Thu, 24 Nov 2022 03:22:29 -0800 Message-Id: <20221124112229.789975-1-leitao@debian.org> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.4 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1750376562843456044?= X-GMAIL-MSGID: =?utf-8?q?1750376562843456044?= |
Series |
[RESEND,net-next] tcp: socket-specific version of WARN_ON_ONCE()
|
|
Commit Message
Breno Leitao
Nov. 24, 2022, 11:22 a.m. UTC
There are cases where we need information about the socket during a
warning, so, it could help us to find bugs that happens and do not have
an easy repro.
This diff creates a TCP socket-specific version of WARN_ON_ONCE(), which
dumps more information about the TCP socket.
This new warning is not only useful to give more insight about kernel bugs, but,
it is also helpful to expose information that might be coming from buggy
BPF applications, such as BPF applications that sets invalid
tcp_sock->snd_cwnd values.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
include/net/tcp.h | 3 ++-
include/net/tcp_debug.h | 10 ++++++++++
net/ipv4/tcp.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 1 deletion(-)
create mode 100644 include/net/tcp_debug.h
Comments
From: Breno Leitao <leitao@debian.org> Date: Thu, 24 Nov 2022 03:22:29 -0800 > There are cases where we need information about the socket during a > warning, so, it could help us to find bugs that happens and do not have > an easy repro. > > This diff creates a TCP socket-specific version of WARN_ON_ONCE(), which > dumps more information about the TCP socket. > > This new warning is not only useful to give more insight about kernel bugs, but, > it is also helpful to expose information that might be coming from buggy > BPF applications, such as BPF applications that sets invalid > tcp_sock->snd_cwnd values. Have you finally found a root cause on BPF or TCP side ? > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > include/net/tcp.h | 3 ++- > include/net/tcp_debug.h | 10 ++++++++++ > net/ipv4/tcp.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 42 insertions(+), 1 deletion(-) > create mode 100644 include/net/tcp_debug.h > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 14d45661a84d..e490af8e6fdc 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -40,6 +40,7 @@ > #include <net/inet_ecn.h> > #include <net/dst.h> > #include <net/mptcp.h> > +#include <net/tcp_debug.h> > > #include <linux/seq_file.h> > #include <linux/memcontrol.h> > @@ -1229,7 +1230,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp) > > static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val) > { > - WARN_ON_ONCE((int)val <= 0); > + TCP_SOCK_WARN_ON_ONCE(tp, (int)val <= 0); > tp->snd_cwnd = val; > } > > diff --git a/include/net/tcp_debug.h b/include/net/tcp_debug.h > new file mode 100644 > index 000000000000..50e96d87d335 > --- /dev/null > +++ b/include/net/tcp_debug.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_TCP_DEBUG_H > +#define _LINUX_TCP_DEBUG_H > + > +void tcp_sock_warn(const struct tcp_sock *tp); > + > +#define TCP_SOCK_WARN_ON_ONCE(tcp_sock, condition) \ > + DO_ONCE_LITE_IF(condition, tcp_sock_warn, tcp_sock) > + > +#endif /* _LINUX_TCP_DEBUG_H */ > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 54836a6b81d6..dd682f60c7cb 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) > } > EXPORT_SYMBOL_GPL(tcp_abort); > > +void tcp_sock_warn(const struct tcp_sock *tp) > +{ > + const struct sock *sk = (const struct sock *)tp; > + struct inet_sock *inet = inet_sk(sk); > + struct inet_connection_sock *icsk = inet_csk(sk); > + > + WARN_ON(1); > + > + if (!tp) Is this needed ? > + return; > + > + pr_warn("Socket Info: family=%u state=%d sport=%u dport=%u ccname=%s cwnd=%u", > + sk->sk_family, sk->sk_state, ntohs(inet->inet_sport), > + ntohs(inet->inet_dport), icsk->icsk_ca_ops->name, tcp_snd_cwnd(tp)); > + > + switch (sk->sk_family) { > + case AF_INET: > + pr_warn("saddr=%pI4 daddr=%pI4", &inet->inet_saddr, > + &inet->inet_daddr); As with tcp_syn_flood_action(), [address]:port format is easy to read and consistent in kernel ? > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + pr_warn("saddr=%pI6 daddr=%pI6", &sk->sk_v6_rcv_saddr, > + &sk->sk_v6_daddr); > + break; > +#endif > + } > +} > +EXPORT_SYMBOL_GPL(tcp_sock_warn); > + > extern struct tcp_congestion_ops tcp_reno; > > static __initdata unsigned long thash_entries; > -- > 2.30.2
Hello, On Thu, 2022-11-24 at 03:22 -0800, Breno Leitao wrote: > There are cases where we need information about the socket during a > warning, so, it could help us to find bugs that happens and do not have > an easy repro. > > This diff creates a TCP socket-specific version of WARN_ON_ONCE(), which > dumps more information about the TCP socket. > > This new warning is not only useful to give more insight about kernel bugs, but, > it is also helpful to expose information that might be coming from buggy > BPF applications, such as BPF applications that sets invalid > tcp_sock->snd_cwnd values. I personally find this use-case a little too tight, you could likelly fetch the same information with a perf probe or something similar. > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > include/net/tcp.h | 3 ++- > include/net/tcp_debug.h | 10 ++++++++++ > net/ipv4/tcp.c | 30 ++++++++++++++++++++++++++++++ > 3 files changed, 42 insertions(+), 1 deletion(-) > create mode 100644 include/net/tcp_debug.h > > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 14d45661a84d..e490af8e6fdc 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -40,6 +40,7 @@ > #include <net/inet_ecn.h> > #include <net/dst.h> > #include <net/mptcp.h> > +#include <net/tcp_debug.h> > > #include <linux/seq_file.h> > #include <linux/memcontrol.h> > @@ -1229,7 +1230,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp) > > static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val) > { > - WARN_ON_ONCE((int)val <= 0); > + TCP_SOCK_WARN_ON_ONCE(tp, (int)val <= 0); > tp->snd_cwnd = val; > } > > diff --git a/include/net/tcp_debug.h b/include/net/tcp_debug.h > new file mode 100644 > index 000000000000..50e96d87d335 > --- /dev/null > +++ b/include/net/tcp_debug.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _LINUX_TCP_DEBUG_H > +#define _LINUX_TCP_DEBUG_H > + > +void tcp_sock_warn(const struct tcp_sock *tp); > + > +#define TCP_SOCK_WARN_ON_ONCE(tcp_sock, condition) \ > + DO_ONCE_LITE_IF(condition, tcp_sock_warn, tcp_sock) > + > +#endif /* _LINUX_TCP_DEBUG_H */ > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 54836a6b81d6..dd682f60c7cb 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) > } > EXPORT_SYMBOL_GPL(tcp_abort); > > +void tcp_sock_warn(const struct tcp_sock *tp) > +{ > + const struct sock *sk = (const struct sock *)tp; > + struct inet_sock *inet = inet_sk(sk); > + struct inet_connection_sock *icsk = inet_csk(sk); > + > + WARN_ON(1); > + > + if (!tp) > + return; > + > + pr_warn("Socket Info: family=%u state=%d sport=%u dport=%u ccname=%s cwnd=%u", > + sk->sk_family, sk->sk_state, ntohs(inet->inet_sport), > + ntohs(inet->inet_dport), icsk->icsk_ca_ops->name, tcp_snd_cwnd(tp)); > + > + switch (sk->sk_family) { > + case AF_INET: > + pr_warn("saddr=%pI4 daddr=%pI4", &inet->inet_saddr, > + &inet->inet_daddr); > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + pr_warn("saddr=%pI6 daddr=%pI6", &sk->sk_v6_rcv_saddr, > + &sk->sk_v6_daddr); > + break; > +#endif Please, adjust the output format as suggested by Kuniyuki, thanks! Paolo
On Tue, Nov 29, 2022 at 10:00:55AM +0900, Kuniyuki Iwashima wrote: > From: Breno Leitao <leitao@debian.org> > Date: Thu, 24 Nov 2022 03:22:29 -0800 > > There are cases where we need information about the socket during a > > warning, so, it could help us to find bugs that happens and do not have > > an easy repro. > > > > This diff creates a TCP socket-specific version of WARN_ON_ONCE(), which > > dumps more information about the TCP socket. > > > > This new warning is not only useful to give more insight about kernel bugs, but, > > it is also helpful to expose information that might be coming from buggy > > BPF applications, such as BPF applications that sets invalid > > tcp_sock->snd_cwnd values. > > Have you finally found a root cause on BPF or TCP side ? Yes, this demonstrated to be very useful to find out BPF applications that are doing nasty things with the congestion window. We currently have this patch applied to Meta's infrastructure to track BPF applications that are misbehaving, and easily track down to which BPF application is the responsible one. > > +#endif /* _LINUX_TCP_DEBUG_H */ > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index 54836a6b81d6..dd682f60c7cb 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) > > } > > EXPORT_SYMBOL_GPL(tcp_abort); > > > > +void tcp_sock_warn(const struct tcp_sock *tp) > > +{ > > + const struct sock *sk = (const struct sock *)tp; > > + struct inet_sock *inet = inet_sk(sk); > > + struct inet_connection_sock *icsk = inet_csk(sk); > > + > > + WARN_ON(1); > > + > > + if (!tp) > > Is this needed ? We are de-referencing tp/sk in the lines below, so, I think it is safe to check if they are not NULL before the de-refencing it. Should I do check for "ck" instead of "tp" to make the code a bit cleaner to read? > > + pr_warn("Socket Info: family=%u state=%d sport=%u dport=%u ccname=%s cwnd=%u", > > + sk->sk_family, sk->sk_state, ntohs(inet->inet_sport), > > + ntohs(inet->inet_dport), icsk->icsk_ca_ops->name, tcp_snd_cwnd(tp)); > > + > > + switch (sk->sk_family) { > > + case AF_INET: > > + pr_warn("saddr=%pI4 daddr=%pI4", &inet->inet_saddr, > > + &inet->inet_daddr); > > As with tcp_syn_flood_action(), [address]:port format is easy > to read and consistent in kernel ? Absolutely. I am going to fix it in v2. Thanks!
> On Nov 29, 2022, at 21:48, Breno Leitao <leitao@debian.org> wrote: >> On Tue, Nov 29, 2022 at 10:00:55AM +0900, Kuniyuki Iwashima wrote: >> From: Breno Leitao <leitao@debian.org> >> Date: Thu, 24 Nov 2022 03:22:29 -0800 >>> There are cases where we need information about the socket during a >>> warning, so, it could help us to find bugs that happens and do not have >>> an easy repro. >>> >>> This diff creates a TCP socket-specific version of WARN_ON_ONCE(), which >>> dumps more information about the TCP socket. >>> >>> This new warning is not only useful to give more insight about kernel bugs, but, >>> it is also helpful to expose information that might be coming from buggy >>> BPF applications, such as BPF applications that sets invalid >>> tcp_sock->snd_cwnd values. >> >> Have you finally found a root cause on BPF or TCP side ? > > Yes, this demonstrated to be very useful to find out BPF applications > that are doing nasty things with the congestion window. > > We currently have this patch applied to Meta's infrastructure to track > BPF applications that are misbehaving, and easily track down to which > BPF application is the responsible one. If you have a fix merged on the BPF side, it would be helpful to mention the commit to well understand the issue, background, and why other tooling is not enough as Paolo wondered. >>> +#endif /* _LINUX_TCP_DEBUG_H */ >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >>> index 54836a6b81d6..dd682f60c7cb 100644 >>> --- a/net/ipv4/tcp.c >>> +++ b/net/ipv4/tcp.c >>> @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) >>> } >>> EXPORT_SYMBOL_GPL(tcp_abort); >>> >>> +void tcp_sock_warn(const struct tcp_sock *tp) >>> +{ >>> + const struct sock *sk = (const struct sock *)tp; >>> + struct inet_sock *inet = inet_sk(sk); >>> + struct inet_connection_sock *icsk = inet_csk(sk); >>> + >>> + WARN_ON(1); >>> + >>> + if (!tp) >> >> Is this needed ? > > We are de-referencing tp/sk in the lines below, so, I think it is safe to > check if they are not NULL before the de-refencing it. tp->snd_cwnd is accessed just after this WARN, so I thought there were no cases where tp is NULL. If it exists, KASAN should be complaining. I think this additional if could confuse future readers and want to make sure if there is such a case. Thank you! > > Should I do check for "ck" instead of "tp" to make the code a bit > cleaner to read? > >>> + pr_warn("Socket Info: family=%u state=%d sport=%u dport=%u ccname=%s cwnd=%u", >>> + sk->sk_family, sk->sk_state, ntohs(inet->inet_sport), >>> + ntohs(inet->inet_dport), icsk->icsk_ca_ops->name, tcp_snd_cwnd(tp)); >>> + >>> + switch (sk->sk_family) { >>> + case AF_INET: >>> + pr_warn("saddr=%pI4 daddr=%pI4", &inet->inet_saddr, >>> + &inet->inet_daddr); >> >> As with tcp_syn_flood_action(), [address]:port format is easy >> to read and consistent in kernel ? > > Absolutely. I am going to fix it in v2. Thanks!
On Tue, 29 Nov 2022 11:18:27 +0100 Paolo Abeni wrote: > On Thu, 2022-11-24 at 03:22 -0800, Breno Leitao wrote: > > There are cases where we need information about the socket during a > > warning, so, it could help us to find bugs that happens and do not have > > an easy repro. > > > > This diff creates a TCP socket-specific version of WARN_ON_ONCE(), which > > dumps more information about the TCP socket. > > > > This new warning is not only useful to give more insight about kernel bugs, but, > > it is also helpful to expose information that might be coming from buggy > > BPF applications, such as BPF applications that sets invalid > > tcp_sock->snd_cwnd values. > > I personally find this use-case a little too tight, you could likelly > fetch the same information with a perf probe or something similar. It's just the initial case, to keep the patch small. The intent is to convert all TCP warnings to this helper. As Breno says in the first sentence this is about having enough relevant information to zero in on the cause of the rare crashes / warnings (which are hit quite a lot on our "millions of machines").
On Tue, Nov 29, 2022 at 09:16:16PM +0000, Iwashima, Kuniyuki wrote: > > On Nov 29, 2022, at 21:48, Breno Leitao <leitao@debian.org> wrote: > >> On Tue, Nov 29, 2022 at 10:00:55AM +0900, Kuniyuki Iwashima wrote: <snip> > >>> +void tcp_sock_warn(const struct tcp_sock *tp) > >>> +{ > >>> + const struct sock *sk = (const struct sock *)tp; > >>> + struct inet_sock *inet = inet_sk(sk); > >>> + struct inet_connection_sock *icsk = inet_csk(sk); > >>> + > >>> + WARN_ON(1); > >>> + > >>> + if (!tp) > >> > >> Is this needed ? > > > > We are de-referencing tp/sk in the lines below, so, I think it is safe to > > check if they are not NULL before the de-refencing it. > > tp->snd_cwnd is accessed just after this WARN, > so I thought there were no cases where tp is NULL. Oh, important to say that we want to re-use this macro on other places as well. This initial usage (on tcp_snd_cwnd_set()) is just for the initial patch. I see value replacing some WARN_ON_*() by TCP_SOCK_WARN_ON_ONCE() in other parts of the code, so, this check is to protect this warning when TCP_SOCK_WARN_ON_ONCE() is called from different places. Anyway, I definitely can remove the check here, but, we might want to re-add it later, as we replace some WARN_ON_* by TCP_SOCK_WARN_ON_*(); > I think this additional if could confuse future readers and > want to make sure if there is such a case. How come checking if a pointer is valid before de-refencing it could confuse readers? Thank you for the review!
diff --git a/include/net/tcp.h b/include/net/tcp.h index 14d45661a84d..e490af8e6fdc 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -40,6 +40,7 @@ #include <net/inet_ecn.h> #include <net/dst.h> #include <net/mptcp.h> +#include <net/tcp_debug.h> #include <linux/seq_file.h> #include <linux/memcontrol.h> @@ -1229,7 +1230,7 @@ static inline u32 tcp_snd_cwnd(const struct tcp_sock *tp) static inline void tcp_snd_cwnd_set(struct tcp_sock *tp, u32 val) { - WARN_ON_ONCE((int)val <= 0); + TCP_SOCK_WARN_ON_ONCE(tp, (int)val <= 0); tp->snd_cwnd = val; } diff --git a/include/net/tcp_debug.h b/include/net/tcp_debug.h new file mode 100644 index 000000000000..50e96d87d335 --- /dev/null +++ b/include/net/tcp_debug.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_TCP_DEBUG_H +#define _LINUX_TCP_DEBUG_H + +void tcp_sock_warn(const struct tcp_sock *tp); + +#define TCP_SOCK_WARN_ON_ONCE(tcp_sock, condition) \ + DO_ONCE_LITE_IF(condition, tcp_sock_warn, tcp_sock) + +#endif /* _LINUX_TCP_DEBUG_H */ diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 54836a6b81d6..dd682f60c7cb 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4705,6 +4705,36 @@ int tcp_abort(struct sock *sk, int err) } EXPORT_SYMBOL_GPL(tcp_abort); +void tcp_sock_warn(const struct tcp_sock *tp) +{ + const struct sock *sk = (const struct sock *)tp; + struct inet_sock *inet = inet_sk(sk); + struct inet_connection_sock *icsk = inet_csk(sk); + + WARN_ON(1); + + if (!tp) + return; + + pr_warn("Socket Info: family=%u state=%d sport=%u dport=%u ccname=%s cwnd=%u", + sk->sk_family, sk->sk_state, ntohs(inet->inet_sport), + ntohs(inet->inet_dport), icsk->icsk_ca_ops->name, tcp_snd_cwnd(tp)); + + switch (sk->sk_family) { + case AF_INET: + pr_warn("saddr=%pI4 daddr=%pI4", &inet->inet_saddr, + &inet->inet_daddr); + break; +#if IS_ENABLED(CONFIG_IPV6) + case AF_INET6: + pr_warn("saddr=%pI6 daddr=%pI6", &sk->sk_v6_rcv_saddr, + &sk->sk_v6_daddr); + break; +#endif + } +} +EXPORT_SYMBOL_GPL(tcp_sock_warn); + extern struct tcp_congestion_ops tcp_reno; static __initdata unsigned long thash_entries;