Subject: Re: [PATCH v3 2/2] ip6_tunnel: fix ip6 tunnel lookup in collect_md
 mode
To: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>,
        "David S. Miller" <davem@davemloft.net>
References: <1504514174-14958-1-git-send-email-yanhaishuang@cmss.chinamobile.com>
 <1504514174-14958-2-git-send-email-yanhaishuang@cmss.chinamobile.com>
CC: <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
From: Alexei Starovoitov <ast@fb.com>
Message-ID: <ece190cd-b8a1-225a-84c6-80ca2b46a44d@fb.com>
Date: Tue, 5 Sep 2017 20:14:30 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <1504514174-14958-2-git-send-email-yanhaishuang@cmss.chinamobile.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Sep 2017 03:14:33.5884 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR15MB0970
X-OriginatorOrg: fb.com
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-06_01:,,
 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1383
Lines: 41

On 9/4/17 1:36 AM, Haishuang Yan wrote:
> In collect_md mode, if the tun dev is down, it still can call
> __ip6_tnl_rcv to receive on packets, and the rx statistics increase
> improperly.
>
> Fixes: 8d79266bc48c ("ip6_tunnel: add collect_md mode to IPv6 tunnels")
> Cc: Alexei Starovoitov <ast@fb.com>
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
>
> ---
> Change since v3:
>   * Increment rx_dropped if tunnel device is not up, suggested by
>   Pravin B Shelar
>   * Fix wrong recipient address
> ---
>  net/ipv6/ip6_tunnel.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 10a693a..e91d3b6 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -171,8 +171,11 @@ static struct net_device_stats *ip6_get_stats(struct net_device *dev)
>  	}
>
>  	t = rcu_dereference(ip6n->collect_md_tun);
> -	if (t)
> -		return t;
> +	if (t) {
> +		if (t->dev->flags & IFF_UP)
> +			return t;
> +		t->dev->stats.rx_dropped++;
> +	}

Why increment the stats only for this drop case?
There are plenty of other conditions where packet
will be dropped in ip6 tunnel. I think it's important
to present consistent behavior to the users,
so I'd increment drop stats either for all drop cases
or for none. And today it's none.
The ! IFF_UP case should probably be return NULL too.