* [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"
@ 2013-03-13 12:37 Timo Teräs
2013-03-13 15:05 ` Isaku Yamahata
2013-03-17 3:01 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Timo Teräs @ 2013-03-13 12:37 UTC (permalink / raw)
To: netdev, Isaku Yamahata, Eric Dumazet, David S. Miller; +Cc: Timo Teräs
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.
Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
net/ipv4/ip_gre.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
This commit appeared in 3.8.x. So should go to 3.8.x-stable.
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index d0ef0e6..91d66db 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -798,10 +798,7 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
if (dev->header_ops && dev->type == ARPHRD_IPGRE) {
gre_hlen = 0;
- if (skb->protocol == htons(ETH_P_IP))
- tiph = (const struct iphdr *)skb->data;
- else
- tiph = &tunnel->parms.iph;
+ tiph = (const struct iphdr *)skb->data;
} else {
gre_hlen = tunnel->hlen;
tiph = &tunnel->parms.iph;
--
1.8.1.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"
2013-03-13 12:37 [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally" Timo Teräs
@ 2013-03-13 15:05 ` Isaku Yamahata
2013-03-13 15:56 ` Timo Teras
2013-03-17 3:01 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Isaku Yamahata @ 2013-03-13 15:05 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev, Eric Dumazet, David S. Miller
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))
....
thanks,
>
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
> net/ipv4/ip_gre.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> This commit appeared in 3.8.x. So should go to 3.8.x-stable.
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index d0ef0e6..91d66db 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -798,10 +798,7 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
>
> if (dev->header_ops && dev->type == ARPHRD_IPGRE) {
> gre_hlen = 0;
> - if (skb->protocol == htons(ETH_P_IP))
> - tiph = (const struct iphdr *)skb->data;
> - else
> - tiph = &tunnel->parms.iph;
> + tiph = (const struct iphdr *)skb->data;
> } else {
> gre_hlen = tunnel->hlen;
> tiph = &tunnel->parms.iph;
> --
> 1.8.1.5
>
--
yamahata
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"
2013-03-13 15:05 ` Isaku Yamahata
@ 2013-03-13 15:56 ` Timo Teras
2013-03-14 2:53 ` Isaku Yamahata
0 siblings, 1 reply; 5+ messages in thread
From: Timo Teras @ 2013-03-13 15:56 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: netdev, Eric Dumazet, David S. Miller
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"
2013-03-13 15:56 ` Timo Teras
@ 2013-03-14 2:53 ` Isaku Yamahata
0 siblings, 0 replies; 5+ messages in thread
From: Isaku Yamahata @ 2013-03-14 2:53 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev, Eric Dumazet, David S. Miller
On Wed, Mar 13, 2013 at 05:56:43PM +0200, Timo Teras wrote:
> 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.
You're right. Thanks for explanation.
Originally I tried to copy TTL in MPLS label into outer IP header.
But dropped MPLS bits.
--
yamahata
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally"
2013-03-13 12:37 [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally" Timo Teräs
2013-03-13 15:05 ` Isaku Yamahata
@ 2013-03-17 3:01 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2013-03-17 3:01 UTC (permalink / raw)
To: timo.teras; +Cc: netdev, yamahata, edumazet
From: Timo Teräs <timo.teras@iki.fi>
Date: Wed, 13 Mar 2013 14:37:49 +0200
> 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.
>
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
Applied, and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-17 3:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-13 12:37 [PATCH] Revert "ip_gre: make ipgre_tunnel_xmit() not parse network header as IP unconditionally" Timo Teräs
2013-03-13 15:05 ` Isaku Yamahata
2013-03-13 15:56 ` Timo Teras
2013-03-14 2:53 ` Isaku Yamahata
2013-03-17 3:01 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).