| Message ID | 1363178269-27553-1-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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.