Re: [PATCH v2 1/2] ip_tunnel: fix ip tunnel lookup in collect_md mode

[严海双]

> On 20 Jun 2017, at 2:08 AM, Pravin Shelar  wrote:
> 
> On Mon, Jun 19, 2017 at 6:13 AM, 严海双  
> wrote:
>> 
>> 
>>> On 19 Jun 2017, at 1:43 PM, Pravin Shelar  wrote:
>>> 
>>> On Fri, Jun 16, 2017 at 8:27 PM, Haishuang Yan
>>>  wrote:
 In collect_md mode, if the tun dev is down, it still can call
 ip_tunnel_rcv to receive on packets, and the rx statistics increase
 improperly.
 
 Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
 Cc: Pravin B Shelar 
 Signed-off-by: Haishuang Yan 
 
 ---
 Change since v2:
 * Fix wrong recipient addresss
 ---
 net/ipv4/ip_tunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
 index 0f1d876..a3caba1 100644
 --- a/net/ipv4/ip_tunnel.c
 +++ b/net/ipv4/ip_tunnel.c
 @@ -176,7 +176,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct 
 ip_tunnel_net *itn,
   return cand;
 
   t = rcu_dereference(itn->collect_md_tun);
 -   if (t)
 +   if (t && (t->dev->flags & IFF_UP))
   return t;
 
>>> It would be nice if we could increment drop count if tunnel device is not 
>>> up.
>>> 
>> Hi Pravin
>> 
>> I think it’s not necessary, for example as gre tunnel, if ipgre_rcv fails, 
>> it would trigger send an icmp unreachable
>> message:
>> 
>>if (ipgre_rcv(skb, , hdr_len) == PACKET_RCVD)
>>return 0;
>> 
>>icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);
>> 
>> Since the tunnel device didn’t touch the packets, so increase drop 
>> statistics is not necessary.
>> 
> icmp err packets are not reliable on all networks. device stats are
> much more convenient during debugging connectivity issues.
> 

Okay, if the tunnel device is not up, packets will transfer to fallback tunnel, 
and if 
the fallback device is up, the RX drops will be increased as expected:

gre0: flags=193  mtu 1476
inet 172.16.20.1  netmask 255.255.255.0
unspec 00-00-00-00-00-00-F0-00-00-00-00-00-00-00-00-00  txqueuelen 1000 
 (UNSPEC)
RX packets 105  bytes 4522 (4.4 KiB)
RX errors 0  dropped 105  overruns 0  frame 0
TX packets 0  bytes 0 (0.0 B)
TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0

My question is  whether we should increase the drops of original tunnel device 
instead of fallback device?

Re: [PATCH v2 1/2] ip_tunnel: fix ip tunnel lookup in collect_md mode

[Pravin Shelar]

On Mon, Jun 19, 2017 at 6:13 AM, 严海双  wrote:
>
>
>> On 19 Jun 2017, at 1:43 PM, Pravin Shelar  wrote:
>>
>> On Fri, Jun 16, 2017 at 8:27 PM, Haishuang Yan
>>  wrote:
>>> In collect_md mode, if the tun dev is down, it still can call
>>> ip_tunnel_rcv to receive on packets, and the rx statistics increase
>>> improperly.
>>>
>>> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
>>> Cc: Pravin B Shelar 
>>> Signed-off-by: Haishuang Yan 
>>>
>>> ---
>>> Change since v2:
>>>  * Fix wrong recipient addresss
>>> ---
>>> net/ipv4/ip_tunnel.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>>> index 0f1d876..a3caba1 100644
>>> --- a/net/ipv4/ip_tunnel.c
>>> +++ b/net/ipv4/ip_tunnel.c
>>> @@ -176,7 +176,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net 
>>> *itn,
>>>return cand;
>>>
>>>t = rcu_dereference(itn->collect_md_tun);
>>> -   if (t)
>>> +   if (t && (t->dev->flags & IFF_UP))
>>>return t;
>>>
>> It would be nice if we could increment drop count if tunnel device is not up.
>>
> Hi Pravin
>
> I think it’s not necessary, for example as gre tunnel, if ipgre_rcv fails, it 
> would trigger send an icmp unreachable
> message:
>
> if (ipgre_rcv(skb, , hdr_len) == PACKET_RCVD)
> return 0;
>
> icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);
>
> Since the tunnel device didn’t touch the packets, so increase drop statistics 
> is not necessary.
>
icmp err packets are not reliable on all networks. device stats are
much more convenient during debugging connectivity issues.

