Re: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue

From: maowenan <maowenan@huawei.com>
To: Tim Froidcoeur <tim.froidcoeur@tessares.net>, <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	"cpaasch@apple.com" <cpaasch@apple.com>,
	"jonathan.lemon@gmail.com" <jonathan.lemon@gmail.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"matthieu.baerts@tessares.net" <matthieu.baerts@tessares.net>,
	"aprout@ll.mit.edu" <aprout@ll.mit.edu>,
	"edumazet@google.com" <edumazet@google.com>,
	"jtl@netflix.com" <jtl@netflix.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mkubecek@suse.cz" <mkubecek@suse.cz>,
	"ncardwell@google.com" <ncardwell@google.com>,
	"sashal@kernel.org" <sashal@kernel.org>,
	"ycheng@google.com" <ycheng@google.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH 4.14] tcp: fix tcp_rtx_queue_tail in case of empty retransmit queue
Date: Tue, 3 Sep 2019 16:55:38 +0800	[thread overview]
Message-ID: <88936af6-4b98-c78f-930f-47e5d69c961d@huawei.com> (raw)
In-Reply-To: <CAOj+RUvXMaoVKzSeDab4oTn3p=-BJtuhgqwKDCUuhCQWHO7bgQ@mail.gmail.com>

On 2019/9/3 14:58, Tim Froidcoeur wrote:
> Hi,
> 
> I also tried to reproduce this in a targeted way, and run into the
> same difficulty as you: satisfying the first condition “
> (sk->sk_wmem_queued >> 1) > limit “.
> I will not have bandwidth the coming days to try and reproduce it in
> this way. Maybe simply forcing a very small send buffer using sysctl
> net.ipv4.tcp_wmem might even do the trick?
> 
> I suspect that the bug is easier to trigger with the MPTCP patch like
> I did originally, due to the way this patch manages the tcp subflow
> buffers (it can temporarily overfill the buffers, satisfying that
> first condition more often).
> 
> another thing, the stacktrace you shared before seems caused by
> another issue (corrupted socket?), it will not be solved by the patch
> we submitted.

The trace shows zero window probe message can be BUG_ON in skb_queue_prev,
this is reproduced on our platform with syzkaller. It can be resolved by
your fix patch.
The thing I need to think is why the first condition can be satisfied?
Eric, Do you have any comments to reproduce it as the first condition
is hard to be true?
(sk->sk_wmem_queued >> 1) > limit

> 
> kind regards,
> 
> Tim
> 
> 
> On Tue, Sep 3, 2019 at 5:22 AM maowenan <maowenan@huawei.com> wrote:
>>
>> Hi Tim,
>>
>>
>>
>> I try to reproduce it with packetdrill or user application, but I can’t.
>>
>> The first condition “ (sk->sk_wmem_queued >> 1) > limit “    can’t be satisfied,
>>
>> This condition is to avoid tiny SO_SNDBUF values set by user.
>>
>> It also adds the some room due to the fact that tcp_sendmsg()
>>
>> and tcp_sendpage() might overshoot sk_wmem_queued by about one full
>>
>> TSO skb (64KB size).
>>
>>
>>
>>         limit = sk->sk_sndbuf + 2 * SKB_TRUESIZE(GSO_MAX_SIZE);
>>
>>         if (unlikely((sk->sk_wmem_queued >> 1) > limit &&
>>
>>                      skb != tcp_rtx_queue_head(sk) &&
>>
>>                      skb != tcp_rtx_queue_tail(sk))) {
>>
>>                 NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPWQUEUETOOBIG);
>>
>>                 return -ENOMEM;
>>
>>         }
>>
>>
>>
>> Can you try to reproduce it with packetdrill or C socket application?
>>
>>
> 
> 
> 