netdev - Re: [RFC PATCH net] tcp: Update pcount after skb

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]

Message-ID: <CANn89iLd1bqCOxbALcDNzi--Hn8A970HmvM2__ryzjwSbOwZ6Q@mail.gmail.com>
Date:	Fri, 5 Jun 2015 09:53:51 -0700
From:	Eric Dumazet <edumazet@...gle.com>
To:	Martin KaFai Lau <kafai@...com>
Cc:	netdev <netdev@...r.kernel.org>, Kernel Team <kernel-team@...com>,
	Neal Cardwell <ncardwell@...gle.com>,
	Yuchung Cheng <ycheng@...gle.com>
Subject: Re: [RFC PATCH net] tcp: Update pcount after skb_pull() during mtu probing

On Fri, Jun 5, 2015 at 9:16 AM, Martin KaFai Lau <kafai@...com> wrote:
> The problem is caught by this WARN_ON(len > skb->len) in tcp_fragment():
>
> [<ffffffff810510ca>] warn_slowpath_null+0x1a/0x20
> [<ffffffff8160ec90>] tcp_fragment+0x2a0/0x2b0
> [<ffffffff81604e06>] tcp_mark_head_lost+0x196/0x230
> [<ffffffff8160585d>] tcp_update_scoreboard+0x4d/0x80
> [<ffffffff8160a9ac>] tcp_fastretrans_alert+0x6ac/0xa90
> [<ffffffff8160b834>] tcp_ack+0x9d4/0x10e0
> [<ffffffff8160c699>] tcp_rcv_established+0x309/0x7e0
>
> The WARN_ON pointed out that tcp_skb_pcount (i.e.
> TCP_SKB_CB(skb)->tcp_gso_segs) and skb->len is inconsistent.
>
> The WARN_ON stack goes away after setting net.ipv4.tcp_mtu_probing to 0.
>
> This patch is to set the pcount after skb_pull() was called
> in tcp_mtu_probe().
>
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> Reported-by: Grant Zhang <gzhang@...tly.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Neal Cardwell <ncardwell@...gle.com>
> Cc: Yuchung Cheng <ycheng@...gle.com>
> ---
>  net/ipv4/tcp_output.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index a369e8a..6c95c28 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -1984,8 +1984,9 @@ static int tcp_mtu_probe(struct sock *sk)
>                                                                  skb->len, 0);
>                         } else {
>                                 __pskb_trim_head(skb, copy);
> -                               tcp_set_skb_tso_segs(sk, skb, mss_now);
>                         }
> +                       tcp_set_skb_tso_segs(sk, skb, mss_now);
> +
>                         TCP_SKB_CB(skb)->seq += copy;
>                 }
>
>

Sounds good, although I would simply get rid of all this complexity in
this very unlikely path.

Would you instead try the following ?

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index eeb59befaf06867b00e1dd6ded7742b2f0bcd821..41ef2c96b2e810e289d6d773e4be09c493b99c09
100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1987,16 +1987,7 @@ static int tcp_mtu_probe(struct sock *sk)
                } else {
                        TCP_SKB_CB(nskb)->tcp_flags |=
TCP_SKB_CB(skb)->tcp_flags &
                                                   ~(TCPHDR_FIN|TCPHDR_PSH);
-                       if (!skb_shinfo(skb)->nr_frags) {
-                               skb_pull(skb, copy);
-                               if (skb->ip_summed != CHECKSUM_PARTIAL)
-                                       skb->csum = csum_partial(skb->data,
-                                                                skb->len, 0);
-                       } else {
-                               __pskb_trim_head(skb, copy);
-                               tcp_set_skb_tso_segs(sk, skb, mss_now);
-                       }
-                       TCP_SKB_CB(skb)->seq += copy;
+                       tcp_trim_head(sk, skb, copy);
                }

                len += copy;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html