[PATCH v3 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[PATCH v3 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Haishuang Yan]

When ip_tunnel_rcv fails, the tun_dst won't be freed, so call
dst_release to free it in error code path.

CC: Pravin B Shelar <pshelar@nicira.com>
Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>

---
Changes in v2:
  - Add the the missing Fixes information
Changes in v3:
  - Free tun_dst from error code path
---
 net/ipv4/ip_tunnel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index b878ecb..0f1d876 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -446,6 +446,8 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
 	return 0;
 
 drop:
+	if (tun_dst)
+		dst_release((struct dst_entry *)tun_dst);
 	kfree_skb(skb);
 	return 0;
 }
-- 
1.8.3.1

[PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Haishuang Yan]

When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
dst_release to free it in error code path.

CC: Alexei Starovoitov <ast@fb.com>
Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>

---
Changes in v2:
  - Add the the missing Fixes information
Changes in v3:
  - Free tun_dst from error code path
---
 net/ipv6/ip6_tunnel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 9b37f97..ef99d59 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -859,6 +859,8 @@ static int __ip6_tnl_rcv(struct ip6_tnl *tunnel, struct sk_buff *skb,
 	return 0;
 
 drop:
+	if (tun_dst)
+		dst_release((struct dst_entry *)tun_dst);
 	kfree_skb(skb);
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Alexei Starovoitov]

On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
> dst_release to free it in error code path.
> 
> CC: Alexei Starovoitov <ast@fb.com>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>

I don't get it. Why did you send another version of the patch?
What was wrong with previous approach that myself and Eric acked?

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Eric Dumazet]

On Wed, Jun 7, 2017 at 9:38 PM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
>> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
>> dst_release to free it in error code path.
>>
>> CC: Alexei Starovoitov <ast@fb.com>
>> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>
> I don't get it. Why did you send another version of the patch?
> What was wrong with previous approach that myself and Eric acked?
>

Answer lies in Pravin feedback on v2 of the patch on ipv4 side.

Re: [PATCH v3 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Eric Dumazet]

On Thu, 2017-06-08 at 12:32 +0800, Haishuang Yan wrote:
> When ip_tunnel_rcv fails, the tun_dst won't be freed, so call
> dst_release to free it in error code path.
> 
> CC: Pravin B Shelar <pshelar@nicira.com>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> 
> ---
> Changes in v2:
>   - Add the the missing Fixes information
> Changes in v3:
>   - Free tun_dst from error code path
> ---
>  net/ipv4/ip_tunnel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index b878ecb..0f1d876 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -446,6 +446,8 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
>  	return 0;
>  
>  drop:
> +	if (tun_dst)
> +		dst_release((struct dst_entry *)tun_dst);
>  	kfree_skb(skb);
>  	return 0;
>  }

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[Alexei Starovoitov]

On Thu, Jun 08, 2017 at 12:56:58PM +0800, 严海双 wrote:
> 
> > On 8 Jun 2017, at 12:38 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > 
> > On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
> >> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
> >> dst_release to free it in error code path.
> >> 
> >> CC: Alexei Starovoitov <ast@fb.com>
> >> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> >> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> > 
> > I don't get it. Why did you send another version of the patch?
> > What was wrong with previous approach that myself and Eric acked?
> > 
> > 
> 
> Sorry for your confusing, because Pravin Shelar give a feedback in ipv4 patch, see below:

hmm. right.
Then it raises the question: How did you test this and previous patch?

since previous version was sort-of fixing the bug, but completely
breaking the logic...

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[严海双]

> On 8 Jun 2017, at 1:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> 
> On Thu, Jun 08, 2017 at 12:56:58PM +0800, 严海双 wrote:
>> 
>>> On 8 Jun 2017, at 12:38 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
>>>> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
>>>> dst_release to free it in error code path.
>>>> 
>>>> CC: Alexei Starovoitov <ast@fb.com>
>>>> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
>>>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>>> 
>>> I don't get it. Why did you send another version of the patch?
>>> What was wrong with previous approach that myself and Eric acked?
>>> 
>>> 
>> 
>> Sorry for your confusing, because Pravin Shelar give a feedback in ipv4 patch, see below:
> 
> hmm. right.
> Then it raises the question: How did you test this and previous patch?
> 
> since previous version was sort-of fixing the bug, but completely
> breaking the logic...
> 
> 

Sorry for my previous fault, I tried to fix this problem in theory without testing carefully.
I have tested the latest patches, it works ok now.

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[David Miller]

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Wed, 7 Jun 2017 22:00:43 -0700

