Problematic commits in the ipsec tree

On Thu, Aug 22, 2013 at 12:47:24PM +0200, Steffen Klassert wrote:
> Hannes,
> 
> I have two problematic commits from you in the ipsec tree. The first one is:
> 
> commit 0ea9d5e3e (xfrm: introduce helper for safe determination of mtu)
> 
> This breakes pmtu discovery for IPv4 because now we use the device mtu
> instead of the reduced IPsec mtu in xfrm4_tunnel_check_size() if a IPv4
> socket is at the skb.

I am currently testing this following patch. It should restore old behavior
for ipv4 sockets.

Actually I would like to extend this check so that we only take the
dst_mtu(dst->path) in case of IP_PMTUDISC_PROBE. But that would be a
patch for ipsec-next. I was not sure if we always dispatch to xfrm_mtu
in this code-path.

> 
> The second is:
> 
> commit 844d48746 (xfrm: choose protocol family by skb protocol)
> 
> This breaks pmtu discovery for IPv6 too because skb->protocol can be null
> in __xfrm6_output().

I am currently checking if there are side-effects if we set skb->protocol in
raw sockets. This should solve this problem. Btw. this is also the case for
IPv4.

Hope to have tested patches later today.

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 Thu, Aug 22, 2013 at 03:53:42PM +0200, Hannes Frederic Sowa wrote:
> On Thu, Aug 22, 2013 at 12:47:24PM +0200, Steffen Klassert wrote:
> > Hannes,
> > 
> > I have two problematic commits from you in the ipsec tree. The first one is:
> > 
> > commit 0ea9d5e3e (xfrm: introduce helper for safe determination of mtu)
> > 
> > This breakes pmtu discovery for IPv4 because now we use the device mtu
> > instead of the reduced IPsec mtu in xfrm4_tunnel_check_size() if a IPv4
> > socket is at the skb.
> 
> I am currently testing this following patch. It should restore old behavior
> for ipv4 sockets.
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index ac5b025..65d3529 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -1730,8 +1730,6 @@ static inline int xfrm_skb_dst_mtu(struct sk_buff *skb)
>  
>  	if (sk && skb->protocol == htons(ETH_P_IPV6))
>  		return ip6_skb_dst_mtu(skb);
> -	else if (sk && skb->protocol == htons(ETH_P_IP))
> -		return ip_skb_dst_mtu(skb);
>  	return dst_mtu(skb_dst(skb));
>  }

This looks still fragile. xfrm_skb_dst_mtu() is called from
__xfrm6_output() and from xfrm4_tunnel_check_size().
We will have the same bug again as soon as somebody thinks that
it is save to call it from xfrm6_tunnel_check_size() too. So I
think it is better not to call it from xfrm4_tunnel_check_size().

Also, why do we need xfrm_skb_dst_mtu() at all? When it is called
from __xfrm6_output(), we know that this is IPv6. So we can call
ip6_skb_dst_mtu() directly as it was before your change.

--
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

Hello!

On Fri, Aug 23, 2013 at 10:58:07AM +0200, Steffen Klassert wrote:
> On Thu, Aug 22, 2013 at 03:53:42PM +0200, Hannes Frederic Sowa wrote:
> > On Thu, Aug 22, 2013 at 12:47:24PM +0200, Steffen Klassert wrote:
> > > Hannes,
> > > 
> > > I have two problematic commits from you in the ipsec tree. The first one is:
> > > 
> > > commit 0ea9d5e3e (xfrm: introduce helper for safe determination of mtu)
> > > 
> > > This breakes pmtu discovery for IPv4 because now we use the device mtu
> > > instead of the reduced IPsec mtu in xfrm4_tunnel_check_size() if a IPv4
> > > socket is at the skb.
> > 
> > I am currently testing this following patch. It should restore old behavior
> > for ipv4 sockets.
> > 
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > index ac5b025..65d3529 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -1730,8 +1730,6 @@ static inline int xfrm_skb_dst_mtu(struct sk_buff *skb)
> >  
> >  	if (sk && skb->protocol == htons(ETH_P_IPV6))
> >  		return ip6_skb_dst_mtu(skb);
> > -	else if (sk && skb->protocol == htons(ETH_P_IP))
> > -		return ip_skb_dst_mtu(skb);
> >  	return dst_mtu(skb_dst(skb));
> >  }
> 
> This looks still fragile. xfrm_skb_dst_mtu() is called from
> __xfrm6_output() and from xfrm4_tunnel_check_size().
> We will have the same bug again as soon as somebody thinks that
> it is save to call it from xfrm6_tunnel_check_size() too. So I
> think it is better not to call it from xfrm4_tunnel_check_size().

