linux-hyperv.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
@ 2021-08-24  7:20 longli
  2021-08-24 11:02 ` Wei Liu
  2021-08-24 12:25 ` Bjorn Helgaas
  0 siblings, 2 replies; 13+ messages in thread
From: longli @ 2021-08-24  7:20 UTC (permalink / raw)
  To: linux-pci, linux-kernel, linux-hyperv
  Cc: Long Li, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Wei Liu, Dexuan Cui, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Michael Kelley,
	Dan Carpenter

From: Long Li <longli@microsoft.com>

In hv_pci_bus_exit, the code is holding a spinlock while calling
pci_destroy_slot(), which takes a mutex.

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" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Rob Herring <robh@kernel.org>
Cc: "Krzysztof Wilczyński" <kw@linux.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Michael Kelley <mikelley@microsoft.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Long Li <longli@microsoft.com>
---
 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-24  7:20 [PATCH] PCI: hv: Fix a bug on removing child devices on the bus longli
@ 2021-08-24 11:02 ` Wei Liu
  2021-08-24 17:28   ` Long Li
  2021-08-24 12:25 ` Bjorn Helgaas
  1 sibling, 1 reply; 13+ messages in thread
From: Wei Liu @ 2021-08-24 11:02 UTC (permalink / raw)
  To: longli
  Cc: linux-pci, linux-kernel, linux-hyperv, Long Li, K. Y. Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Kelley, Dan Carpenter

On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
> 
> In hv_pci_bus_exit, the code is holding a spinlock while calling
> pci_destroy_slot(), which takes a mutex.
> 
> 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" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Cc: Wei Liu <wei.liu@kernel.org>
> Cc: Dexuan Cui <decui@microsoft.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: "Krzysztof Wilczyński" <kw@linux.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Michael Kelley <mikelley@microsoft.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  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;

This can be moved to where it is needed -- the if(!keep_dev) branch --
to limit its scope.

>  
>  	/*
>  	 * 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_for_each_entry_safe can also be used here, right?

Wei.

>  			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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-24  7:20 [PATCH] PCI: hv: Fix a bug on removing child devices on the bus longli
  2021-08-24 11:02 ` Wei Liu
@ 2021-08-24 12:25 ` Bjorn Helgaas
  2021-08-24 17:30   ` Long Li
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2021-08-24 12:25 UTC (permalink / raw)
  To: longli
  Cc: linux-pci, linux-kernel, linux-hyperv, Long Li, K. Y. Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Kelley, Dan Carpenter

"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 <longli@microsoft.com>
> 
> 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" <kys@microsoft.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Cc: Wei Liu <wei.liu@kernel.org>
> Cc: Dexuan Cui <decui@microsoft.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: "Krzysztof Wilczyński" <kw@linux.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Michael Kelley <mikelley@microsoft.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

A lore link to Dan's report would be useful here.

> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-24 11:02 ` Wei Liu
@ 2021-08-24 17:28   ` Long Li
  2021-08-25 19:11     ` Michael Kelley
  0 siblings, 1 reply; 13+ messages in thread
From: Long Li @ 2021-08-24 17:28 UTC (permalink / raw)
  To: Wei Liu, longli
  Cc: linux-pci, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Michael Kelley, Dan Carpenter

> Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> 
> On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > In hv_pci_bus_exit, the code is holding a spinlock while calling
> > pci_destroy_slot(), which takes a mutex.
> >
> > 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" <kys@microsoft.com>
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Cc: Wei Liu <wei.liu@kernel.org>
> > Cc: Dexuan Cui <decui@microsoft.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: "Krzysztof Wilczyński" <kw@linux.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Michael Kelley <mikelley@microsoft.com>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  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;
> 
> This can be moved to where it is needed -- the if(!keep_dev) branch -- to limit its
> scope.
> 
> >
> >  	/*
> >  	 * 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_for_each_entry_safe can also be used here, right?
> 
> Wei.

I will address your comments.

Long

> 
> >  			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
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-24 12:25 ` Bjorn Helgaas
@ 2021-08-24 17:30   ` Long Li
  0 siblings, 0 replies; 13+ messages in thread
From: Long Li @ 2021-08-24 17:30 UTC (permalink / raw)
  To: Bjorn Helgaas, longli
  Cc: linux-pci, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Michael Kelley, Dan Carpenter

> Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> 
> "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 <longli@microsoft.com>
> >
> > 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" <kys@microsoft.com>
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>
> > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > Cc: Wei Liu <wei.liu@kernel.org>
> > Cc: Dexuan Cui <decui@microsoft.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: "Krzysztof Wilczyński" <kw@linux.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Michael Kelley <mikelley@microsoft.com>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> A lore link to Dan's report would be useful here.

I will address your comments and send v2.

Long

> 
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  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
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-24 17:28   ` Long Li
@ 2021-08-25 19:11     ` Michael Kelley
  2021-08-25 20:25       ` Long Li
  2021-08-25 20:32       ` Rob Herring
  0 siblings, 2 replies; 13+ messages in thread
From: Michael Kelley @ 2021-08-25 19:11 UTC (permalink / raw)
  To: Long Li, Wei Liu, longli
  Cc: linux-pci, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Dan Carpenter

From: Long Li <longli@microsoft.com> Sent: Tuesday, August 24, 2021 10:28 AM
> 
> > Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> >
> > On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
> > > From: Long Li <longli@microsoft.com>
> > >
> > > In hv_pci_bus_exit, the code is holding a spinlock while calling
> > > pci_destroy_slot(), which takes a mutex.
> > >
> > > 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" <kys@microsoft.com>
> > > Cc: Haiyang Zhang <haiyangz@microsoft.com>
> > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > Cc: Wei Liu <wei.liu@kernel.org>
> > > Cc: Dexuan Cui <decui@microsoft.com>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: "Krzysztof Wilczyński" <kw@linux.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Michael Kelley <mikelley@microsoft.com>
> > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Long Li <longli@microsoft.com>
> > > ---
> > >  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;
> >
> > This can be moved to where it is needed -- the if(!keep_dev) branch -- to limit its
> > scope.
> >
> > >
> > >  	/*
> > >  	 * 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_for_each_entry_safe can also be used here, right?
> >
> > Wei.
> 
> I will address your comments.
> 
> Long

I thought list_for_each_entry_safe() is for use when list manipulation
is *not* protected by a lock and you want to safely walk the list
even if an entry gets removed.  If the list is protected by a lock or
not subject to contention (as is the case here), then
list_for_each_entry() is the simpler implementation.  The original
implementation didn't need to use the _safe version because of
the spin lock.

Or do I have it backwards?  

Michael

> 
> >
> > >  			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
> > >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-25 19:11     ` Michael Kelley
@ 2021-08-25 20:25       ` Long Li
  2021-08-26 16:50         ` Michael Kelley
  2021-08-25 20:32       ` Rob Herring
  1 sibling, 1 reply; 13+ messages in thread
From: Long Li @ 2021-08-25 20:25 UTC (permalink / raw)
  To: Michael Kelley, Wei Liu, longli
  Cc: linux-pci, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Dan Carpenter

> Subject: RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> 
> From: Long Li <longli@microsoft.com> Sent: Tuesday, August 24, 2021 10:28
> AM
> >
> > > Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on
> > > the bus
> > >
> > > On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com
> wrote:
> > > > From: Long Li <longli@microsoft.com>
> > > >
> > > > In hv_pci_bus_exit, the code is holding a spinlock while calling
> > > > pci_destroy_slot(), which takes a mutex.
> > > >
> > > > 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" <kys@microsoft.com>
> > > > Cc: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > > Cc: Wei Liu <wei.liu@kernel.org>
> > > > Cc: Dexuan Cui <decui@microsoft.com>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: "Krzysztof Wilczyński" <kw@linux.com>
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Michael Kelley <mikelley@microsoft.com>
> > > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Long Li <longli@microsoft.com>
> > > > ---
> > > >  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;
> > >
> > > This can be moved to where it is needed -- the if(!keep_dev) branch
> > > -- to limit its scope.
> > >
> > > >
> > > >  	/*
> > > >  	 * 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_for_each_entry_safe can also be used here, right?
> > >
> > > Wei.
> >
> > I will address your comments.
> >
> > Long
> 
> I thought list_for_each_entry_safe() is for use when list manipulation is *not*
> protected by a lock and you want to safely walk the list even if an entry gets
> removed.  If the list is protected by a lock or not subject to contention (as is the
> case here), then
> list_for_each_entry() is the simpler implementation.  The original
> implementation didn't need to use the _safe version because of the spin lock.
> 
> Or do I have it backwards?
> 
> Michael

I think we need list_for_each_entry_safe() because we delete the list elements while going through them:

Here is the comment on list_for_each_entry_safe():
/**
 * Loop through the list, keeping a backup pointer to the element. This
 * macro allows for the deletion of a list element while looping through the
 * list.
 *
 * See list_for_each_entry for more details.
 */

> 
> >
> > >
> > > >  			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
> > > >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-25 19:11     ` Michael Kelley
  2021-08-25 20:25       ` Long Li
@ 2021-08-25 20:32       ` Rob Herring
  1 sibling, 0 replies; 13+ messages in thread
