netdev - Re: [PATCH net-next 2/2] net: Loosen constraints for recalculating checksum in 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: <CAEP_g=94-Nsia9xCTwe0=2VnZgESZ6XHPJQ96FwrsgPTWEbiWg@mail.gmail.com>
Date:	Mon, 29 Apr 2013 13:01:44 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>,
	Jarno Rajahalme <jarno.rajahalme@....com>,
	Joseph Gasparakis <joseph.gasparakis@...el.com>,
	Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@...el.com>,
	Alexander Duyck <alexander.h.duyck@...el.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Maciej Żenczykowski <maze@...gle.com>
Subject: Re: [PATCH net-next 2/2] net: Loosen constraints for recalculating
 checksum in skb_segment()

On Thu, Apr 25, 2013 at 12:26 AM, Simon Horman <horms@...ge.net.au> wrote:
> On Tue, Apr 23, 2013 at 04:43:58PM -0700, Jesse Gross wrote:
>> On Mon, Apr 22, 2013 at 7:19 PM, Simon Horman <horms@...ge.net.au> wrote:
>> > In the case where a non-MPLS GSO skb becomes an MPLS GSO skb, via
>> > Open vSwitch's push MPLS action it is desirable to provide segmentation
>> > in software. In this case the original protocol of the skb may have allowed
>> > its checksumming to be offloaded but this may no longer be supported now
>> > the skb is MPLS. Actually it seems to me that this is the likely case.
>> >
>> > In order to allow the checksum to be updated in this case loosen
>> > the rules for recalculating the checksum on in skb_segment().
>> >
>> > N.B.: I must confess that I am a little unsure of the details of
>> > the implementation of skb_checksum(). But I have observed that this
>> > is necessary as skb_checksum() hits the following:
>> >
>> >                 if (!hsize && i >= nfrags) {
>> >                         ...
>> >                         fskb = fskb->next;
>> >                         ...
>> >                 }
>> >                 ...
>> >                 if (fskb != skb_shinfo(skb)->frag_list)
>> >                         ...
>> >
>> > Signed-off-by: Simon Horman <horms@...ge.net.au>
>>
>> I'm surprised at the need for changes here since neither vlans nor
>> tunneling protocols needed something similar. Do you know what is
>> special about MPLS that is triggering this?
>
> After some testing I believe that the answer for GRE is, much to my
> surprise, that it doesn't work without this patch. At least not for the
> test case I have been using.
>
> To test GRE I set up a machine to receive IP packets and forward them over
> a GRE (not TEB) tunnel created using the ip command. In that case I see
> incorrect TCP checksums and then retransmissions when GSO segmentation
> occurs.  A tcpdump is below.

Hmm, skb_segment() should be independent of protocol but I can see how
this is a case that wouldn't get exercised frequently and therefore
could have bugs. It seems like the case would be receiving a packet
that gets merged using GRO and then imposing some kind of header (but
not a single level of vlan since that is stored out of band). I think
probably it's necessary to do some more careful analysis to make sure
that this change is safe in all cases (or at least expand the commit
message since it's not immediately obvious at the moment).
--
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