[PATCH] ACPI: only free map once in osl.c

[PATCH] ACPI: only free map once in osl.c

[Francesco Ruggeri]

acpi_os_map_cleanup checks map->refcount outside of acpi_ioremap_lock
before freeing the map. This creates a race condition the can result
in the map being freed more than once.
A panic can be caused by running

for ((i=0; i<10; i++))
do
        for ((j=0; j<100000; j++))
        do
                cat /sys/firmware/acpi/tables/data/BERT >/dev/null
        done &
done

This patch makes sure that only the process that drops the reference
to 0 does the freeing.

Fixes: b7c1fadd6c2e ("ACPI: Do not use krefs under a mutex in osl.c")
Signed-off-by: Francesco Ruggeri <fruggeri@arista.com>
---
 drivers/acpi/osl.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index a2e844a8e9ed..41168c027a5a 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -374,19 +374,21 @@ void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 }
 EXPORT_SYMBOL_GPL(acpi_os_map_memory);
 
-static void acpi_os_drop_map_ref(struct acpi_ioremap *map)
+/* Must be called with mutex_lock(&acpi_ioremap_lock) */
+static unsigned long acpi_os_drop_map_ref(struct acpi_ioremap *map)
 {
-	if (!--map->refcount)
+	unsigned long refcount = --map->refcount;
+
+	if (!refcount)
 		list_del_rcu(&map->list);
+	return refcount;
 }
 
 static void acpi_os_map_cleanup(struct acpi_ioremap *map)
 {
-	if (!map->refcount) {
-		synchronize_rcu_expedited();
-		acpi_unmap(map->phys, map->virt);
-		kfree(map);
-	}
+	synchronize_rcu_expedited();
+	acpi_unmap(map->phys, map->virt);
+	kfree(map);
 }
 
 /**
@@ -406,6 +408,7 @@ static void acpi_os_map_cleanup(struct acpi_ioremap *map)
 void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
+	unsigned long refcount;
 
 	if (!acpi_permanent_mmap) {
 		__acpi_unmap_table(virt, size);
@@ -419,10 +422,11 @@ void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
 		WARN(true, PREFIX "%s: bad address %p\n", __func__, virt);
 		return;
 	}
-	acpi_os_drop_map_ref(map);
+	refcount = acpi_os_drop_map_ref(map);
 	mutex_unlock(&acpi_ioremap_lock);
 
-	acpi_os_map_cleanup(map);
+	if (!refcount)
+		acpi_os_map_cleanup(map);
 }
 EXPORT_SYMBOL_GPL(acpi_os_unmap_iomem);
 
@@ -457,6 +461,7 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
 {
 	u64 addr;
 	struct acpi_ioremap *map;
+	unsigned long refcount;
 
 	if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
 		return;
@@ -472,10 +477,11 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
 		mutex_unlock(&acpi_ioremap_lock);
 		return;
 	}
-	acpi_os_drop_map_ref(map);
+	refcount = acpi_os_drop_map_ref(map);
 	mutex_unlock(&acpi_ioremap_lock);
 
-	acpi_os_map_cleanup(map);
+	if (!refcount)
+		acpi_os_map_cleanup(map);
 }
 EXPORT_SYMBOL(acpi_os_unmap_generic_address);
 
-- 
2.19.1

Re: [PATCH] ACPI: only free map once in osl.c

[Dmitry Safonov]

Hi Francesco,

I believe, there's still an issue with your patch.

On Wed, 20 Nov 2019 at 05:50, Francesco Ruggeri <fruggeri@arista.com> wrote:
> @@ -472,10 +477,11 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
>                 mutex_unlock(&acpi_ioremap_lock);
>                 return;
>         }
> -       acpi_os_drop_map_ref(map);
> +       refcount = acpi_os_drop_map_ref(map);
>         mutex_unlock(&acpi_ioremap_lock);

Here comes acpi_os_get_iomem() increasing the refcount again.

>
> -       acpi_os_map_cleanup(map);
> +       if (!refcount)
> +               acpi_os_map_cleanup(map);
>  }
>  EXPORT_SYMBOL(acpi_os_unmap_generic_address);

And you free the acpi_ioremap that's being used:

>  static void acpi_os_map_cleanup(struct acpi_ioremap *map)
>  {
> -       if (!map->refcount) {
> -               synchronize_rcu_expedited();
> -               acpi_unmap(map->phys, map->virt);
> -               kfree(map);
> -       }
> +       synchronize_rcu_expedited();
> +       acpi_unmap(map->phys, map->virt);
> +       kfree(map);
>  }

Thanks,
             Dmitry

Re: [PATCH] ACPI: only free map once in osl.c

[Francesco Ruggeri]

On Thu, Nov 21, 2019 at 1:19 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
> Hi Francesco,
>
> I believe, there's still an issue with your patch.
>
> On Wed, 20 Nov 2019 at 05:50, Francesco Ruggeri <fruggeri@arista.com> wrote:
> > @@ -472,10 +477,11 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
> >                 mutex_unlock(&acpi_ioremap_lock);
> >                 return;
> >         }
> > -       acpi_os_drop_map_ref(map);
> > +       refcount = acpi_os_drop_map_ref(map);
> >         mutex_unlock(&acpi_ioremap_lock);
>
> Here comes acpi_os_get_iomem() increasing the refcount again.

Thanks Dmitry.
I think that any code that increments the refcount does so after
looking for map in acpi_ioremap under acpi_ioremap_lock,
and the process that drops the last reference removes map
from the list, also under acpi_ioremap_lock, so I am not sure
this could happen.
The synchronize_rcu_expedited in acpi_os_map_cleanup should
then take care of any other references to map (which it is my
understanding require acpi_ioremap_lock or rcu read lock).

Thanks,
Francesco

Re: [PATCH] ACPI: only free map once in osl.c

[Dmitry Safonov]

On 11/21/19 10:49 PM, Francesco Ruggeri wrote:
> On Thu, Nov 21, 2019 at 1:19 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>>
>> Hi Francesco,
>>
>> I believe, there's still an issue with your patch.
>>
>> On Wed, 20 Nov 2019 at 05:50, Francesco Ruggeri <fruggeri@arista.com> wrote:
>>> @@ -472,10 +477,11 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
>>>                 mutex_unlock(&acpi_ioremap_lock);
>>>                 return;
>>>         }
>>> -       acpi_os_drop_map_ref(map);
>>> +       refcount = acpi_os_drop_map_ref(map);
>>>         mutex_unlock(&acpi_ioremap_lock);
>>
>> Here comes acpi_os_get_iomem() increasing the refcount again.
> 
> Thanks Dmitry.
> I think that any code that increments the refcount does so after
> looking for map in acpi_ioremap under acpi_ioremap_lock,
> and the process that drops the last reference removes map
> from the list, also under acpi_ioremap_lock, so I am not sure
> this could happen.
> The synchronize_rcu_expedited in acpi_os_map_cleanup should
> then take care of any other references to map (which it is my
> understanding require acpi_ioremap_lock or rcu read lock).

Ah, right you are!
Sorry for a false alarm.

Thanks,
          Dmitry

Re: [PATCH] ACPI: only free map once in osl.c

[Dmitry Safonov]

On Wed, 20 Nov 2019 at 05:50, Francesco Ruggeri <fruggeri@arista.com> wrote:
>
> acpi_os_map_cleanup checks map->refcount outside of acpi_ioremap_lock
> before freeing the map. This creates a race condition the can result
> in the map being freed more than once.
> A panic can be caused by running
>
> for ((i=0; i<10; i++))
> do
>         for ((j=0; j<100000; j++))
>         do
>                 cat /sys/firmware/acpi/tables/data/BERT >/dev/null
>         done &
> done
>
> This patch makes sure that only the process that drops the reference
> to 0 does the freeing.
>
> Fixes: b7c1fadd6c2e ("ACPI: Do not use krefs under a mutex in osl.c")
> Signed-off-by: Francesco Ruggeri <fruggeri@arista.com>

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>

Thanks,
             Dmitry

Re: [PATCH] ACPI: only free map once in osl.c

[Francesco Ruggeri]

On Thu, Nov 21, 2019 at 2:58 PM Dmitry Safonov <0x7f454c46@gmail.com> wrote:
>
> On Wed, 20 Nov 2019 at 05:50, Francesco Ruggeri <fruggeri@arista.com> wrote:
> >
> > acpi_os_map_cleanup checks map->refcount outside of acpi_ioremap_lock
> > before freeing the map. This creates a race condition the can result
> > in the map being freed more than once.
> > A panic can be caused by running
> >
> > for ((i=0; i<10; i++))
> > do
> >         for ((j=0; j<100000; j++))
> >         do
> >                 cat /sys/firmware/acpi/tables/data/BERT >/dev/null
> >         done &
> > done
> >
> > This patch makes sure that only the process that drops the reference
> > to 0 does the freeing.
> >
> > Fixes: b7c1fadd6c2e ("ACPI: Do not use krefs under a mutex in osl.c")
> > Signed-off-by: Francesco Ruggeri <fruggeri@arista.com>
>
> Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
>
> Thanks,
>              Dmitry

Any more comments on this?
Can this be applied or is more work needed?

Thanks,
Francesco Ruggeri

Re: [PATCH] ACPI: only free map once in osl.c

[Rafael J. Wysocki]

On Wednesday, November 20, 2019 6:47:27 AM CET Francesco Ruggeri wrote:
> acpi_os_map_cleanup checks map->refcount outside of acpi_ioremap_lock
> before freeing the map. This creates a race condition the can result
> in the map being freed more than once.
> A panic can be caused by running
> 
> for ((i=0; i<10; i++))
> do
>         for ((j=0; j<100000; j++))
>         do
>                 cat /sys/firmware/acpi/tables/data/BERT >/dev/null
>         done &
> done
> 
> This patch makes sure that only the process that drops the reference
> to 0 does the freeing.
> 
> Fixes: b7c1fadd6c2e ("ACPI: Do not use krefs under a mutex in osl.c")
> Signed-off-by: Francesco Ruggeri <fruggeri@arista.com>
> ---
>  drivers/acpi/osl.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index a2e844a8e9ed..41168c027a5a 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -374,19 +374,21 @@ void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  }
>  EXPORT_SYMBOL_GPL(acpi_os_map_memory);
>  
> -static void acpi_os_drop_map_ref(struct acpi_ioremap *map)
> +/* Must be called with mutex_lock(&acpi_ioremap_lock) */
> +static unsigned long acpi_os_drop_map_ref(struct acpi_ioremap *map)
>  {
> -	if (!--map->refcount)
> +	unsigned long refcount = --map->refcount;
> +
> +	if (!refcount)
>  		list_del_rcu(&map->list);
> +	return refcount;
>  }
>  
>  static void acpi_os_map_cleanup(struct acpi_ioremap *map)
>  {
> -	if (!map->refcount) {
> -		synchronize_rcu_expedited();
> -		acpi_unmap(map->phys, map->virt);
> -		kfree(map);
> -	}
> +	synchronize_rcu_expedited();
> +	acpi_unmap(map->phys, map->virt);
> +	kfree(map);
>  }
>  
>  /**
> @@ -406,6 +408,7 @@ static void acpi_os_map_cleanup(struct acpi_ioremap *map)
>  void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
> +	unsigned long refcount;
>  
>  	if (!acpi_permanent_mmap) {
>  		__acpi_unmap_table(virt, size);
> @@ -419,10 +422,11 @@ void __ref acpi_os_unmap_iomem(void __iomem *virt, acpi_size size)
>  		WARN(true, PREFIX "%s: bad address %p\n", __func__, virt);
>  		return;
>  	}
> -	acpi_os_drop_map_ref(map);
> +	refcount = acpi_os_drop_map_ref(map);
>  	mutex_unlock(&acpi_ioremap_lock);
>  
> -	acpi_os_map_cleanup(map);
> +	if (!refcount)
> +		acpi_os_map_cleanup(map);
>  }
>  EXPORT_SYMBOL_GPL(acpi_os_unmap_iomem);
>  
> @@ -457,6 +461,7 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
>  {
>  	u64 addr;
>  	struct acpi_ioremap *map;
> +	unsigned long refcount;
>  
>  	if (gas->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY)
>  		return;
> @@ -472,10 +477,11 @@ void acpi_os_unmap_generic_address(struct acpi_generic_address *gas)
>  		mutex_unlock(&acpi_ioremap_lock);
>  		return;
>  	}
> -	acpi_os_drop_map_ref(map);
> +	refcount = acpi_os_drop_map_ref(map);
>  	mutex_unlock(&acpi_ioremap_lock);
>  
> -	acpi_os_map_cleanup(map);
> +	if (!refcount)
> +		acpi_os_map_cleanup(map);
>  }
>  EXPORT_SYMBOL(acpi_os_unmap_generic_address);
>  
> 

Applying as a stable-candidate fix for 5.5, thanks!