| Message ID | 20130817175116.GA7001@order.stressinduktion.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Simon! On Sat, Aug 17, 2013 at 07:51:16PM +0200, Hannes Frederic Sowa wrote: > When pushing a new header before current one call skb_reset_inner_headers > to record the position of the inner headers in the various ipv6 tunnel > protocols. > > We later need this to correctly identify the addresses needed to send > back an error in the xfrm layer. > > This change is safe, because skb->protocol is always checked before > dereferencing data from the inner protocol. > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > > This patch is based on Steffen Klassert's ipsec tree. > > net/ipv6/ip6_gre.c | 5 +++++ > net/ipv6/ip6_tunnel.c | 6 ++++++ > net/ipv6/sit.c | 5 +++++ > 3 files changed, 16 insertions(+) > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > index ecd6073..90747f1 100644 > --- a/net/ipv6/ip6_gre.c > +++ b/net/ipv6/ip6_gre.c > @@ -724,6 +724,11 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb, > ipv6_push_nfrag_opts(skb, &opt.ops, &proto, NULL); > } > > + if (likely(!skb->encapsulation)) { > + skb_reset_inner_headers(skb); > + skb->encapsulation = 1; > + } > + While doing these patches, I wondered how skb->inner_protocol will be used in future (you added it in 0d89d2035fe063461a5ddb609b2c12e7fb006e44 ("MPLS: Add limited GSO support")). Current use by tunnels seems safe to me, but I wondered how you would extend its use? Thanks, Hannes -- 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 Sat, 2013-08-17 at 19:51 +0200, Hannes Frederic Sowa wrote: > When pushing a new header before current one call skb_reset_inner_headers > to record the position of the inner headers in the various ipv6 tunnel > protocols. > > We later need this to correctly identify the addresses needed to send > back an error in the xfrm layer. > > This change is safe, because skb->protocol is always checked before > dereferencing data from the inner protocol. > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- Acked-by: Eric Dumazet <edumazet@googl.com> -- 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 Sat, Aug 17, 2013 at 08:07:38PM +0200, Hannes Frederic Sowa wrote: > Hi Simon! > > On Sat, Aug 17, 2013 at 07:51:16PM +0200, Hannes Frederic Sowa wrote: > > When pushing a new header before current one call skb_reset_inner_headers > > to record the position of the inner headers in the various ipv6 tunnel > > protocols. > > > > We later need this to correctly identify the addresses needed to send > > back an error in the xfrm layer. > > > > This change is safe, because skb->protocol is always checked before > > dereferencing data from the inner protocol. > > > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > > Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > --- > > > > This patch is based on Steffen Klassert's ipsec tree. > > > > net/ipv6/ip6_gre.c | 5 +++++ > > net/ipv6/ip6_tunnel.c | 6 ++++++ > > net/ipv6/sit.c | 5 +++++ > > 3 files changed, 16 insertions(+) > > > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > > index ecd6073..90747f1 100644 > > --- a/net/ipv6/ip6_gre.c > > +++ b/net/ipv6/ip6_gre.c > > @@ -724,6 +724,11 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb, > > ipv6_push_nfrag_opts(skb, &opt.ops, &proto, NULL); > > } > > > > + if (likely(!skb->encapsulation)) { > > + skb_reset_inner_headers(skb); > > + skb->encapsulation = 1; > > + } > > + > > While doing these patches, I wondered how skb->inner_protocol will be > used in future (you added it in 0d89d2035fe063461a5ddb609b2c12e7fb006e44 > ("MPLS: Add limited GSO support")). Current use by tunnels seems safe to > me, but I wondered how you would extend its use? Hi, I must confess that I'm not entirely sure that I understand the question. The purpose of adding inner_protocol was to allow GSO of MPLS as MPLS is rather special and does not include the inner protocol anywhere in the packet. So this allows it to be known when GSO occurs if it was previously known - I believe the sole use-case here is if a packet wasn't MPLS when received but then turned into MPLS by Open vSwtich. I'm not aware of any other encapsulation/tunnelling protocols which share MPLS's unusual property of not including the inner-protocol in the packet, but if they exist then skb->inner_protocol may be useful when adding GSO support for them. Beyond that I do not envisage any extension of the use of skb->inner_protocol at this point. But I feel that somehow I am missing the point of your question. -- 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 Sun, Aug 18, 2013 at 02:24:16PM +1000, Simon Horman wrote: > On Sat, Aug 17, 2013 at 08:07:38PM +0200, Hannes Frederic Sowa wrote: > > Hi Simon! > > > > On Sat, Aug 17, 2013 at 07:51:16PM +0200, Hannes Frederic Sowa wrote: > > > When pushing a new header before current one call skb_reset_inner_headers > > > to record the position of the inner headers in the various ipv6 tunnel > > > protocols. > > > > > > We later need this to correctly identify the addresses needed to send > > > back an error in the xfrm layer. > > > > > > This change is safe, because skb->protocol is always checked before > > > dereferencing data from the inner protocol. > > > > > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > > > Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > > > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > --- > > > > > > This patch is based on Steffen Klassert's ipsec tree. > > > > > > net/ipv6/ip6_gre.c | 5 +++++ > > > net/ipv6/ip6_tunnel.c | 6 ++++++ > > > net/ipv6/sit.c | 5 +++++ > > > 3 files changed, 16 insertions(+) > > > > > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > > > index ecd6073..90747f1 100644 > > > --- a/net/ipv6/ip6_gre.c > > > +++ b/net/ipv6/ip6_gre.c > > > @@ -724,6 +724,11 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb, > > > ipv6_push_nfrag_opts(skb, &opt.ops, &proto, NULL); > > > } > > > > > > + if (likely(!skb->encapsulation)) { > > > + skb_reset_inner_headers(skb); > > > + skb->encapsulation = 1; > > > + } > > > + > > > > While doing these patches, I wondered how skb->inner_protocol will be > > used in future (you added it in 0d89d2035fe063461a5ddb609b2c12e7fb006e44 > > ("MPLS: Add limited GSO support")). Current use by tunnels seems safe to > > me, but I wondered how you would extend its use? > > Hi, > > I must confess that I'm not entirely sure that I understand the question. Sorry to be not clear enough. I try to present my envisioned use case for inner_protocol: I do think there may be some other corner cases when reporting back errors to a local socket when multiple tunnels+ipsecs are stacked into each other. The first encapsulation sets skb->inner_network_header, so that we can later extract the needed information for the error generation via inner_ip{v6,}_hdr. It would be handy here to use inner_protocol to mark the type of inner_network_header in advance (all this is because, even if we still have a reference to the local socket and know its af, it could also have emitted an IPv4 frame, so we have to jump to the ipv4 error handling path). Thanks, Hannes -- 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 Sun, Aug 18, 2013 at 01:00:38PM +0200, Hannes Frederic Sowa wrote: > On Sun, Aug 18, 2013 at 02:24:16PM +1000, Simon Horman wrote: > > On Sat, Aug 17, 2013 at 08:07:38PM +0200, Hannes Frederic Sowa wrote: > > > Hi Simon! > > > > > > On Sat, Aug 17, 2013 at 07:51:16PM +0200, Hannes Frederic Sowa wrote: > > > > When pushing a new header before current one call skb_reset_inner_headers > > > > to record the position of the inner headers in the various ipv6 tunnel > > > > protocols. > > > > > > > > We later need this to correctly identify the addresses needed to send > > > > back an error in the xfrm layer. > > > > > > > > This change is safe, because skb->protocol is always checked before > > > > dereferencing data from the inner protocol. > > > > > > > > Cc: Steffen Klassert <steffen.klassert@secunet.com> > > > > Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > > > > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > > > > --- > > > > > > > > This patch is based on Steffen Klassert's ipsec tree. > > > > > > > > net/ipv6/ip6_gre.c | 5 +++++ > > > > net/ipv6/ip6_tunnel.c | 6 ++++++ > > > > net/ipv6/sit.c | 5 +++++ > > > > 3 files changed, 16 insertions(+) > > > > > > > > diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c > > > > index ecd6073..90747f1 100644 > > > > --- a/net/ipv6/ip6_gre.c > > > > +++ b/net/ipv6/ip6_gre.c > > > > @@ -724,6 +724,11 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb, > > > > ipv6_push_nfrag_opts(skb, &opt.ops, &proto, NULL); > > > > } > > > > > > > > + if (likely(!skb->encapsulation)) { > > > > + skb_reset_inner_headers(skb); > > > > + skb->encapsulation = 1; > > > > + } > > > > + > > > > > > While doing these patches, I wondered how skb->inner_protocol will be > > > used in future (you added it in 0d89d2035fe063461a5ddb609b2c12e7fb006e44 > > > ("MPLS: Add limited GSO support")). Current use by tunnels seems safe to > > > me, but I wondered how you would extend its use? > > > > Hi, > > > > I must confess that I'm not entirely sure that I understand the question. > > Sorry to be not clear enough. I try to present my envisioned use case for > inner_protocol: > > I do think there may be some other corner cases when reporting back > errors to a local socket when multiple tunnels+ipsecs are stacked into > each other. The first encapsulation sets skb->inner_network_header, so > that we can later extract the needed information for the error generation > via inner_ip{v6,}_hdr. It would be handy here to use inner_protocol to > mark the type of inner_network_header in advance (all this is because, > even if we still have a reference to the local socket and know its af, > it could also have emitted an IPv4 frame, so we have to jump to the ipv4 > error handling path). That sounds reasonable to me and I don't think it would interfere with the current usage or vice versa. -- 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/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c index ecd6073..90747f1 100644 --- a/net/ipv6/ip6_gre.c +++ b/net/ipv6/ip6_gre.c @@ -724,6 +724,11 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb, ipv6_push_nfrag_opts(skb, &opt.ops, &proto, NULL); } + if (likely(!skb->encapsulation)) { + skb_reset_inner_headers(skb); + skb->encapsulation = 1; + } + skb_push(skb, gre_hlen); skb_reset_network_header(skb); skb_set_transport_header(skb, sizeof(*ipv6h)); diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c index 1e55866..46ba243 100644 --- a/net/ipv6/ip6_tunnel.c +++ b/net/ipv6/ip6_tunnel.c @@ -1027,6 +1027,12 @@ static int ip6_tnl_xmit2(struct sk_buff *skb, init_tel_txopt(&opt, encap_limit); ipv6_push_nfrag_opts(skb, &opt.ops, &proto, NULL); } + + if (likely(!skb->encapsulation)) { + skb_reset_inner_headers(skb); + skb->encapsulation = 1; + } + skb_push(skb, sizeof(struct ipv6hdr)); skb_reset_network_header(skb); ipv6h = ipv6_hdr(skb); diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c index a3437a4..fbfc5a8 100644 --- a/net/ipv6/sit.c +++ b/net/ipv6/sit.c @@ -888,6 +888,11 @@ static netdev_tx_t ipip6_tunnel_xmit(struct sk_buff *skb, ttl = iph6->hop_limit; tos = INET_ECN_encapsulate(tos, ipv6_get_dsfield(iph6)); + if (likely(!skb->encapsulation)) { + skb_reset_inner_headers(skb); + skb->encapsulation = 1; + } + err = iptunnel_xmit(dev_net(dev), rt, skb, fl4.saddr, fl4.daddr, IPPROTO_IPV6, tos, ttl, df); iptunnel_xmit_stats(err, &dev->stats, dev->tstats);
When pushing a new header before current one call skb_reset_inner_headers to record the position of the inner headers in the various ipv6 tunnel protocols. We later need this to correctly identify the addresses needed to send back an error in the xfrm layer. This change is safe, because skb->protocol is always checked before dereferencing data from the inner protocol. Cc: Steffen Klassert <steffen.klassert@secunet.com> Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- This patch is based on Steffen Klassert's ipsec tree. net/ipv6/ip6_gre.c | 5 +++++ net/ipv6/ip6_tunnel.c | 6 ++++++ net/ipv6/sit.c | 5 +++++ 3 files changed, 16 insertions(+)