netdev - Re: [PATCH] tcp: make retransmitted SKB fit into the send window

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Jul 2022 14:17:24 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Yonglong Li <liyonglong@...natelecom.cn>
Cc:     netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH] tcp: make retransmitted SKB fit into the send window

On Thu, Jul 7, 2022 at 12:50 PM Yonglong Li <liyonglong@...natelecom.cn> wrote:
>
> From: liyonglong <liyonglong@...natelecom.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@...natelecom.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
>