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;
Date:   Tue, 29 Nov 2022 04:47:26 -0800
From:   Breno Leitao <leitao@debian.org>
To:     Kuniyuki Iwashima <kuniyu@amazon.com>
Cc:     davem@davemloft.net, dsahern@kernel.org, edumazet@google.com,
        kuba@kernel.org, leit@fb.com, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, pabeni@redhat.com, yoshfuji@linux-ipv6.org
Subject: Re: [PATCH RESEND net-next] tcp: socket-specific version of
 WARN_ON_ONCE()
Message-ID: <Y4X/XidkaLaD5Zak@gmail.com>
References: <20221124112229.789975-1-leitao@debian.org>
 <20221129010055.75780-1-kuniyu@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221129010055.75780-1-kuniyu@amazon.com>
Precedence: bulk

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!