netdev - Re: tcp: skb_shift() issues w.r.t. SACK processing

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:	Fri, 2 Mar 2012 16:57:30 -0500
From:	Neal Cardwell <ncardwell@...gle.com>
To:	Vijay Subramanian <subramanian.vijay@...il.com>
Cc:	netdev <netdev@...r.kernel.org>, ilpo.jarvinen@...sinki.fi
Subject: Re: tcp: skb_shift() issues w.r.t. SACK processing

On Thu, Mar 1, 2012 at 4:36 PM, Vijay Subramanian
<subramanian.vijay@...il.com> wrote:
> Hi,
>
> I was looking at the skb_shift() function that is called by
> tcp_shift_skb_data() to shift bytes from a skb to the previous skb.
> This is used to collapse multiple
> SACK blocks into a single skb. I noticed the following two things.
>
> 1: skb_shift() has the following snippet.
>  if (!to ||
>            !skb_can_coalesce(tgt, to, skb_frag_page(fragfrom),
>                              fragfrom->page_offset)) {
>                merge = -1;
>        } else {
>                merge = to - 1;
>
>                todo -= skb_frag_size(fragfrom);
>                if (todo < 0) {
>                        if (skb_prepare_for_shift(skb) ||
>                            skb_prepare_for_shift(tgt))
>                                return 0;
>
>                        /* All previous frag pointers might be stale! */
>                        fragfrom = &skb_shinfo(skb)->frags[from];
>                        fragto = &skb_shinfo(tgt)->frags[merge];
>
>                        skb_frag_size_add(fragto, shiftlen);
>                        skb_frag_size_sub(fragfrom, shiftlen);
>                        fragfrom->page_offset += shiftlen;
>
>                        goto onlymerged;
>                }
>
>                from++;
>        }
>
> The (todo < 0) part is executed when we need to copy less than
> skb_frag_size(fragfrom). Essentially, after the bytes have been
> shifted, the frag in the original skb will still retain some data.
> However, unless I am reading this wrong, the bytes are actually never
> shifted. The frag offsets and size are updated and then we jump to
> label onlymerged which  updates other counters.
> It looks like the data will be lost since it is in neither skb. This
> may not matter in normal case since this data has been SACKed by
> receiver and will be discarded after cumulative ack.
> But in case of receiver SACK reneging, this data may potentially be
> resent. A similar case later in the function is treated correctly with
> the data actually getting copied.

I think skb_shift() is actually OK in this regard. The code in
question is guarded by a call to skb_can_coalesce(), which ensures
that the source and destination fragments are actually from the same
page, and are adjacent within the page. So in this case, the code
seems correct in its assumption that all we need to do is increment
the size of the destination fragment and decrement the size of the
source fragment.

neal

ps: I haven't looked into your second question.
--
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