| Message ID | 1418282079-30245-1-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Timo Teräs <timo.teras@iki.fi> Date: Thu, 11 Dec 2014 09:14:39 +0200 > @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > * to gre header. > */ > skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > + skb_reset_mac_header(mac); Please explain to me how this compiles, let alone be functionally tested. -- 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 Thu, 11 Dec 2014 14:36:27 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Timo Teräs <timo.teras@iki.fi> > Date: Thu, 11 Dec 2014 09:14:39 +0200 > > > @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff > > *skb, > > * to gre header. > > */ > > skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > > + skb_reset_mac_header(mac); > > Please explain to me how this compiles, let alone be functionally > tested. Sorry. I made the change twice; once on the build box. And again on my git checkout on work station. Must've been on seriously coffee deprived state. Should be obviously: + skb_reset_mac_header(skb); Is there other comments on it? Or shall I resend? -- 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
From: Timo Teras <timo.teras@iki.fi> Date: Thu, 11 Dec 2014 21:44:56 +0200 > On Thu, 11 Dec 2014 14:36:27 -0500 (EST) > David Miller <davem@davemloft.net> wrote: > >> From: Timo Teräs <timo.teras@iki.fi> >> Date: Thu, 11 Dec 2014 09:14:39 +0200 >> >> > @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff >> > *skb, >> > * to gre header. >> > */ >> > skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); >> > + skb_reset_mac_header(mac); >> >> Please explain to me how this compiles, let alone be functionally >> tested. > > Sorry. I made the change twice; once on the build box. And again on my > git checkout on work station. Must've been on seriously coffee deprived > state. > > Should be obviously: > + skb_reset_mac_header(skb); > > Is there other comments on it? I don't care where you applied the patch, if you didn't type make after making the change, that is extremely careless. -- 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 94213c8..6d2f84d 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -250,10 +250,6 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, struct ip_tunnel *tunnel = netdev_priv(dev); const struct iphdr *tnl_params; - skb = gre_handle_offloads(skb, !!(tunnel->parms.o_flags&TUNNEL_CSUM)); - if (IS_ERR(skb)) - goto out; - if (dev->header_ops) { /* Need space for new headers */ if (skb_cow_head(skb, dev->needed_headroom - @@ -266,6 +262,7 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, * to gre header. */ skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); + skb_reset_mac_header(mac); } else { if (skb_cow_head(skb, dev->needed_headroom)) goto free_skb; @@ -273,6 +270,10 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, tnl_params = &tunnel->parms.iph; } + skb = gre_handle_offloads(skb, !!(tunnel->parms.o_flags&TUNNEL_CSUM)); + if (IS_ERR(skb)) + goto out; + __gre_xmit(skb, dev, tnl_params, skb->protocol); return NETDEV_TX_OK;
The NBMA GRE tunnels temporarily push GRE header that contain the per-packet NBMA destination on the skb via header ops early in xmit path. It is the later pulled before the real GRE header is constructed. The inner mac was thus set differently in nbma case: the GRE header has been pushed by neighbor layer, and mac header points to beginning of the temporary gre header (set by dev_queue_xmit). Now that the offloads expect mac header to point to the gre payload, fix the xmit patch to: - pull first the temporary gre header away - and reset mac header to point to gre payload This fixes tso to work again with nbma tunnels. Fixes: 14051f0452a2 ("gre: Use inner mac length when computing tunnel length" Signed-off-by: Timo Teräs <timo.teras@iki.fi> Cc: Tom Herbert <therbert@google.com> Cc: Alexander Duyck <alexander.h.duyck@redhat.com> --- Though, normally mac header does point to the begging of the hardware header. E.g. in ethernet case it's pointing to the ethernet header. But now in gre case it's instead pointing to the payload which seems counter-intuitive to me. But I guess tunnels are a bit of special case, and there's valid reasons to have it point to tunnel payload too. Applying this patch on top of the Tom's previous fix of 14051f0452a2 seems to now make my dmvpn scenario work again. So this should go to -stable too (atleast 3.14). net/ipv4/ip_gre.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)