From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753904AbdJaROE (ORCPT ); Tue, 31 Oct 2017 13:14:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46272 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753540AbdJaROC (ORCPT ); Tue, 31 Oct 2017 13:14:02 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 869AD883B6 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=vkuznets@redhat.com From: Vitaly Kuznetsov To: Stephen Hemminger Cc: netdev@vger.kernel.org, devel@linuxdriverproject.org, Haiyang Zhang , Stephen Hemminger , linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU References: <20171031134204.15287-1-vkuznets@redhat.com> <20171031134204.15287-3-vkuznets@redhat.com> <20171031174451.096978cd@shemminger-XPS-13-9360> Date: Tue, 31 Oct 2017 18:13:58 +0100 In-Reply-To: <20171031174451.096978cd@shemminger-XPS-13-9360> (Stephen Hemminger's message of "Tue, 31 Oct 2017 17:44:51 +0100") Message-ID: <87shdzw889.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 31 Oct 2017 17:14:02 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Stephen Hemminger writes: > On Tue, 31 Oct 2017 14:42:02 +0100 > Vitaly Kuznetsov 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. > Yes, this is only to support rtnl_dereference() down the stack. >> --- 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. > I thought we don't care that much about the speed of this patch as rndis_filter_device_remove() is only called on device remove/mtu change/... and we need to interact with the host -- and this is already slow. > Thank you for trying to address these races. But it should be > done carefully not by just slapping RCU everywhere. Ok, I may have missed something. I'll try reproducing the crash and finding a better fine-grained solution. Thanks for the feedback! -- Vitaly From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vitaly Kuznetsov Subject: Re: [PATCH net-next 2/4] hv_netvsc: protect nvdev->extension with RCU Date: Tue, 31 Oct 2017 18:13:58 +0100 Message-ID: <87shdzw889.fsf@vitty.brq.redhat.com> References: <20171031134204.15287-1-vkuznets@redhat.com> <20171031134204.15287-3-vkuznets@redhat.com> <20171031174451.096978cd@shemminger-XPS-13-9360> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Haiyang Zhang , Stephen Hemminger , linux-kernel@vger.kernel.org, devel@linuxdriverproject.org To: Stephen Hemminger Return-path: In-Reply-To: <20171031174451.096978cd@shemminger-XPS-13-9360> (Stephen Hemminger's message of "Tue, 31 Oct 2017 17:44:51 +0100") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" List-Id: netdev.vger.kernel.org Stephen Hemminger writes: > On Tue, 31 Oct 2017 14:42:02 +0100 > Vitaly Kuznetsov 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. > Yes, this is only to support rtnl_dereference() down the stack. >> --- 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. > I thought we don't care that much about the speed of this patch as rndis_filter_device_remove() is only called on device remove/mtu change/... and we need to interact with the host -- and this is already slow. > Thank you for trying to address these races. But it should be > done carefully not by just slapping RCU everywhere. Ok, I may have missed something. I'll try reproducing the crash and finding a better fine-grained solution. Thanks for the feedback! -- Vitaly