* [PATCH] tcp: make retransmitted SKB fit into the send window
@ 2022-07-07 10:19 Yonglong Li
2022-07-07 12:17 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Yonglong Li @ 2022-07-07 10:19 UTC (permalink / raw)
To: netdev; +Cc: davem, dsahern, edumazet, kuba, pabeni, liyonglong
From: liyonglong <liyonglong@chinatelecom.cn>
current code of __tcp_retransmit_skb only check TCP_SKB_CB(skb)->seq
in send window, it will cause retransmit more than send window data.
Signed-off-by: liyonglong <liyonglong@chinatelecom.cn>
---
net/ipv4/tcp_output.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 18c913a..3530d1f 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -3176,7 +3176,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
TCP_SKB_CB(skb)->seq != tp->snd_una)
return -EAGAIN;
- len = cur_mss * segs;
+ len = min_t(int, tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq, cur_mss * segs);
if (skb->len > len) {
if (tcp_fragment(sk, TCP_FRAG_IN_RTX_QUEUE, skb, len,
cur_mss, GFP_ATOMIC))
@@ -3190,7 +3190,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
diff -= tcp_skb_pcount(skb);
if (diff)
tcp_adjust_pcount(sk, skb, diff);
- if (skb->len < cur_mss)
+ if (skb->len < cur_mss && len >= cur_mss)
tcp_retrans_try_collapse(sk, skb, cur_mss);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] tcp: make retransmitted SKB fit into the send window
2022-07-07 10:19 [PATCH] tcp: make retransmitted SKB fit into the send window Yonglong Li
@ 2022-07-07 12:17 ` Eric Dumazet
2022-07-08 2:40 ` Yonglong Li
0 siblings, 1 reply; 3+ messages in thread
From: Eric Dumazet @ 2022-07-07 12:17 UTC (permalink / raw)
To: Yonglong Li
Cc: netdev, David Miller, David Ahern, Jakub Kicinski, Paolo Abeni
On Thu, Jul 7, 2022 at 12:50 PM Yonglong Li <liyonglong@chinatelecom.cn> wrote:
>
> From: liyonglong <liyonglong@chinatelecom.cn>
>
> current code of __tcp_retransmit_skb only check TCP_SKB_CB(skb)->seq
> in send window, it will cause retransmit more than send window data.
This changelog is confusing. I understand the check is already done ?
I think it would be better to explain how a receiver can retract its window,
even if TCP RFCs specifically forbid this.
>
> Signed-off-by: liyonglong <liyonglong@chinatelecom.cn>
This probably needs a Fixes: tag, even if targeting net-next (which I
prefer because this kind of patch is risky)
> ---
> net/ipv4/tcp_output.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 18c913a..3530d1f 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3176,7 +3176,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
> TCP_SKB_CB(skb)->seq != tp->snd_una)
> return -EAGAIN;
>
> - len = cur_mss * segs;
> + len = min_t(int, tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq, cur_mss * segs);
I think it is unfortunate to not align len to a multiple of cur_mss,
if possible.
Also this might break so-called zero window probes.
We might need to send some payload, to discover if the ACK the
receiver sent us to re-open its window was lost.
> if (skb->len > len) {
What happens if len == 0 ?
> if (tcp_fragment(sk, TCP_FRAG_IN_RTX_QUEUE, skb, len,
> cur_mss, GFP_ATOMIC))
> @@ -3190,7 +3190,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
> diff -= tcp_skb_pcount(skb);
> if (diff)
> tcp_adjust_pcount(sk, skb, diff);
> - if (skb->len < cur_mss)
> + if (skb->len < cur_mss && len >= cur_mss)
This seems to be weak.
I suggest to do it properly in tcp_retrans_try_collapse() because
there is already a related test there :
if (after(TCP_SKB_CB(skb)->end_seq, tcp_wnd_end(tp)))
break;
But this seems not done properly.
> tcp_retrans_try_collapse(sk, skb, cur_mss);
> }
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tcp: make retransmitted SKB fit into the send window
2022-07-07 12:17 ` Eric Dumazet
@ 2022-07-08 2:40 ` Yonglong Li
0 siblings, 0 replies; 3+ messages in thread
From: Yonglong Li @ 2022-07-08 2:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, David Miller, David Ahern, Jakub Kicinski, Paolo Abeni
On 7/7/2022 8:17 PM, Eric Dumazet wrote:
> On Thu, Jul 7, 2022 at 12:50 PM Yonglong Li <liyonglong@chinatelecom.cn> wrote:
>>
>> From: liyonglong <liyonglong@chinatelecom.cn>
>>
>> current code of __tcp_retransmit_skb only check TCP_SKB_CB(skb)->seq
>> in send window, it will cause retransmit more than send window data.
>
> This changelog is confusing. I understand the check is already done ?
>
> I think it would be better to explain how a receiver can retract its window,
> even if TCP RFCs specifically forbid this.
>
Hi Eric,
Thanks for your reply.
I simulate this case by using packetdrill.
22:14:25.340439 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [S], seq 2685051498, win 65535, options [mss 1460,sackOK,TS val 316430404 ecr 0,nop,wscale 8], length 0
22:14:25.393232 IP 192.0.2.1.8080 > 192.168.226.67.55188: Flags [S.], seq 0, ack 2685051499, win 6000, options [mss 1000,nop,nop,sackOK], length 0
22:14:25.393278 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [.], ack 1, win 65535, length 0
22:14:25.393343 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [.], seq 1:2001, ack 1, win 65535, length 2000: HTTP
22:14:25.393348 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [.], seq 2001:4001, ack 1, win 65535, length 2000: HTTP
22:14:25.393350 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [P.], seq 4001:5001, ack 1, win 65535, length 1000: HTTP
22:14:25.393353 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [.], seq 5001:6001, ack 1, win 65535, length 1000: HTTP
22:14:25.445239 IP 192.0.2.1.8080 > 192.168.226.67.55188: Flags [.], ack 4001, win 1001, length 0 // client retract window to 1001
22:14:25.557222 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [.], seq 5001:6001, ack 1, win 65535, length 1000: HTTP // now send windonw is [4001,5002], and tlp send 5001-6001
22:14:25.816255 IP 192.168.226.67.55188 > 192.0.2.1.8080: Flags [P.], seq 4001:5001, ack 1, win 65535, length 1000: HTTP
>>
>> Signed-off-by: liyonglong <liyonglong@chinatelecom.cn>
>
> This probably needs a Fixes: tag, even if targeting net-next (which I
> prefer because this kind of patch is risky)
>
>
>> ---
>> net/ipv4/tcp_output.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index 18c913a..3530d1f 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -3176,7 +3176,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
>> TCP_SKB_CB(skb)->seq != tp->snd_una)
>> return -EAGAIN;
>>
>> - len = cur_mss * segs;
>> + len = min_t(int, tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq, cur_mss * segs);
>
> I think it is unfortunate to not align len to a multiple of cur_mss,
> if possible.
>
> Also this might break so-called zero window probes.
> We might need to send some payload, to discover if the ACK the
> receiver sent us to re-open its window was lost.
>
you are right. I miss "zero window probes" case. this patch will get it doesn't work.
>
>> if (skb->len > len) {
>
> What happens if len == 0 ?
>
if len == 0, it will return in pre-check:
if (!before(TCP_SKB_CB(skb)->seq, tcp_wnd_end(tp)) &&
TCP_SKB_CB(skb)->seq != tp->snd_una)
return -EAGAIN;
>
>
>> if (tcp_fragment(sk, TCP_FRAG_IN_RTX_QUEUE, skb, len,
>> cur_mss, GFP_ATOMIC))
>> @@ -3190,7 +3190,7 @@ int __tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb, int segs)
>> diff -= tcp_skb_pcount(skb);
>> if (diff)
>> tcp_adjust_pcount(sk, skb, diff);
>> - if (skb->len < cur_mss)
>> + if (skb->len < cur_mss && len >= cur_mss)
>
> This seems to be weak.
>
> I suggest to do it properly in tcp_retrans_try_collapse() because
> there is already a related test there :
>
> if (after(TCP_SKB_CB(skb)->end_seq, tcp_wnd_end(tp)))
> break;
>
> But this seems not done properly.
>
>> tcp_retrans_try_collapse(sk, skb, cur_mss);
>> }
>>
>> --
>> 1.8.3.1
>>
>
--
Li YongLong
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-08 2:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 10:19 [PATCH] tcp: make retransmitted SKB fit into the send window Yonglong Li
2022-07-07 12:17 ` Eric Dumazet
2022-07-08 2:40 ` Yonglong Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.