[PATCH net] ip_tunnel: Fix dst ref-count.

[PATCH net] ip_tunnel: Fix dst ref-count.

[Pravin]

Commit 10ddceb22ba (ip_tunnel:multicast process cause panic due
to skb->_skb_refdst NULL pointer) removed dst-drop call from
ip-tunnel-recv.

Following commit reintroduce dst-drop and fix the original bug by
checking loopback packet before releasing dst.
Original bug: https://bugzilla.kernel.org/show_bug.cgi?id=70681

CC: Xin Long <lucien.xin@gmail.com>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
 net/ipv4/gre_demux.c      |    8 ++++++++
 net/ipv4/ip_tunnel.c      |    3 ---
 net/ipv4/ip_tunnel_core.c |    1 +
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
index 1863422f..250be74 100644
--- a/net/ipv4/gre_demux.c
+++ b/net/ipv4/gre_demux.c
@@ -182,6 +182,14 @@ static int gre_cisco_rcv(struct sk_buff *skb)
 	int i;
 	bool csum_err = false;
 
+#ifdef CONFIG_NET_IPGRE_BROADCAST
+	if (ipv4_is_multicast(ip_hdr(skb)->daddr)) {
+		/* Looped back packet, drop it! */
+		if (rt_is_output_route(skb_rtable(skb)))
+			goto drop;
+	}
+#endif
+
 	if (parse_gre_header(skb, &tpi, &csum_err) < 0)
 		goto drop;
 
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 78a89e6..a82a22d 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -416,9 +416,6 @@ int ip_tunnel_rcv(struct ip_tunnel *tunnel, struct sk_buff *skb,
 
 #ifdef CONFIG_NET_IPGRE_BROADCAST
 	if (ipv4_is_multicast(iph->daddr)) {
-		/* Looped back packet, drop it! */
-		if (rt_is_output_route(skb_rtable(skb)))
-			goto drop;
 		tunnel->dev->stats.multicast++;
 		skb->pkt_type = PACKET_BROADCAST;
 	}
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 6f847dd..8d69626 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -108,6 +108,7 @@ int iptunnel_pull_header(struct sk_buff *skb, int hdr_len, __be16 inner_proto)
 	nf_reset(skb);
 	secpath_reset(skb);
 	skb_clear_hash_if_not_l4(skb);
+	skb_dst_drop(skb);
 	skb->vlan_tci = 0;
 	skb_set_queue_mapping(skb, 0);
 	skb->pkt_type = PACKET_HOST;
-- 
1.7.9.5

Re: [PATCH net] ip_tunnel: Fix dst ref-count.

[David Miller]

From: Pravin <pshelar@nicira.com>
Date: Sun, 23 Mar 2014 22:06:36 -0700

> Commit 10ddceb22ba (ip_tunnel:multicast process cause panic due
> to skb->_skb_refdst NULL pointer) removed dst-drop call from
> ip-tunnel-recv.
> 
> Following commit reintroduce dst-drop and fix the original bug by
> checking loopback packet before releasing dst.
> Original bug: https://bugzilla.kernel.org/show_bug.cgi?id=70681
> 
> CC: Xin Long <lucien.xin@gmail.com>
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>

I don't understand why only the GRE tunnel implementation needs the
check, whereas ipip and sit seem to have the same code associated
paths.

Plus I think this is incredibly sloppy, making the implementations
have to attend to this detail.  Why the heck do we have a common
piece of infrastructure if we end up with all of this duplicated
code anyways?

Put this check somewhere common, so that we are guaranteed that the
issue is really fixed for all tunnel types, no matter what.

Thanks.

Re: [PATCH net] ip_tunnel: Fix dst ref-count.

[Pravin Shelar]

On Mon, Mar 24, 2014 at 12:24 PM, David Miller <davem@davemloft.net> wrote:
> From: Pravin <pshelar@nicira.com>
> Date: Sun, 23 Mar 2014 22:06:36 -0700
>
>> Commit 10ddceb22ba (ip_tunnel:multicast process cause panic due
>> to skb->_skb_refdst NULL pointer) removed dst-drop call from
>> ip-tunnel-recv.
>>
>> Following commit reintroduce dst-drop and fix the original bug by
>> checking loopback packet before releasing dst.
>> Original bug: https://bugzilla.kernel.org/show_bug.cgi?id=70681
>>
>> CC: Xin Long <lucien.xin@gmail.com>
>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>
> I don't understand why only the GRE tunnel implementation needs the
> check, whereas ipip and sit seem to have the same code associated
> paths.
>
Multicast support is for GRE. The code is always been protected by
CONFIG_NET_IPGRE_BROADCAST. So I do not understand how this applies to
other tunnel protocol. Can you point me to the code you are worried
about?

> Plus I think this is incredibly sloppy, making the implementations
> have to attend to this detail.  Why the heck do we have a common
> piece of infrastructure if we end up with all of this duplicated
> code anyways?
>
> Put this check somewhere common, so that we are guaranteed that the
> issue is really fixed for all tunnel types, no matter what.
>
> Thanks.

Re: [PATCH net] ip_tunnel: Fix dst ref-count.

[David Miller]

From: Pravin Shelar <pshelar@nicira.com>
Date: Mon, 24 Mar 2014 12:36:41 -0700

> On Mon, Mar 24, 2014 at 12:24 PM, David Miller <davem@davemloft.net> wrote:
>> From: Pravin <pshelar@nicira.com>
>> Date: Sun, 23 Mar 2014 22:06:36 -0700
>>
>>> Commit 10ddceb22ba (ip_tunnel:multicast process cause panic due
>>> to skb->_skb_refdst NULL pointer) removed dst-drop call from
>>> ip-tunnel-recv.
>>>
>>> Following commit reintroduce dst-drop and fix the original bug by
>>> checking loopback packet before releasing dst.
>>> Original bug: https://bugzilla.kernel.org/show_bug.cgi?id=70681
>>>
>>> CC: Xin Long <lucien.xin@gmail.com>
>>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>>
>> I don't understand why only the GRE tunnel implementation needs the
>> check, whereas ipip and sit seem to have the same code associated
>> paths.
>>
> Multicast support is for GRE. The code is always been protected by
> CONFIG_NET_IPGRE_BROADCAST. So I do not understand how this applies to
> other tunnel protocol. Can you point me to the code you are worried
> about?

Ok I'll take a second look at this, thanks.

Re: [PATCH net] ip_tunnel: Fix dst ref-count.

[David Miller]

From: Pravin Shelar <pshelar@nicira.com>
Date: Mon, 24 Mar 2014 12:36:41 -0700

> On Mon, Mar 24, 2014 at 12:24 PM, David Miller <davem@davemloft.net> wrote:
>> From: Pravin <pshelar@nicira.com>
>> Date: Sun, 23 Mar 2014 22:06:36 -0700
>>
>>> Commit 10ddceb22ba (ip_tunnel:multicast process cause panic due
>>> to skb->_skb_refdst NULL pointer) removed dst-drop call from
>>> ip-tunnel-recv.
>>>
>>> Following commit reintroduce dst-drop and fix the original bug by
>>> checking loopback packet before releasing dst.
>>> Original bug: https://bugzilla.kernel.org/show_bug.cgi?id=70681
>>>
>>> CC: Xin Long <lucien.xin@gmail.com>
>>> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
>>
>> I don't understand why only the GRE tunnel implementation needs the
>> check, whereas ipip and sit seem to have the same code associated
>> paths.
>>
> Multicast support is for GRE. The code is always been protected by
> CONFIG_NET_IPGRE_BROADCAST. So I do not understand how this applies to
> other tunnel protocol. Can you point me to the code you are worried
> about?

Ok, looks good, applied and queued up for -stable thanks!