From: Rob Herring @ 2021-08-25 20:32 UTC (permalink / raw)
  To: Michael Kelley
  Cc: Long Li, Wei Liu, longli, linux-pci, linux-kernel, linux-hyperv,
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
	Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas,
	Dan Carpenter

On Wed, Aug 25, 2021 at 2:11 PM Michael Kelley <mikelley@microsoft.com> wrote:
>
> From: Long Li <longli@microsoft.com> Sent: Tuesday, August 24, 2021 10:28 AM
> >
> > > Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> > >
> > > On Tue, Aug 24, 2021 at 12:20:20AM -0700, longli@linuxonhyperv.com wrote:
> > > > From: Long Li <longli@microsoft.com>
> > > >
> > > > In hv_pci_bus_exit, the code is holding a spinlock while calling
> > > > pci_destroy_slot(), which takes a mutex.
> > > >
> > > > 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" <kys@microsoft.com>
> > > > Cc: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > > > Cc: Wei Liu <wei.liu@kernel.org>
> > > > Cc: Dexuan Cui <decui@microsoft.com>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: "Krzysztof Wilczyński" <kw@linux.com>
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Michael Kelley <mikelley@microsoft.com>
> > > > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Long Li <longli@microsoft.com>
> > > > ---
> > > >  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;
> > >
> > > This can be moved to where it is needed -- the if(!keep_dev) branch -- to limit its
> > > scope.
> > >
> > > >
> > > >   /*
> > > >    * 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_for_each_entry_safe can also be used here, right?
> > >
> > > Wei.
> >
> > I will address your comments.
> >
> > Long
>
> I thought list_for_each_entry_safe() is for use when list manipulation
> is *not* protected by a lock and you want to safely walk the list
> even if an entry gets removed.  If the list is protected by a lock or
> not subject to contention (as is the case here), then
> list_for_each_entry() is the simpler implementation.  The original
> implementation didn't need to use the _safe version because of
> the spin lock.
>
> Or do I have it backwards?

"_safe" only means "safe against removal of list entry" as the
kerneldoc says. But that means removal within the loop iteration, not
any writer. A lock is needed in either case if there's another writer.

Don't ask me about the RCU variant though...

Rob

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-25 20:25       ` Long Li
@ 2021-08-26 16:50         ` Michael Kelley
  2021-08-26 19:41           ` Wei Liu
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Kelley @ 2021-08-26 16:50 UTC (permalink / raw)
  To: Long Li, Wei Liu, longli
  Cc: linux-pci, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Dan Carpenter

From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021 1:25 PM

> >
> > I thought list_for_each_entry_safe() is for use when list manipulation is *not*
> > protected by a lock and you want to safely walk the list even if an entry gets
> > removed.  If the list is protected by a lock or not subject to contention (as is the
> > case here), then
> > list_for_each_entry() is the simpler implementation.  The original
> > implementation didn't need to use the _safe version because of the spin lock.
> >
> > Or do I have it backwards?
> >
> > Michael
> 
> I think we need list_for_each_entry_safe() because we delete the list elements while going through them:
> 
> Here is the comment on list_for_each_entry_safe():
> /**
>  * Loop through the list, keeping a backup pointer to the element. This
>  * macro allows for the deletion of a list element while looping through the
>  * list.
>  *
>  * See list_for_each_entry for more details.
>  */
> 

