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: <CAPshTCi515brk2mR=im9CYJUHWEFT2=wcT=VCR_VwkS1RV2BbQ@mail.gmail.com>
Date:	Tue, 4 Mar 2014 16:54:21 -0800
From:	Jerry Chu <hkchu@...gle.com>
To:	Joseph Gasparakis <joseph.gasparakis@...el.com>
Cc:	Or Gerlitz <ogerlitz@...lanox.com>,
	David Miller <davem@...emloft.net>,
	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 5:01 PM, Joseph Gasparakis
<joseph.gasparakis@...el.com> wrote:
>
>
> On Tue, 4 Mar 2014, Jerry Chu wrote:
>
>> On Tue, Mar 4, 2014 at 2:53 PM, Joseph Gasparakis
>> <joseph.gasparakis@...el.com> wrote:
>> >
>> >
>> > On Tue, 4 Mar 2014, Jerry Chu 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
>> >>
>> >> 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)
>> >
>> > i40e also supports SCTP csumming for the inner packet, too.
>> >
>> >>
>> >> 2.2. skb->encapsulation is off => only GRE csum (if one is present) is
>> >> validated.
>> >>
>> >
>> > Apart for the SCTP request for inclusion, it looks reasonable to me.
>> >
>> >> >
>> >> > 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.
>> >
>> > Well, we care about outer too. It is just that the inner headers are for
>> > the inner csum, the outer headers are for the outer csum. The outer
>> > headers will be always there anyway, so the patch introduced the inner
>> > ones. But I guess this is what you meant, right?
>>
>> Actually i was wondering that since there is only one set csum_start/csum_offset
>> fields per skb, which would you support CHECKSUM_PARTIAL for both inner
>> and outer? You can only support one, right? (I haven't checked the UDP encap
>> code to see how this works though.)
>>
>> Jerry
>
> I am not sure I understand the question, Jerry... Are you asking in Tx
> path if the ip_summed==CHECKSUM_PARTIAL, which headers will the
> csum_start/csum_offset refer to?

Yes (if the question makes sense since i haven't read any UDP encap
code - will the outer (UDP) csum be enabled?).

Thanks,

Jerry

>
>> >
>> >>
>> >> >
>> >> > 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