From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753811AbdJaQpS (ORCPT <rfc822;w@1wt.eu>);
        Tue, 31 Oct 2017 12:45:18 -0400
Received: from mail-lf0-f45.google.com ([209.85.215.45]:55958 "EHLO
        mail-lf0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753298AbdJaQpQ (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 31 Oct 2017 12:45:16 -0400
X-Google-Smtp-Source: ABhQp+QrOQ3MRk3T/ASp4LiaG1B2PUDY2RvorkM1tobKYQ97/4carnLyNAJA9hsJ6B1lyBD2/DCNlA==
Date: Tue, 31 Oct 2017 17:44:51 +0100
From: Stephen Hemminger <stephen@networkplumber.org>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: netdev@vger.kernel.org, devel@linuxdriverproject.org,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with
 RCU
Message-ID: <20171031174451.096978cd@shemminger-XPS-13-9360>
In-Reply-To: <20171031134204.15287-3-vkuznets@redhat.com>
References: <20171031134204.15287-1-vkuznets@redhat.com>
        <20171031134204.15287-3-vkuznets@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, 31 Oct 2017 14:42:02 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> @@ -2002,7 +2002,9 @@ static int netvsc_probe(struct hv_device *dev,
>  	device_info.recv_sections = NETVSC_DEFAULT_RX;
>  	device_info.recv_section_size = NETVSC_RECV_SECTION_SIZE;
>  
> +	rtnl_lock();
>  	nvdev = rndis_filter_device_add(dev, &device_info);
> +	rtnl_unlock();

rtnl is not necessary here. probe can not be bothered by other changes.

> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -402,20 +402,27 @@ int rndis_filter_receive(struct net_device *ndev,
>  			 void *data, u32 buflen)
>  {
>  	struct net_device_context *net_device_ctx = netdev_priv(ndev);
> -	struct rndis_device *rndis_dev = net_dev->extension;
> +	struct rndis_device *rndis_dev;
>  	struct rndis_message *rndis_msg = data;
> +	int ret = 0;
> +
> +	rcu_read_lock_bh();
> +
> +	rndis_dev = rcu_dereference_bh(net_dev->extension);

filter_receive is already called only from NAPI only and has RCU lock and soft
irq disabled. This is not necessary.

> -	net_dev->extension = NULL;
> +	rcu_assign_pointer(net_dev->extension, NULL);
> +
> +	synchronize_rcu();

rcu_assign_pointer with NULL is never a good idea.
And synchronize_rcu is slow. Since net_device is already protected
by RCU (for deletion) it should not be necessary.


Thank you for trying to address these races. But it should be
done carefully not by just slapping RCU everywhere.