netdev - Re: tcp_shift_skb_data uses wrong mss in non-gso case?

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:	Mon, 23 Aug 2010 15:34:04 -0700
From:	Jesse Brandeburg <jesse.brandeburg@...il.com>
To:	Robert Evans <robert.evans@...daqomx.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: tcp_shift_skb_data uses wrong mss in non-gso case?

CC Ilpo, the creator of this patch being discussed:

On Tue, Aug 17, 2010 at 3:35 PM, Robert Evans
<robert.evans@...daqomx.com> wrote:
> Hello all,
>
> In reading through the latest SACK code introduced by 832d11c, I have noticed
> that for the in_sack case tcp_shift_skb will take mss = tcp_skb_seglen(skb).
> This seems to be wrong since the queue might contain small packets (f.e.
> TCP_NODELAY). If the collapse succeeds, the resulting skb will have an
> arbitrarily small gso_size equal to the original skb length.

yeah, gso_size really should never be == skb->length, because then it
implies you're offloading a frame to be segmented with no segmentable
data.

> 8ed88d4:net/ipv4/tcp_input.c
>   1506         in_sack = !after(start_seq, TCP_SKB_CB(skb)->seq) &&
>   1507                   !before(end_seq, TCP_SKB_CB(skb)->end_seq);
>   1508
>   1509         if (in_sack) {
>   1510                 len = skb->len;
>   1511                 pcount = tcp_skb_pcount(skb);
>   1512                 mss = tcp_skb_seglen(skb);     /* possibly wrong? */
>   1513
>   1514                 /* TODO: Fix DSACKs to not fragment already SACKed and w
>   1514 e can
>   1515                  * drop this restriction as unnecessary
>   1516                  */
>   1517                 if (mss != tcp_skb_seglen(prev))
>   1518                         goto fallback;
>   1519         } else {
>
> This ends up being troublesome if the segment is later retransmitted and the
> device driver has trouble with very small gso_size (e1000e seems to be an
> example).

I bet lots of other drivers will have issue with this too.

> Is the small gso_size the correct and/or desired behavior?  Or am I missing
> something else that prevents this from being a problem?

I believe that this is invalid for the stack to do, Ilpo, Herbert?
what do you think?
--
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