| Message ID | 20160304070525.GA3347@gauss.secunet.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On 04/03/16 20:05, Steffen Klassert wrote: > On Wed, Feb 24, 2016 at 09:37:39PM +0000, Mark McKinstry wrote: >> On 19/02/16 01:19, Steffen Klassert wrote: >>> On Thu, Feb 18, 2016 at 01:40:00AM +0000, Mark McKinstry wrote: >>>> This patch fixes our issue, thanks. In our scenario the tunnel path MTU >>>> now gets updated so that subsequent large packets sent over the tunnel >>>> get fragmented correctly. >>> I've applied this patch to the ipsec tree now. >>> Thanks for testing! >> I spoke too soon. Upon further testing with this patch we have found it >> causes >> a skt buffer leak. This is problematic for us and can cause memory >> exhaustion in >> one of our test scenarios that has an IPv4 IPsec tunnel over a PPP link. > The patch below is what I plan to apply on top of the original patch. > > Subject: [PATCH] vti: Fix recource leeks on pmtu discovery > > A recent patch introduced pmtu handling directly in the > vti transmit routine. Unfortunately we now return without > releasing the dst_entry and freeing the sk_buff. This patch > fixes the issue. > > Fixes: 325b71fe0f57 ("vti: Add pmtu handling to vti_xmit.") > Reported-by: Mark McKinstry <Mark.McKinstry@alliedtelesis.co.nz> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > --- > net/ipv4/ip_vti.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c > index 6862305..2ea2b6e 100644 > --- a/net/ipv4/ip_vti.c > +++ b/net/ipv4/ip_vti.c > @@ -206,7 +206,8 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev, > else > icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu); > > - return -EMSGSIZE; > + dst_release(dst); > + goto tx_error; > } > > err = dst_output(tunnel->net, skb->sk, skb); Your patch adds a dst_release() call to my suggested fix, but this is problematic because the kfree_skb() call at tx_error already takes care of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > skb_release_head_state() > skb_dst_drop() > refdst_drop() > dst_release(). In our scenario your patch results in a negative refcount kernel warning being generated in dst_release() for every packet that is too big to go over the vti. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 09:52:05PM +0000, Mark McKinstry wrote: > Your patch adds a dst_release() call to my suggested fix, but this is > problematic because the kfree_skb() call at tx_error already takes care > of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > > skb_release_head_state() > skb_dst_drop() > > refdst_drop() > dst_release(). In our scenario your patch results in > a negative refcount kernel warning being generated in dst_release() for > every packet that is too big to go over the vti. Hm. I've just noticed that my pmtu test does not trigger this codepath, so I did not see the warning. Seems like we do the pmtu handling too late, it should happen before we do skb_dst_set(). Also skb_scrub_packet() resets skb->ignore_df, so checking ignore_df after skb_scrub_packet() does not make much sense. I'll send an updated version after some more testing. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" 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/ipv4/ip_vti.c b/net/ipv4/ip_vti.c index 6862305..2ea2b6e 100644 --- a/net/ipv4/ip_vti.c +++ b/net/ipv4/ip_vti.c @@ -206,7 +206,8 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct net_device *dev, else icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu); - return -EMSGSIZE; + dst_release(dst); + goto tx_error; } err = dst_output(tunnel->net, skb->sk, skb);