netdev - Re: [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path

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: <CAADnVQ+H6e5HZi1BiFYixkgp22RkO8J4qr0MK0z9MpsOdgbPeA@mail.gmail.com>
Date:	Tue, 4 Mar 2014 17:14:26 -0800
From:	Alexei Starovoitov <alexei.starovoitov@...il.com>
To:	Jerry Chu <hkchu@...gle.com>
Cc:	Or Gerlitz <ogerlitz@...lanox.com>,
	David Miller <davem@...emloft.net>,
	Joseph Gasparakis <joseph.gasparakis@...el.com>,
	Pravin B Shelar <pshelar@...ira.com>,
	Eric Dumazet <edumazet@...gle.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Tom Herbert <therbert@...gle.com>
Subject: Re: [PATCH] net-gre-gro: Fix a bug that breaks the forwarding path

On Tue, Mar 4, 2014 at 2:13 PM, Jerry Chu <hkchu@...gle.com> wrote:
> Hi Or,
>
> Thanks for writing this up.
>
> On Tue, Mar 4, 2014 at 8:13 AM, Or Gerlitz <ogerlitz@...lanox.com> wrote:
>> On 28/02/2014 23:56, David Miller wrote:
>>>
>>> The topic of the skb->encapsulation semantics has come up several times in
>>> the past few weeks. We cannot move forward on any changes in this area until
>>> the semantics are well defined, and documented. Can someone work on a patch
>>> which documents skb->encapsulation properly, and then we can come back to
>>> fixing this bug? Thanks.
>>
>>
>> Lets try... the skb->encapsulation flag was introduced and used in 3.8 by
>> the
>> sequence of these three commits
>>
>> 0afb166 vxlan: Add capability of Rx checksum offload for inner packet
>> d6727fe vxlan: capture inner headers during encapsulation
>> 6a674e9 net: Add support for hardware-offloaded encapsulation
>>
>> When discussed earlier on the list in the context of the skb->ip_summed
>> field,
>> Tom Herbert came with the following interpretation for the semantics which
>> Joseph confirmed
>>
>> "when skb->encapsulation is set the ip_summed is valid for both the inner
>> and outer header
>> (e.g. CHECKSUM_COMPLETE is always assumed okay for both layers). If
>> skb->encapsulation is not set then ip_summed is only valid for outer header"
>
> For GRE encapped pkts is the following interpretation correct?
>
> 1) ip_summed == CHECKSUM_COMPLETE
>     => csum covers IP payload csum of the outer IP datagram

my understanding here is that if ip_summed == CHECKSUM_COMPLETE
the hw computed skb->csum covers the whole packet except mac header.
vlan_untag() will adjust csum
then ip_local_deliver_finish() will pop iphdr without touching skb->csum,
since for the valid iphdr, it's a nop operation.
then gre_rcv()-> gre_cisco_rcv()->parse_gre_header()->iptunnel_pull_header()
will adjust skb->csum after pulling gre header.
and so on.

xoring csum at every pull is not cheap, so when HW sets
CHECKSUM_UNNECESSARY and verifies inner tcp/udp checksum it's the best.
Ideally HW would have a fallback to checksum_complete if it cannot parse encap.

> 2) ip_summed == CHECKSUM_UNNECESSARY
> 2.1. skb->encapsulation is on => both GRE csum (if one is present) and TCP/UDP
> csum have been validated (assuming inner is a TCP or UDP pkt)
>
> 2.2. skb->encapsulation is off => only GRE csum (if one is present) is
> validated.
>
>>
>> As for the TX side of things, the change-log of commit 6a674e9 states
>>
>> "For Tx encapsulation offload, the driver will need to set the right bits in
>> netdev->hw_enc_features. The protocol driver will have to set the
>> skb->encapsulation bit and populate the inner headers, so the NIC driver
>> will use those inner headers to calculate the csum in hardware."
>
> So we only support/care about csum offload for the inner pkts, which
> makes sense.
>
>>
>> So in higher level, it seems that the role of the skb->encapsulation field
>> is to mark the skb to carry encapsulated packet for the code path between
>> the time the packet is encapsulated by the protocol driver (e.g vxlan/ipip)
>> to the time driver xmit is called. Or from the time driver rx code runs till
>> the the time the packet is decapsulated.
>>
>> Further, my personal interpretation was that on the rx path, skb should
>> carry the encapsulation flag **only** if the HW was able to offload the
>> inner checksum.
>
> SGTM.
>
> Jerry
>
>>
>> Joseph, what's your thinking here?
>>
>> Or.
>>
>>
> --
> 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
--
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