[PATCH 1/2] xfrm6: call kfree_skb when skb is toobig

[PATCH 1/2] xfrm6: call kfree_skb when skb is toobig

[Thadeu Lima de Souza Cascardo]

After commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
and reporting on xmit"), some too big skbs might 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

The fix is to call kfree_skb in case of transmit failures.

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

diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index 5959ce9620eb..6a74080005cf 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -170,9 +170,11 @@ static int __xfrm6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 	if (toobig && xfrm6_local_dontfrag(skb)) {
 		xfrm6_local_rxpmtu(skb, mtu);
+		kfree_skb(skb);
 		return -EMSGSIZE;
 	} else if (!skb->ignore_df && toobig && skb->sk) {
 		xfrm_local_error(skb, mtu);
+		kfree_skb(skb);
 		return -EMSGSIZE;
 	}
 
-- 
2.17.1

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

[Thadeu Lima de Souza Cascardo]

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);
 
 		if (skb->protocol == htons(ETH_P_IPV6)) {
-- 
2.17.1

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

[Steffen Klassert]

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

Re: [PATCH 1/2] xfrm6: call kfree_skb when skb is toobig

[Sabrina Dubroca]

2018-08-30, 09:58:16 -0300, Thadeu Lima de Souza Cascardo wrote:
> After commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
> and reporting on xmit"), some too big skbs might 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
> 
> The fix is to call kfree_skb in case of transmit failures.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

I was about to post the same patch. Arguably, the commit introducing
this bug is the one that added those "return -EMSGSIZE" to
__xfrm6_output without freeing.

Either way, it's missing a Fixes: tag, which should be one of those,
or both:

Fixes: d6990976af7c ("vti6: fix PMTU caching and reporting on xmit")
Fixes: dd767856a36e ("xfrm6: Don't call icmpv6_send on local error")

-- 
Sabrina

Re: [PATCH 1/2] xfrm6: call kfree_skb when skb is toobig

[Steffen Klassert]

On Thu, Aug 30, 2018 at 03:23:11PM +0200, Sabrina Dubroca wrote:
> 2018-08-30, 09:58:16 -0300, Thadeu Lima de Souza Cascardo wrote:
> > After commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
> > and reporting on xmit"), some too big skbs might 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
> > 
> > The fix is to call kfree_skb in case of transmit failures.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

Good catch!

> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> 
> I was about to post the same patch. Arguably, the commit introducing
> this bug is the one that added those "return -EMSGSIZE" to
> __xfrm6_output without freeing.
> 
> Either way, it's missing a Fixes: tag, which should be one of those,
> or both:
> 
> Fixes: d6990976af7c ("vti6: fix PMTU caching and reporting on xmit")
> Fixes: dd767856a36e ("xfrm6: Don't call icmpv6_send on local error")

This bug can be triggered even without vti6, so the correct
Fixes tag would be the latter.

Thadeu, please resend this one with the Fixes tag.

Thanks!

[PATCH v2] xfrm6: call kfree_skb when skb is toobig

[Thadeu Lima de Souza Cascardo]

After commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
and reporting on xmit"), some too big skbs might 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

The fix is to call kfree_skb in case of transmit failures.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Fixes: dd767856a36e ("xfrm6: Don't call icmpv6_send on local error")
---
 net/ipv6/xfrm6_output.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index 5959ce9620eb..6a74080005cf 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -170,9 +170,11 @@ static int __xfrm6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 	if (toobig && xfrm6_local_dontfrag(skb)) {
 		xfrm6_local_rxpmtu(skb, mtu);
+		kfree_skb(skb);
 		return -EMSGSIZE;
 	} else if (!skb->ignore_df && toobig && skb->sk) {
 		xfrm_local_error(skb, mtu);
+		kfree_skb(skb);
 		return -EMSGSIZE;
 	}
 
-- 
2.17.1

Re: [PATCH v2] xfrm6: call kfree_skb when skb is toobig

[Steffen Klassert]

On Fri, Aug 31, 2018 at 08:38:49AM -0300, Thadeu Lima de Souza Cascardo wrote:
> After commit d6990976af7c5d8f55903bfb4289b6fb030bf754 ("vti6: fix PMTU caching
> and reporting on xmit"), some too big skbs might 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
> 
> The fix is to call kfree_skb in case of transmit failures.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
> Fixes: dd767856a36e ("xfrm6: Don't call icmpv6_send on local error")

Applied, thanks a lot!