[PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[Hannes Frederic Sowa]

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;
+	}
+
 	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);
-- 
1.8.3.1

Re: [PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[Hannes Frederic Sowa]

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

Re: [PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[Eric Dumazet]

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>

Re: [PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[Simon Horman]

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.

Re: [PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[Hannes Frederic Sowa]

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

Re: [PATCH ipsec 1/3] ipv6: wire up skb->encapsulation

[Simon Horman]

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.