Got it.  Thanks (and to Rob Herring).   I read that comment but
with the wrong assumptions and didn't understand it correctly.

Interestingly, pci-hyperv.c has another case of looping through
this list and removing items where the _safe version is not used.
See pci_devices_present_work() where the missing children are
moved to a list on the stack.

Michael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-26 16:50         ` Michael Kelley
@ 2021-08-26 19:41           ` Wei Liu
  2021-08-26 20:09             ` Long Li
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Liu @ 2021-08-26 19:41 UTC (permalink / raw)
  To: Michael Kelley
  Cc: Long Li, Wei Liu, longli, linux-pci, linux-kernel, linux-hyperv,
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Dan Carpenter

On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
> From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021 1:25 PM
> 
> > >
> > > I thought list_for_each_entry_safe() is for use when list manipulation is *not*
> > > protected by a lock and you want to safely walk the list even if an entry gets
> > > removed.  If the list is protected by a lock or not subject to contention (as is the
> > > case here), then
> > > list_for_each_entry() is the simpler implementation.  The original
> > > implementation didn't need to use the _safe version because of the spin lock.
> > >
> > > Or do I have it backwards?
> > >
> > > Michael
> > 
> > I think we need list_for_each_entry_safe() because we delete the list elements while going through them:
> > 
> > Here is the comment on list_for_each_entry_safe():
> > /**
> >  * Loop through the list, keeping a backup pointer to the element. This
> >  * macro allows for the deletion of a list element while looping through the
> >  * list.
> >  *
> >  * See list_for_each_entry for more details.
> >  */
> > 
> 
> Got it.  Thanks (and to Rob Herring).   I read that comment but
> with the wrong assumptions and didn't understand it correctly.
> 
> Interestingly, pci-hyperv.c has another case of looping through
> this list and removing items where the _safe version is not used.
> See pci_devices_present_work() where the missing children are
> moved to a list on the stack.

That can be converted too, I think.

The original code is not wrong per-se. It is just not as concise as
using list_for_each_entry_safe.

Wei.

> 
> Michael

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-26 19:41           ` Wei Liu
@ 2021-08-26 20:09             ` Long Li
  2021-08-26 20:15               ` Wei Liu
  0 siblings, 1 reply; 13+ messages in thread
From: Long Li @ 2021-08-26 20:09 UTC (permalink / raw)
  To: Wei Liu, Michael Kelley
  Cc: longli, linux-pci, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Dexuan Cui, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Dan Carpenter

> Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> 
> On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
> > From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021
> > 1:25 PM
> >
> > > >
> > > > I thought list_for_each_entry_safe() is for use when list
> > > > manipulation is *not* protected by a lock and you want to safely
> > > > walk the list even if an entry gets removed.  If the list is
> > > > protected by a lock or not subject to contention (as is the case
> > > > here), then
> > > > list_for_each_entry() is the simpler implementation.  The original
> > > > implementation didn't need to use the _safe version because of the spin
> lock.
> > > >
> > > > Or do I have it backwards?
> > > >
> > > > Michael
> > >
> > > I think we need list_for_each_entry_safe() because we delete the list
> elements while going through them:
> > >
> > > Here is the comment on list_for_each_entry_safe():
> > > /**
> > >  * Loop through the list, keeping a backup pointer to the element.
> > > This
> > >  * macro allows for the deletion of a list element while looping
> > > through the
> > >  * list.
> > >  *
> > >  * See list_for_each_entry for more details.
> > >  */
> > >
> >
> > Got it.  Thanks (and to Rob Herring).   I read that comment but
> > with the wrong assumptions and didn't understand it correctly.
> >
> > Interestingly, pci-hyperv.c has another case of looping through this
> > list and removing items where the _safe version is not used.
> > See pci_devices_present_work() where the missing children are moved to
> > a list on the stack.
> 
> That can be converted too, I think.
> 
> The original code is not wrong per-se. It is just not as concise as using
> list_for_each_entry_safe.
> 
> Wei.

