Patchwork [v2,4/4] PCI/AER: fix a small race condition window when rmmod aer_inject

login
register
mail settings
Submitter Yijing Wang
Date Sept. 12, 2012, 12:33 p.m.
Message ID <1347453225-7704-4-git-send-email-wangyijing@huawei.com>
Download mbox | patch
Permalink /patch/183357/
State Changes Requested
Headers show

Comments

Yijing Wang - Sept. 12, 2012, 12:33 p.m.
Fix a small race condition window between pci_bus_ops_pop() and
pci_bus_set_ops() functions.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/pcie/aer/aer_inject.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
Huang Ying - Sept. 13, 2012, 1:29 a.m.
On Wed, 2012-09-12 at 20:33 +0800, Yijing Wang wrote:
> Fix a small race condition window between pci_bus_ops_pop() and
> pci_bus_set_ops() functions.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/pci/pcie/aer/aer_inject.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
> index bd043db..69c2d77 100644
> --- a/drivers/pci/pcie/aer/aer_inject.c
> +++ b/drivers/pci/pcie/aer/aer_inject.c
> @@ -141,12 +141,11 @@ static struct pci_ops *__find_pci_bus_ops(struct pci_bus *bus)
>  	return NULL;
>  }
>  
> +/* inject_lock must be held before calling */
>  static struct pci_bus_ops *pci_bus_ops_pop(void)
>  {
> -	unsigned long flags;
>  	struct pci_bus_ops *bus_ops = NULL;
>  
> -	spin_lock_irqsave(&inject_lock, flags);
>  	if (list_empty(&pci_bus_ops_list))
>  		bus_ops = NULL;
>  	else {
> @@ -154,7 +153,6 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
>  		list_del(lh);
>  		bus_ops = list_entry(lh, struct pci_bus_ops, list);
>  	}
> -	spin_unlock_irqrestore(&inject_lock, flags);
>  	return bus_ops;
>  }
>  
> @@ -610,14 +608,12 @@ static void __exit aer_inject_exit(void)
>  	bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
>  	misc_deregister(&aer_inject_device);
>  
> +	spin_lock_irqsave(&inject_lock, flags);
>  	while ((bus_ops = pci_bus_ops_pop())) {
>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
>  		kfree(bus_ops);
>  	}
>  	
> -	clean_untracked_pci_aer_ops();
> -
> -	spin_lock_irqsave(&inject_lock, flags);
>  	list_for_each_entry_safe(err, err_next, &einjected, list) {
>  		list_del(&err->list);
>  		kfree(err);

This may trigger a AB BA dead lock.  In aer_inject_exit, the lock
sequence is:

inject_lock -> pci_lock

In pci config read/write path, the lock sequence is:

pci_lock -> inject_lock

With lockdep enabled, you may found warning on that.

Best Regards,
Huang YIng


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yijing Wang - Sept. 13, 2012, 10:59 a.m.
On 2012/9/13 9:29, Huang Ying wrote:
> On Wed, 2012-09-12 at 20:33 +0800, Yijing Wang wrote:
>> Fix a small race condition window between pci_bus_ops_pop() and
>> pci_bus_set_ops() functions.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  drivers/pci/pcie/aer/aer_inject.c |    8 ++------
>>  1 files changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
>> index bd043db..69c2d77 100644
>> --- a/drivers/pci/pcie/aer/aer_inject.c
>> +++ b/drivers/pci/pcie/aer/aer_inject.c
>> @@ -141,12 +141,11 @@ static struct pci_ops *__find_pci_bus_ops(struct pci_bus *bus)
>>  	return NULL;
>>  }
>>  
>> +/* inject_lock must be held before calling */
>>  static struct pci_bus_ops *pci_bus_ops_pop(void)
>>  {
>> -	unsigned long flags;
>>  	struct pci_bus_ops *bus_ops = NULL;
>>  
>> -	spin_lock_irqsave(&inject_lock, flags);
>>  	if (list_empty(&pci_bus_ops_list))
>>  		bus_ops = NULL;
>>  	else {
>> @@ -154,7 +153,6 @@ static struct pci_bus_ops *pci_bus_ops_pop(void)
>>  		list_del(lh);
>>  		bus_ops = list_entry(lh, struct pci_bus_ops, list);
>>  	}
>> -	spin_unlock_irqrestore(&inject_lock, flags);
>>  	return bus_ops;
>>  }
>>  
>> @@ -610,14 +608,12 @@ static void __exit aer_inject_exit(void)
>>  	bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
>>  	misc_deregister(&aer_inject_device);
>>  
>> +	spin_lock_irqsave(&inject_lock, flags);
>>  	while ((bus_ops = pci_bus_ops_pop())) {
>>  		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
>>  		kfree(bus_ops);
>>  	}
>>  	
>> -	clean_untracked_pci_aer_ops();
>> -
>> -	spin_lock_irqsave(&inject_lock, flags);
>>  	list_for_each_entry_safe(err, err_next, &einjected, list) {
>>  		list_del(&err->list);
>>  		kfree(err);
> 
> This may trigger a AB BA dead lock.  In aer_inject_exit, the lock
> sequence is:
> 
> inject_lock -> pci_lock
> 
> In pci config read/write path, the lock sequence is:
> 
> pci_lock -> inject_lock
> 
> With lockdep enabled, you may found warning on that.

OK, I will carry out a detailed test with lockdep enabled, and try to improve this patch.
Thanks for review and comments!

------
Thanks
Yijing

> 
> Best Regards,
> Huang YIng
> 
> 
> 
> .
>

Patch

diff --git a/drivers/pci/pcie/aer/aer_inject.c b/drivers/pci/pcie/aer/aer_inject.c
index bd043db..69c2d77 100644
--- a/drivers/pci/pcie/aer/aer_inject.c
+++ b/drivers/pci/pcie/aer/aer_inject.c
@@ -141,12 +141,11 @@  static struct pci_ops *__find_pci_bus_ops(struct pci_bus *bus)
 	return NULL;
 }
 
+/* inject_lock must be held before calling */
 static struct pci_bus_ops *pci_bus_ops_pop(void)
 {
-	unsigned long flags;
 	struct pci_bus_ops *bus_ops = NULL;
 
-	spin_lock_irqsave(&inject_lock, flags);
 	if (list_empty(&pci_bus_ops_list))
 		bus_ops = NULL;
 	else {
@@ -154,7 +153,6 @@  static struct pci_bus_ops *pci_bus_ops_pop(void)
 		list_del(lh);
 		bus_ops = list_entry(lh, struct pci_bus_ops, list);
 	}
-	spin_unlock_irqrestore(&inject_lock, flags);
 	return bus_ops;
 }
 
@@ -610,14 +608,12 @@  static void __exit aer_inject_exit(void)
 	bus_unregister_notifier(&pci_bus_type, &aerinj_hp_notifier);
 	misc_deregister(&aer_inject_device);
 
+	spin_lock_irqsave(&inject_lock, flags);
 	while ((bus_ops = pci_bus_ops_pop())) {
 		pci_bus_set_ops(bus_ops->bus, bus_ops->ops);
 		kfree(bus_ops);
 	}
 	
-	clean_untracked_pci_aer_ops();
-
-	spin_lock_irqsave(&inject_lock, flags);
 	list_for_each_entry_safe(err, err_next, &einjected, list) {
 		list_del(&err->list);
 		kfree(err);