Hm, I don't think I can follow you completly here. I searched for allocations
of ipv6 skbs (where they originated from a socket) and checked these
allocations also initialize the skb->protocol field (the second patch).

I wonder if ip6_skb_dst_mtu was correct all along and if we should just
switch to dst_mtu(skb_dst(skb)) in all cases?

> Also, why do we need xfrm_skb_dst_mtu() at all? When it is called
> from __xfrm6_output(), we know that this is IPv6. So we can call
> ip6_skb_dst_mtu() directly as it was before your change.

The problem was that ip6_skb_dst_mtu dereferences the ipv6_pinfo socket to
check for IPV6_PMTUDISC_PROBE. If IPv4 is encapsulated in IPv6 and the packet
originated from the same host (sk != NULL) we call down to ip6_skb_dst_mtu.
This function checks that ipv6_pinfo is not NULL, so it worked by accident
currently.

My plan would be to check that all IPv4 skb allocations use the correct
skb->protocol, too, or if we can use the skb->inner_protocol field to mark
packets in the tunnel drivers from which protocol they originated.

Greetings,

  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 Fri, Aug 23, 2013 at 01:03:23PM +0200, Hannes Frederic Sowa wrote:
> Hello!
> 
> On Fri, Aug 23, 2013 at 10:58:07AM +0200, Steffen Klassert wrote:
> > On Thu, Aug 22, 2013 at 03:53:42PM +0200, Hannes Frederic Sowa wrote:
> > > On Thu, Aug 22, 2013 at 12:47:24PM +0200, Steffen Klassert wrote:
> > > > Hannes,
> > > > 
> > > > I have two problematic commits from you in the ipsec tree. The first one is:
> > > > 
> > > > commit 0ea9d5e3e (xfrm: introduce helper for safe determination of mtu)
> > > > 
> > > > This breakes pmtu discovery for IPv4 because now we use the device mtu
> > > > instead of the reduced IPsec mtu in xfrm4_tunnel_check_size() if a IPv4
> > > > socket is at the skb.
> > > 
> > > I am currently testing this following patch. It should restore old behavior
> > > for ipv4 sockets.
> > > 
> > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > > index ac5b025..65d3529 100644
> > > --- a/include/net/xfrm.h
> > > +++ b/include/net/xfrm.h
> > > @@ -1730,8 +1730,6 @@ static inline int xfrm_skb_dst_mtu(struct sk_buff *skb)
> > >  
> > >  	if (sk && skb->protocol == htons(ETH_P_IPV6))
> > >  		return ip6_skb_dst_mtu(skb);
> > > -	else if (sk && skb->protocol == htons(ETH_P_IP))
> > > -		return ip_skb_dst_mtu(skb);
> > >  	return dst_mtu(skb_dst(skb));
> > >  }
> > 
> > This looks still fragile. xfrm_skb_dst_mtu() is called from
> > __xfrm6_output() and from xfrm4_tunnel_check_size().
> > We will have the same bug again as soon as somebody thinks that
> > it is save to call it from xfrm6_tunnel_check_size() too. So I
> > think it is better not to call it from xfrm4_tunnel_check_size().
> 
> Hm, I don't think I can follow you completly here. I searched for allocations
> of ipv6 skbs (where they originated from a socket) and checked these
> allocations also initialize the skb->protocol field (the second patch).
> 
> I wonder if ip6_skb_dst_mtu was correct all along and if we should just
> switch to dst_mtu(skb_dst(skb)) in all cases?

Ok, I got it.

How about just checking in __xfrm6_output if we actually have a packet
originated from an IPv6 socket so that we only replace the original call to
ip6_skb_dst_mtu(skb)?

> 
> > Also, why do we need xfrm_skb_dst_mtu() at all? When it is called
> > from __xfrm6_output(), we know that this is IPv6. So we can call
> > ip6_skb_dst_mtu() directly as it was before your change.
> 
> The problem was that ip6_skb_dst_mtu dereferences the ipv6_pinfo socket to
> check for IPV6_PMTUDISC_PROBE. If IPv4 is encapsulated in IPv6 and the packet
> originated from the same host (sk != NULL) we call down to ip6_skb_dst_mtu.
> This function checks that ipv6_pinfo is not NULL, so it worked by accident
> currently.
> 
> My plan would be to check that all IPv4 skb allocations use the correct
> skb->protocol, too, or if we can use the skb->inner_protocol field to mark
> packets in the tunnel drivers from which protocol they originated.

Btw. this also let's us refer to the wrong mtu in case of an v4-mapped v6
socket transmitting over a address family changing tunnel over ipsec.

Greetings,

  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