Re: [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"

[Timo Teras <timo.teras@iki.fi>]

On Thu, 14 Mar 2013 00:05:10 +0900
Isaku Yamahata <yamahata@valinux.co.jp> wrote:

> Hi.
> 
> On Wed, Mar 13, 2013 at 02:37:49PM +0200, Timo Teräs wrote:
> > This reverts commit 412ed94744d16806fbec3bd250fd94e71cde5a1f.
> > 
> > The commit is wrong as tiph points to the outer IPv4 header which is
> > installed at ipgre_header() and not the inner one which is protocol
> > dependant.
> > 
> > This commit broke succesfully opennhrp which use PF_PACKET socket
> > with ETH_P_NHRP protocol. Additionally ssl_addr is set to the
> > link-layer IPv4 address. This address is written by ipgre_header()
> > to the skb earlier, and this is the IPv4 header tiph should point
> > to - regardless of the inner protocol payload.
> 
> Is this the case only for ETH_P_HNRP?
> I wrote the patch having MPLS over GRE in mind.
> Should it be something like this?
>   if (protocol == htons(ETH_P_IP) || protocol == htons(ETH_P_NHRP))
>     ....

No, the original code was correct for all protocols.

tiph refers to the IPv4 headers pushed by ip_gre driver in
ipgre_header() function. This is _always_ present regardless of the
inner protocol. Thus no checking for protocol should be done.

And for reference: NHRP does not have iphdr, it is completely different
payload.

What happens is on xmit path is:

 1. ipgre_header() is always called first with NBMA destination
    address and outer IPv4 + GRE headers are pushed here with
    per-packet destination set

 2. ipgre_tunnel_xmit() is called, tiph points to the tunnel's
    IPv4 header, gre_hlen is set to zero so that the header is not
    pushed twice, but the existing header is modified later

Thus, it does not matter if the payload is MPLS, we will not refer to
the mpls data - the skb has already tunnel's outer IPv4 and GRE header
pushed, and tiph will refer to the tunnel's IPv4 header.

- Timo