> On Thu, Jun 08, 2017 at 12:56:58PM +0800, 严海双 wrote:
>> 
>> > On 8 Jun 2017, at 12:38 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>> > 
>> > On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
>> >> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
>> >> dst_release to free it in error code path.
>> >> 
>> >> CC: Alexei Starovoitov <ast@fb.com>
>> >> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
>> >> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>> > 
>> > I don't get it. Why did you send another version of the patch?
>> > What was wrong with previous approach that myself and Eric acked?
>> > 
>> > 
>> 
>> Sorry for your confusing, because Pravin Shelar give a feedback in ipv4 patch, see below:
> 
> hmm. right.
> Then it raises the question: How did you test this and previous patch?
> 
> since previous version was sort-of fixing the bug, but completely
> breaking the logic...

Anyone who posts new patches this patch isn't testing things thoroughly at all.

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[David Miller]

From: 严海双 <yanhaishuang@cmss.chinamobile.com>
Date: Thu, 8 Jun 2017 15:33:58 +0800

>> On 8 Jun 2017, at 1:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>> 
>> On Thu, Jun 08, 2017 at 12:56:58PM +0800, 严海双 wrote:
>>> 
>>>> On 8 Jun 2017, at 12:38 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>> 
>>>> On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
>>>>> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
>>>>> dst_release to free it in error code path.
>>>>> 
>>>>> CC: Alexei Starovoitov <ast@fb.com>
>>>>> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
>>>>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>>>> 
>>>> I don't get it. Why did you send another version of the patch?
>>>> What was wrong with previous approach that myself and Eric acked?
>>>> 
>>>> 
>>> 
>>> Sorry for your confusing, because Pravin Shelar give a feedback in ipv4 patch, see below:
>> 
>> hmm. right.
>> Then it raises the question: How did you test this and previous patch?
>> 
>> since previous version was sort-of fixing the bug, but completely
>> breaking the logic...
>> 
>> 
> 
> Sorry for my previous fault, I tried to fix this problem in theory without testing carefully.
> I have tested the latest patches, it works ok now.

This does not instill a lot of confidence in us.

I want someone else to test these patches, then you can resubmit them
with proper Tested-by: tags added, since you thought it was OK to submit
a patch without testing in the first place.

Re: [PATCH v3 1/2] ip_tunnel: fix potential issue in ip_tunnel_rcv

[Pravin Shelar]

On Wed, Jun 7, 2017 at 9:32 PM, Haishuang Yan
<yanhaishuang@cmss.chinamobile.com> wrote:
> When ip_tunnel_rcv fails, the tun_dst won't be freed, so call
> dst_release to free it in error code path.
>
> CC: Pravin B Shelar <pshelar@nicira.com>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>
> ---
> Changes in v2:
>   - Add the the missing Fixes information
> Changes in v3:
>   - Free tun_dst from error code path
> ---

Acked-by: Pravin B Shelar <pshelar@ovn.org>

Re: [PATCH v3 2/2] ip6_tunnel: fix potential issue in __ip6_tnl_rcv

[严海双]

> On 8 Jun 2017, at 9:59 PM, David Miller <davem@davemloft.net> wrote:
> 
> From: 严海双 <yanhaishuang@cmss.chinamobile.com>
> Date: Thu, 8 Jun 2017 15:33:58 +0800
> 
>>> On 8 Jun 2017, at 1:00 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> 
>>> On Thu, Jun 08, 2017 at 12:56:58PM +0800, 严海双 wrote:
>>>> 
>>>>> On 8 Jun 2017, at 12:38 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>>>> 
>>>>> On Thu, Jun 08, 2017 at 12:32:44PM +0800, Haishuang Yan wrote:
>>>>>> When __ip6_tnl_rcv fails, the tun_dst won't be freed, so call
>>>>>> dst_release to free it in error code path.
>>>>>> 
>>>>>> CC: Alexei Starovoitov <ast@fb.com>
>>>>>> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
>>>>>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>>>>> 
>>>>> I don't get it. Why did you send another version of the patch?
>>>>> What was wrong with previous approach that myself and Eric acked?
>>>>> 
>>>>> 
>>>> 
>>>> Sorry for your confusing, because Pravin Shelar give a feedback in ipv4 patch, see below:
>>> 
>>> hmm. right.
>>> Then it raises the question: How did you test this and previous patch?
>>> 
>>> since previous version was sort-of fixing the bug, but completely
>>> breaking the logic...
>>> 
>>> 
>> 
>> Sorry for my previous fault, I tried to fix this problem in theory without testing carefully.
>> I have tested the latest patches, it works ok now.
> 
> This does not instill a lot of confidence in us.
> 
> I want someone else to test these patches, then you can resubmit them
> with proper Tested-by: tags added, since you thought it was OK to submit
> a patch without testing in the first place.

Ok, thanks.