I assume we are talking about the following code in pci_devices_present_work():

                list_for_each_entry(hpdev, &hbus->children, list_entry) {
                        if (hpdev->reported_missing) {
                                found = true;
                                put_pcichild(hpdev);
                                list_move_tail(&hpdev->list_entry, &removed);
                                break;
                        }
                }

This code is correct as there is a "break" after a list entry is removed from the list. So there is no need to use the _safe version. This code can be converted to use the _safe version.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-26 20:09             ` Long Li
@ 2021-08-26 20:15               ` Wei Liu
  2021-08-26 20:20                 ` Long Li
  0 siblings, 1 reply; 13+ messages in thread
From: Wei Liu @ 2021-08-26 20:15 UTC (permalink / raw)
  To: Long Li
  Cc: Wei Liu, Michael Kelley, longli, linux-pci, linux-kernel,
	linux-hyperv, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Dexuan Cui, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Dan Carpenter

On Thu, Aug 26, 2021 at 08:09:19PM +0000, Long Li wrote:
> > Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> > 
> > On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
> > > From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25, 2021
> > > 1:25 PM
> > >
> > > > >
> > > > > I thought list_for_each_entry_safe() is for use when list
> > > > > manipulation is *not* protected by a lock and you want to safely
> > > > > walk the list even if an entry gets removed.  If the list is
> > > > > protected by a lock or not subject to contention (as is the case
> > > > > here), then
> > > > > list_for_each_entry() is the simpler implementation.  The original
> > > > > implementation didn't need to use the _safe version because of the spin
> > lock.
> > > > >
> > > > > Or do I have it backwards?
> > > > >
> > > > > Michael
> > > >
> > > > I think we need list_for_each_entry_safe() because we delete the list
> > elements while going through them:
> > > >
> > > > Here is the comment on list_for_each_entry_safe():
> > > > /**
> > > >  * Loop through the list, keeping a backup pointer to the element.
> > > > This
> > > >  * macro allows for the deletion of a list element while looping
> > > > through the
> > > >  * list.
> > > >  *
> > > >  * See list_for_each_entry for more details.
> > > >  */
> > > >
> > >
> > > Got it.  Thanks (and to Rob Herring).   I read that comment but
> > > with the wrong assumptions and didn't understand it correctly.
> > >
> > > Interestingly, pci-hyperv.c has another case of looping through this
> > > list and removing items where the _safe version is not used.
> > > See pci_devices_present_work() where the missing children are moved to
> > > a list on the stack.
> > 
> > That can be converted too, I think.
> > 
> > The original code is not wrong per-se. It is just not as concise as using
> > list_for_each_entry_safe.
> > 
> > Wei.
> 
> I assume we are talking about the following code in pci_devices_present_work():
> 
>                 list_for_each_entry(hpdev, &hbus->children, list_entry) {
>                         if (hpdev->reported_missing) {
>                                 found = true;
>                                 put_pcichild(hpdev);
>                                 list_move_tail(&hpdev->list_entry, &removed);
>                                 break;
>                         }
>                 }
> 
> This code is correct as there is a "break" after a list entry is
> removed from the list. So there is no need to use the _safe version.
> This code can be converted to use the _safe version.

After this block there is another block like

  while (!list_empty(removed)) {
	...
  	list_del(...)

  }

I assumed Michael was referring to that block. :-)

Wei.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
  2021-08-26 20:15               ` Wei Liu