Re: [PATCH v2 1/2] ip_tunnel: fix ip tunnel lookup in collect_md mode

[严海双]

> On 19 Jun 2017, at 1:43 PM, Pravin Shelar  wrote:
> 
> On Fri, Jun 16, 2017 at 8:27 PM, Haishuang Yan
>  wrote:
>> In collect_md mode, if the tun dev is down, it still can call
>> ip_tunnel_rcv to receive on packets, and the rx statistics increase
>> improperly.
>> 
>> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
>> Cc: Pravin B Shelar 
>> Signed-off-by: Haishuang Yan 
>> 
>> ---
>> Change since v2:
>>  * Fix wrong recipient addresss
>> ---
>> net/ipv4/ip_tunnel.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
>> index 0f1d876..a3caba1 100644
>> --- a/net/ipv4/ip_tunnel.c
>> +++ b/net/ipv4/ip_tunnel.c
>> @@ -176,7 +176,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net 
>> *itn,
>>return cand;
>> 
>>t = rcu_dereference(itn->collect_md_tun);
>> -   if (t)
>> +   if (t && (t->dev->flags & IFF_UP))
>>return t;
>> 
> It would be nice if we could increment drop count if tunnel device is not up.
> 
Hi Pravin

I think it’s not necessary, for example as gre tunnel, if ipgre_rcv fails, it 
would trigger send an icmp unreachable
message:

if (ipgre_rcv(skb, , hdr_len) == PACKET_RCVD)
return 0;

icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);

Since the tunnel device didn’t touch the packets, so increase drop statistics 
is not necessary.

Thanks

Re: [PATCH v2 1/2] ip_tunnel: fix ip tunnel lookup in collect_md mode

[Pravin Shelar]

On Fri, Jun 16, 2017 at 8:27 PM, Haishuang Yan
 wrote:
> In collect_md mode, if the tun dev is down, it still can call
> ip_tunnel_rcv to receive on packets, and the rx statistics increase
> improperly.
>
> Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
> Cc: Pravin B Shelar 
> Signed-off-by: Haishuang Yan 
>
> ---
> Change since v2:
>   * Fix wrong recipient addresss
> ---
>  net/ipv4/ip_tunnel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 0f1d876..a3caba1 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -176,7 +176,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net 
> *itn,
> return cand;
>
> t = rcu_dereference(itn->collect_md_tun);
> -   if (t)
> +   if (t && (t->dev->flags & IFF_UP))
> return t;
>
It would be nice if we could increment drop count if tunnel device is not up.

[PATCH v2 1/2] ip_tunnel: fix ip tunnel lookup in collect_md mode

[Haishuang Yan]

In collect_md mode, if the tun dev is down, it still can call
ip_tunnel_rcv to receive on packets, and the rx statistics increase
improperly.

Fixes: 2e15ea390e6f ("ip_gre: Add support to collect tunnel metadata.")
Cc: Pravin B Shelar 
Signed-off-by: Haishuang Yan 

---
Change since v2:
  * Fix wrong recipient addresss
---
 net/ipv4/ip_tunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 0f1d876..a3caba1 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -176,7 +176,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net 
*itn,
return cand;
 
t = rcu_dereference(itn->collect_md_tun);
-   if (t)
+   if (t && (t->dev->flags & IFF_UP))
return t;
 
if (itn->fb_tunnel_dev && itn->fb_tunnel_dev->flags & IFF_UP)
-- 
1.8.3.1