diff mbox series

[2/2] vti6: do not check for ignore_df in order to update pmtu

Message ID 20180830125817.4567-2-cascardo@canonical.com
State Awaiting Upstream, archived
Delegated to: David Miller
Headers show
Series [1/2] xfrm6: call kfree_skb when skb is toobig | expand

Commit Message

Thadeu Lima de Souza Cascardo Aug. 30, 2018, 12:58 p.m. UTC
Before commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
and reporting on xmit"), skb was scrubbed before checking for ignore_df. The
scrubbing meant ignore_df was false, making the check irrelevant. Now that the
scrubbing happens after that, some packets might fail the checking and dst
will not have its pmtu updated.

Not only that, but too big skb will be potentially passed down to
__xfrm6_output, causing it to fail to transmit but not free the skb, causing a
leak of skb, and consequentially a leak of dst references.

After running pmtu.sh, that shows as failure to unregister devices in a namespace:

[  311.397671] unregister_netdevice: waiting for veth_b to become free. Usage count = 1

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 net/ipv6/ip6_vti.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steffen Klassert Aug. 31, 2018, 6:42 a.m. UTC | #1
On Thu, Aug 30, 2018 at 09:58:17AM -0300, Thadeu Lima de Souza Cascardo wrote:
> Before commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
> and reporting on xmit"), skb was scrubbed before checking for ignore_df. The
> scrubbing meant ignore_df was false, making the check irrelevant. Now that the
> scrubbing happens after that, some packets might fail the checking and dst
> will not have its pmtu updated.
> 
> Not only that, but too big skb will be potentially passed down to
> __xfrm6_output, causing it to fail to transmit but not free the skb, causing a
> leak of skb, and consequentially a leak of dst references.
> 
> After running pmtu.sh, that shows as failure to unregister devices in a namespace:
> 
> [  311.397671] unregister_netdevice: waiting for veth_b to become free. Usage count = 1
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>  net/ipv6/ip6_vti.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
> index c72ae3a4fe09..fbd3752ea587 100644
> --- a/net/ipv6/ip6_vti.c
> +++ b/net/ipv6/ip6_vti.c
> @@ -481,7 +481,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
>  	}
>  
>  	mtu = dst_mtu(dst);
> -	if (!skb->ignore_df && skb->len > mtu) {
> +	if (skb->len > mtu) {
>  		skb_dst_update_pmtu(skb, mtu);

The very same patch went already to the net tree two day ago:

commit 9f2895461439fda2801a7906fb4c5fb3dbb37a0a
vti6: remove !skb->ignore_df check from vti6_xmit()
diff mbox series

Patch

diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index c72ae3a4fe09..fbd3752ea587 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -481,7 +481,7 @@  vti6_xmit(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
 	}
 
 	mtu = dst_mtu(dst);
-	if (!skb->ignore_df && skb->len > mtu) {
+	if (skb->len > mtu) {
 		skb_dst_update_pmtu(skb, mtu);
 
 		if (skb->protocol == htons(ETH_P_IPV6)) {