@ 2021-08-26 20:20                 ` Long Li
  0 siblings, 0 replies; 13+ messages in thread
From: Long Li @ 2021-08-26 20:20 UTC (permalink / raw)
  To: Wei Liu
  Cc: Michael Kelley, longli, linux-pci, linux-kernel, linux-hyperv,
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Dexuan Cui,
	Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Dan Carpenter

> Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on the bus
> 
> On Thu, Aug 26, 2021 at 08:09:19PM +0000, Long Li wrote:
> > > Subject: Re: [PATCH] PCI: hv: Fix a bug on removing child devices on
> > > the bus
> > >
> > > On Thu, Aug 26, 2021 at 04:50:28PM +0000, Michael Kelley wrote:
> > > > From: Long Li <longli@microsoft.com> Sent: Wednesday, August 25,
> > > > 2021
> > > > 1:25 PM
> > > >
> > > > > >
> > > > > > I thought list_for_each_entry_safe() is for use when list
> > > > > > manipulation is *not* protected by a lock and you want to
> > > > > > safely walk the list even if an entry gets removed.  If the
> > > > > > list is protected by a lock or not subject to contention (as
> > > > > > is the case here), then
> > > > > > list_for_each_entry() is the simpler implementation.  The
> > > > > > original implementation didn't need to use the _safe version
> > > > > > because of the spin
> > > lock.
> > > > > >
> > > > > > Or do I have it backwards?
> > > > > >
> > > > > > Michael
> > > > >
> > > > > I think we need list_for_each_entry_safe() because we delete the
> > > > > list
> > > elements while going through them:
> > > > >
> > > > > Here is the comment on list_for_each_entry_safe():
> > > > > /**
> > > > >  * Loop through the list, keeping a backup pointer to the element.
> > > > > This
> > > > >  * macro allows for the deletion of a list element while looping
> > > > > through the
> > > > >  * list.
> > > > >  *
> > > > >  * See list_for_each_entry for more details.
> > > > >  */
> > > > >
> > > >
> > > > Got it.  Thanks (and to Rob Herring).   I read that comment but
> > > > with the wrong assumptions and didn't understand it correctly.
> > > >
> > > > Interestingly, pci-hyperv.c has another case of looping through
> > > > this list and removing items where the _safe version is not used.
> > > > See pci_devices_present_work() where the missing children are
> > > > moved to a list on the stack.
> > >
> > > That can be converted too, I think.
> > >
> > > The original code is not wrong per-se. It is just not as concise as
> > > using list_for_each_entry_safe.
> > >
> > > Wei.
> >
> > I assume we are talking about the following code in
> pci_devices_present_work():
> >
> >                 list_for_each_entry(hpdev, &hbus->children, list_entry) {
> >                         if (hpdev->reported_missing) {
> >                                 found = true;
> >                                 put_pcichild(hpdev);
> >                                 list_move_tail(&hpdev->list_entry, &removed);
> >                                 break;
> >                         }
> >                 }
> >
> > This code is correct as there is a "break" after a list entry is
> > removed from the list. So there is no need to use the _safe version.
> > This code can be converted to use the _safe version.
> 
> After this block there is another block like
> 
>   while (!list_empty(removed)) {
> 	...
>   	list_del(...)
> 
>   }
> 
> I assumed Michael was referring to that block. :-)
> 
> Wei.

This block is also correct. We don't have a bug here but there is a better way to code it.

Long

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-08-26 20:21 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24  7:20 [PATCH] PCI: hv: Fix a bug on removing child devices on the bus longli
2021-08-24 11:02 ` Wei Liu
2021-08-24 17:28   ` Long Li
2021-08-25 19:11     ` Michael Kelley
2021-08-25 20:25       ` Long Li
2021-08-26 16:50         ` Michael Kelley
2021-08-26 19:41           ` Wei Liu
2021-08-26 20:09             ` Long Li
2021-08-26 20:15               ` Wei Liu
2021-08-26 20:20                 ` Long Li
2021-08-25 20:32       ` Rob Herring
2021-08-24 12:25 ` Bjorn Helgaas
2021-08-24 17:30   ` Long Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).