From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 21E1FC4338F for ; Tue, 24 Aug 2021 12:25:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0508B61371 for ; Tue, 24 Aug 2021 12:25:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237257AbhHXMZv (ORCPT ); Tue, 24 Aug 2021 08:25:51 -0400 Received: from mail.kernel.org ([198.145.29.99]:58174 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230132AbhHXMZv (ORCPT ); Tue, 24 Aug 2021 08:25:51 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id B73016127B; Tue, 24 Aug 2021 12:25:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1629807907; bh=Edj68RKjcXX2r4keA5K6DEuxfBNmsQDCDQXdGfLJ09I=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=eKhRQQn91hAhG6loFGwgvUTfuk90J2t693MaoSzkx7rKuyyuInsQaMgZU5awAgMiz yFcJR6cf0jhhpE3vB4dGD9mLksEZzdr9I43oJSix1x0HOnWbmOZno+jd5ADXAhOGGw 7VBsLN+b3JDEPIAjmTsNi7pKwnFm4uEa/5d+I/GkW3Y2DHZWzSNnsAGKr2+6tje504 gdzarHSnOqsLqPhNt7yJfwf9JTDdZiL33VNsYfFYESXJXz9WctTg+j1c1a6yEUbDsV 61jwiPYWc7G6JK/UAycvuK3BgJF/MSVl2HltSH/g+u4GP57GgpVtVUtg+V/PoVe45Y CkH0VXc58XDaQ== Date: Tue, 24 Aug 2021 07:25:04 -0500 From: Bjorn Helgaas To: longli@linuxonhyperv.com Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org, Long Li , "K. Y. Srinivasan" , Haiyang Zhang , Stephen Hemminger , Wei Liu , Dexuan Cui , Lorenzo Pieralisi , Rob Herring , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Bjorn Helgaas , Michael Kelley , Dan Carpenter Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus Message-ID: <20210824122504.GA3452187@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1629789620-11049-1-git-send-email-longli@linuxonhyperv.com> Precedence: bulk List-ID: X-Mailing-List: linux-hyperv@vger.kernel.org "Fix a bug ..." is not a very useful subject line. It doesn't say anything about what the patch *does*. It doesn't hint at a locking change. On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote: > From: Long Li > > In hv_pci_bus_exit, the code is holding a spinlock while calling > pci_destroy_slot(), which takes a mutex. It's unfortunate that slots are not better integrated into the PCI core. I'm sorry your driver even has to worry about this. > > This is not safe for spinlock. Fix this by moving the children to be > deleted to a list on the stack, and removing them after spinlock is > released. > > Fixes: 94d22763207a ("PCI: hv: Fix a race condition when removing the device") > > Cc: "K. Y. Srinivasan" > Cc: Haiyang Zhang > Cc: Stephen Hemminger > Cc: Wei Liu > Cc: Dexuan Cui > Cc: Lorenzo Pieralisi > Cc: Rob Herring > Cc: "Krzysztof Wilczyński" > Cc: Bjorn Helgaas > Cc: Michael Kelley > Cc: Dan Carpenter > Reported-by: Dan Carpenter A lore link to Dan's report would be useful here. > Signed-off-by: Long Li > --- > drivers/pci/controller/pci-hyperv.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index a53bd8728d0d..d4f3cce18957 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -3220,6 +3220,7 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) > struct hv_pci_dev *hpdev, *tmp; > unsigned long flags; > int ret; > + struct list_head removed; > > /* > * After the host sends the RESCIND_CHANNEL message, it doesn't > @@ -3229,9 +3230,18 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) > return 0; > > if (!keep_devs) { > - /* Delete any children which might still exist. */ > + INIT_LIST_HEAD(&removed); > + > + /* Move all present children to the list on stack */ > spin_lock_irqsave(&hbus->device_list_lock, flags); > - list_for_each_entry_safe(hpdev, tmp, &hbus->children, list_entry) { > + list_for_each_entry_safe(hpdev, tmp, &hbus->children, list_entry) > + list_move_tail(&hpdev->list_entry, &removed); > + spin_unlock_irqrestore(&hbus->device_list_lock, flags); > + > + /* Remove all children in the list */ > + while (!list_empty(&removed)) { > + hpdev = list_first_entry(&removed, struct hv_pci_dev, > + list_entry); > list_del(&hpdev->list_entry); > if (hpdev->pci_slot) > pci_destroy_slot(hpdev->pci_slot); > @@ -3239,7 +3249,6 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) > put_pcichild(hpdev); > put_pcichild(hpdev); > } > - spin_unlock_irqrestore(&hbus->device_list_lock, flags); > } > > ret = hv_send_resources_released(hdev); > -- > 2.25.1 >