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;
Subject: Re: [PATCH RESEND net-next] tcp: socket-specific version of WARN_ON_ONCE()
Thread-Topic: [PATCH RESEND net-next] tcp: socket-specific version of WARN_ON_ONCE()
From:   "Iwashima, Kuniyuki" <kuniyu@amazon.co.jp>
To:     Breno Leitao <leitao@debian.org>
CC:     "davem@davemloft.net" <davem@davemloft.net>,
        "dsahern@kernel.org" <dsahern@kernel.org>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>, "leit@fb.com" <leit@fb.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "yoshfuji@linux-ipv6.org" <yoshfuji@linux-ipv6.org>
Thread-Index: AQHZA44PkgbcE2NjRk6nD9bv4KRS8q5V2lsAgACOKkA=
Date:   Tue, 29 Nov 2022 21:16:16 +0000
Message-ID: <A5D7EBCF-CC49-4575-9DA7-0419BA1F0E9B@amazon.co.jp>
References: <20221124112229.789975-1-leitao@debian.org>
 <20221129010055.75780-1-kuniyu@amazon.com>,<Y4X/XidkaLaD5Zak@gmail.com>
In-Reply-To: <Y4X/XidkaLaD5Zak@gmail.com>
Accept-Language: ja-JP, en-US
Content-Language: ja-JP
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk


> 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.
>>>=20
>>> This diff creates a TCP socket-specific version of WARN_ON_ONCE(), whic=
h
>>> dumps more information about the TCP socket.
>>>=20
>>> This new warning is not only useful to give more insight about kernel b=
ugs, but,
>>> it is also helpful to expose information that might be coming from bugg=
y
>>> BPF applications, such as BPF applications that sets invalid
>>> tcp_sock->snd_cwnd values.
>>=20
>> Have you finally found a root cause on BPF or TCP side ?
>=20
> Yes, this demonstrated to be very useful to find out BPF applications
> that are doing nasty things with the congestion window.
>=20
> 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,=20
it would be helpful to mention the commit to=20
well understand the issue, background,=20
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);
>>>=20
>>> +void tcp_sock_warn(const struct tcp_sock *tp)
>>> +{
>>> +   const struct sock *sk =3D (const struct sock *)tp;
>>> +   struct inet_sock *inet =3D inet_sk(sk);
>>> +   struct inet_connection_sock *icsk =3D inet_csk(sk);
>>> +
>>> +   WARN_ON(1);
>>> +
>>> +   if (!tp)
>>=20
>> Is this needed ?
>=20
> 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,=20
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=20
want to make sure if there is such a case.

Thank you!

>=20
> Should I do check for "ck" instead of "tp" to make the code a bit
> cleaner to read?
>=20
>>> +   pr_warn("Socket Info: family=3D%u state=3D%d sport=3D%u dport=3D%u =
ccname=3D%s cwnd=3D%u",
>>> +           sk->sk_family, sk->sk_state, ntohs(inet->inet_sport),
>>> +           ntohs(inet->inet_dport), icsk->icsk_ca_ops->name, tcp_snd_c=
wnd(tp));
>>> +
>>> +   switch (sk->sk_family) {
>>> +   case AF_INET:
>>> +           pr_warn("saddr=3D%pI4 daddr=3D%pI4", &inet->inet_saddr,
>>> +                   &inet->inet_daddr);
>>=20
>> As with tcp_syn_flood_action(), [address]:port format is easy
>> to read and consistent in kernel ?
>=20
> Absolutely. I am going to fix it in v2. Thanks!