[PATCH 0/3] iommu/amd/iommu_v2: Fix refcount related issues

[PATCH 0/3] iommu/amd/iommu_v2: Fix refcount related issues

[Vasant Hegde]

This patch series fixes various issues around pasid refcount handling.

Issues addressed in this patchset are as follows:

1. refcount_dec assumes that refcount will never reach 0:
   threfore: REFCOUNT_WARN("decrement hit 0; leaking memory")
   will be invoked on pasid unbind

   *  Fixed by changing refcount_dec to refcount_dec_and_test
      to explicitly handle refcount=1 (skip waiting for wait queue)

2. ppr_handler calls the wake_up API on the pasid_state wait_queue.
   However, once refcount has been decremented, unbind_pasid may have
   already freed this object - causing unallocated memory access

   *  Fixed by adding spinlock to ensure atomicity between refcount
      check and event queue handling. Moved wait_queue APIs to
      wake_up_locked / wake_event_interruptible_locked accordingly

3. If new ppr_handler arrives after get_pasid_wait once pasid unbind
   has decremented refcount to 0 - get_pasid_state can still acquire
   a pointer to pasid_state after being freed by unbind_pasid causing
   unallocated memory access

   *  Fixed by changing ref_count_inc to ref_count_inc_not_zero
      to ensure no new ppr_handle starts after pasid has been unbound
      and NULL (invalid) pasid_state is returned on zero.

Signed-off-by: Daniel Marcovitch <dmarcovitch@nvidia.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

Base commit: v6.2-rc5

Daniel Marcovitch (3):
  iommu/amd/iommu_v2: Fix pasid_state refcount dec hit 0 warning on pasid unbind
  iommu/amd/iommu_v2: Fix pasid_state->wq race
  iommu/amd/iommu_v2: Prevent scheduling new ppr notifier during unbind_pasid

 drivers/iommu/amd/iommu_v2.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/3] iommu/amd/iommu_v2: Fix pasid_state refcount dec hit 0 warning on pasid unbind

[Vasant Hegde]

From: Daniel Marcovitch <dmarcovitch@nvidia.com>

When unbinding pasid - a race condition exists vs outstanding page faults.

To prevent this, the pasid_state object contains a refcount.
    * set to 1 on pasid bind
    * incremented on each ppr notification start
    * decremented on each ppr notification done
    * decremented on pasid unbind

Since refcount_dec assumes that refcount will never reach 0:
  the current implementation causes the following to be invoked on
  pasid unbind:
        REFCOUNT_WARN("decrement hit 0; leaking memory")

Fix this issue by changing refcount_dec to refcount_dec_and_test
to explicitly handle refcount=1.

Signed-off-by: Daniel Marcovitch <dmarcovitch@nvidia.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 864e4ffb6aa9..858748baae0b 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -262,8 +262,8 @@ static void put_pasid_state(struct pasid_state *pasid_state)
 
 static void put_pasid_state_wait(struct pasid_state *pasid_state)
 {
-	refcount_dec(&pasid_state->count);
-	wait_event(pasid_state->wq, !refcount_read(&pasid_state->count));
+	if (!refcount_dec_and_test(&pasid_state->count))
+		wait_event(pasid_state->wq, !refcount_read(&pasid_state->count));
 	free_pasid_state(pasid_state);
 }
 
-- 
2.31.1

Re: [PATCH 1/3] iommu/amd/iommu_v2: Fix pasid_state refcount dec hit 0 warning on pasid unbind

[Jason Gunthorpe]

On Tue, Jan 24, 2023 at 10:43:53AM +0000, Vasant Hegde wrote:
> From: Daniel Marcovitch <dmarcovitch@nvidia.com>
> 
> When unbinding pasid - a race condition exists vs outstanding page faults.
> 
> To prevent this, the pasid_state object contains a refcount.
>     * set to 1 on pasid bind
>     * incremented on each ppr notification start
>     * decremented on each ppr notification done
>     * decremented on pasid unbind

I've no objection to this quick fix

But the issue here is using a refcount for something that is not a
refcount. This usage model is should just be a normal atomic if it is
only counting the number of notifications in progress.

To be a refcount to have to follow one of the standard refcount
patterns, ie get rid of the event queue and consistently do:

if (refcount_dec_and_test())
    free_pasid_state()

Which implies the ppr notification done can do a defered free of the
pasid_state.

Jason

Re: [PATCH 1/3] iommu/amd/iommu_v2: Fix pasid_state refcount dec hit 0 warning on pasid unbind

[Vasant Hegde]

Hi Jason,


On 1/24/2023 8:53 PM, Jason Gunthorpe wrote:
> On Tue, Jan 24, 2023 at 10:43:53AM +0000, Vasant Hegde wrote:
>> From: Daniel Marcovitch <dmarcovitch@nvidia.com>
>>
>> When unbinding pasid - a race condition exists vs outstanding page faults.
>>
>> To prevent this, the pasid_state object contains a refcount.
>>     * set to 1 on pasid bind
>>     * incremented on each ppr notification start
>>     * decremented on each ppr notification done
>>     * decremented on pasid unbind
> 
> I've no objection to this quick fix
> 
> But the issue here is using a refcount for something that is not a
> refcount. This usage model is should just be a normal atomic if it is
> only counting the number of notifications in progress.

Originally it was using atomic counter.. which got converted to refcount by
below commit:

commit 8bc54824da4e8fcf0ed679cf09ac32f23d83254a
Author: Xiyu Yang via iommu <iommu@lists.linux-foundation.org>
Date:   Mon Jul 19 16:32:58 2021 +0800

    iommu/amd: Convert from atomic_t to refcount_t on pasid_state->count

.. after this commit it started throwing warnings. We thought of fixing the
warning instead of reverting above commit.


-Vasant


> 
> To be a refcount to have to follow one of the standard refcount
> patterns, ie get rid of the event queue and consistently do:
> 
> if (refcount_dec_and_test())
>     free_pasid_state()
> 
> Which implies the ppr notification done can do a defered free of the
> pasid_state.
> 
> Jason

Re: [PATCH 1/3] iommu/amd/iommu_v2: Fix pasid_state refcount dec hit 0 warning on pasid unbind

[Jason Gunthorpe]

On Fri, Jan 27, 2023 at 10:14:15AM +0530, Vasant Hegde wrote:
> Hi Jason,
> 
> 
> On 1/24/2023 8:53 PM, Jason Gunthorpe wrote:
> > On Tue, Jan 24, 2023 at 10:43:53AM +0000, Vasant Hegde wrote:
> >> From: Daniel Marcovitch <dmarcovitch@nvidia.com>
> >>
> >> When unbinding pasid - a race condition exists vs outstanding page faults.
> >>
> >> To prevent this, the pasid_state object contains a refcount.
> >>     * set to 1 on pasid bind
> >>     * incremented on each ppr notification start
> >>     * decremented on each ppr notification done
> >>     * decremented on pasid unbind
> > 
> > I've no objection to this quick fix
> > 
> > But the issue here is using a refcount for something that is not a
> > refcount. This usage model is should just be a normal atomic if it is
> > only counting the number of notifications in progress.
> 
> Originally it was using atomic counter.. which got converted to refcount by
> below commit:
> 
> commit 8bc54824da4e8fcf0ed679cf09ac32f23d83254a
> Author: Xiyu Yang via iommu <iommu@lists.linux-foundation.org>
> Date:   Mon Jul 19 16:32:58 2021 +0800
> 
>     iommu/amd: Convert from atomic_t to refcount_t on pasid_state->count
> 
> .. after this commit it started throwing warnings. We thought of fixing the
> warning instead of reverting above commit.

The mechanical clean up was wrong because it subtly wasn't a refcount
:)

But given the other problems here you can fix it with the approach I gave
in the other email

Jason

[PATCH 2/3] iommu/amd/iommu_v2: Fix pasid_state->wq race

[Vasant Hegde]

From: Daniel Marcovitch <dmarcovitch@nvidia.com>

pasid_state->wq is used to wait for all outstanding pri handling prior
to pasid unbind.

ppr handler calls the wake_up API on the pasid_state->wq once
pasid_state refcount has reached 0. Since refcount is initialized to 1,
this will only occur during pasid unbind.

However, unbind_pasid may reach refcount check at this point of ppr_handler
execution. Since refcount is 0, it will not wait on wait_queue - but
rather proceed with pasid_state deallocation, including wait_queue.

This can cause unallocated memory access.

Fixed by adding spinlock to ensure atomicity between refcount check
and event queue handling. Moved wait_queue APIs to
wake_up_locked / wake_event_interruptible_locked accordingly.

Signed-off-by: Daniel Marcovitch <dmarcovitch@nvidia.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu_v2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 858748baae0b..44fed4ca576f 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -256,14 +256,18 @@ static void free_pasid_state(struct pasid_state *pasid_state)
 
 static void put_pasid_state(struct pasid_state *pasid_state)
 {
+	spin_lock(&pasid_state->wq.lock);
 	if (refcount_dec_and_test(&pasid_state->count))
-		wake_up(&pasid_state->wq);
+		wake_up_locked(&pasid_state->wq);
+	spin_unlock(&pasid_state->wq.lock);
 }
 
 static void put_pasid_state_wait(struct pasid_state *pasid_state)
 {
+	spin_lock(&pasid_state->wq.lock);
 	if (!refcount_dec_and_test(&pasid_state->count))
-		wait_event(pasid_state->wq, !refcount_read(&pasid_state->count));
+		wait_event_interruptible_locked(pasid_state->wq, !refcount_read(&pasid_state->count));
+	spin_unlock(&pasid_state->wq.lock);
 	free_pasid_state(pasid_state);
 }
 
-- 
2.31.1

Re: [PATCH 2/3] iommu/amd/iommu_v2: Fix pasid_state->wq race

[Jason Gunthorpe]

On Tue, Jan 24, 2023 at 10:43:54AM +0000, Vasant Hegde wrote:
> From: Daniel Marcovitch <dmarcovitch@nvidia.com>
> 
> pasid_state->wq is used to wait for all outstanding pri handling prior
> to pasid unbind.
> 
> ppr handler calls the wake_up API on the pasid_state->wq once
> pasid_state refcount has reached 0. Since refcount is initialized to 1,
> this will only occur during pasid unbind.
> 
> However, unbind_pasid may reach refcount check at this point of ppr_handler
> execution. Since refcount is 0, it will not wait on wait_queue - but
> rather proceed with pasid_state deallocation, including wait_queue.
> 
> This can cause unallocated memory access.
> 
> Fixed by adding spinlock to ensure atomicity between refcount check
> and event queue handling. Moved wait_queue APIs to
> wake_up_locked / wake_event_interruptible_locked accordingly.
> 
> Signed-off-by: Daniel Marcovitch <dmarcovitch@nvidia.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu_v2.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
> index 858748baae0b..44fed4ca576f 100644
> --- a/drivers/iommu/amd/iommu_v2.c
> +++ b/drivers/iommu/amd/iommu_v2.c
> @@ -256,14 +256,18 @@ static void free_pasid_state(struct pasid_state *pasid_state)
>  
>  static void put_pasid_state(struct pasid_state *pasid_state)
>  {
> +	spin_lock(&pasid_state->wq.lock);
>  	if (refcount_dec_and_test(&pasid_state->count))
> -		wake_up(&pasid_state->wq);
> +		wake_up_locked(&pasid_state->wq);
> +	spin_unlock(&pasid_state->wq.lock);
>  }
>  
>  static void put_pasid_state_wait(struct pasid_state *pasid_state)
>  {
> +	spin_lock(&pasid_state->wq.lock);
>  	if (!refcount_dec_and_test(&pasid_state->count))
> -		wait_event(pasid_state->wq, !refcount_read(&pasid_state->count));
> +		wait_event_interruptible_locked(pasid_state->wq, !refcount_read(&pasid_state->count));
> +	spin_unlock(&pasid_state->wq.lock);

You can't call wait_event_interruptible_locked() while holding a
spinlock

What this is really trying to make is one of those open coded rwlocks
that can be passed into a WQ.

struct refcount count
struct completion comp

init:
  refcount_set(count, 1)

get:
  if !refcount_inc_not_zero(count)
     // Concurrent destruction, can't get the pasid state!

put:
  if refcount_dec_and_test(count)
     complete(&device->comp)

destroy:
   put() // put back the refcount_set()
   wait_for_completion()
   // Now count == 0

   clear_pasid_state()
   free()

Jason

[PATCH 3/3] iommu/amd/iommu_v2: Prevent scheduling new ppr notifier during unbind_pasid

[Vasant Hegde]

From: Daniel Marcovitch <dmarcovitch@nvidia.com>

The pasid state wait_queue / ref_count mechanism allows unbind_pasid to
wait for all outstanding ppr requests to be completed prior to freeing
pasid_state.

However, we are still missing a mechanism to prevent new ppr_notifier
being invoked after refcount has been decremented to 0, and prior to
pasid_state deallocation.

This can cause unallocated memory access.

Fixed by changing ref_count_inc to ref_count_inc_not_zero to ensure no
new ppr_handler starts after pasid has been unbound and NULL (invalid)
pasid_state is returned on zero.

Signed-off-by: Daniel Marcovitch <dmarcovitch@nvidia.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu_v2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 44fed4ca576f..264d9aa447c2 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -240,8 +240,13 @@ static struct pasid_state *get_pasid_state(struct device_state *dev_state,
 		goto out_unlock;
 
 	ret = *ptr;
-	if (ret)
-		refcount_inc(&ret->count);
+
+	/*
+	 * Since pasid_state creation sets initial count to 1,
+	 * a count of 0 indicates that unbind_pasid is in progress
+	 */
+	if (ret && !refcount_inc_not_zero(&ret->count))
+		ret = NULL;
 
 out_unlock:
 	spin_unlock_irqrestore(&dev_state->lock, flags);
-- 
2.31.1

Re: [PATCH 3/3] iommu/amd/iommu_v2: Prevent scheduling new ppr notifier during unbind_pasid

[Jason Gunthorpe]

On Tue, Jan 24, 2023 at 10:43:55AM +0000, Vasant Hegde wrote:
> From: Daniel Marcovitch <dmarcovitch@nvidia.com>
> 
> The pasid state wait_queue / ref_count mechanism allows unbind_pasid to
> wait for all outstanding ppr requests to be completed prior to freeing
> pasid_state.
> 
> However, we are still missing a mechanism to prevent new ppr_notifier
> being invoked after refcount has been decremented to 0, and prior to
> pasid_state deallocation.
> 
> This can cause unallocated memory access.
> 
> Fixed by changing ref_count_inc to ref_count_inc_not_zero to ensure no
> new ppr_handler starts after pasid has been unbound and NULL (invalid)
> pasid_state is returned on zero.

It looks like this is prevented by clear_pasid_state() which will NULL
the pasid entry under the spinlock before an attempt is made to zero
the refcount?

Though maybe that is missed in the free_pasid_states() path?

Also this whole thing is just begging to be converted into an xarray..

Jason

Re: [PATCH 3/3] iommu/amd/iommu_v2: Prevent scheduling new ppr notifier during unbind_pasid

[Vasant Hegde]

Jason,


On 1/24/2023 9:03 PM, Jason Gunthorpe wrote:
> On Tue, Jan 24, 2023 at 10:43:55AM +0000, Vasant Hegde wrote:
>> From: Daniel Marcovitch <dmarcovitch@nvidia.com>
>>
>> The pasid state wait_queue / ref_count mechanism allows unbind_pasid to
>> wait for all outstanding ppr requests to be completed prior to freeing
>> pasid_state.
>>
>> However, we are still missing a mechanism to prevent new ppr_notifier
>> being invoked after refcount has been decremented to 0, and prior to
>> pasid_state deallocation.
>>
>> This can cause unallocated memory access.
>>
>> Fixed by changing ref_count_inc to ref_count_inc_not_zero to ensure no
>> new ppr_handler starts after pasid has been unbound and NULL (invalid)
>> pasid_state is returned on zero.
> 
> It looks like this is prevented by clear_pasid_state() which will NULL
> the pasid entry under the spinlock before an attempt is made to zero
> the refcount?
> 
> Though maybe that is missed in the free_pasid_states() path?

You are right. We just need to add clear_pasid_start in above path (something
like below).

----------

diff --git a/drivers/iommu/amd/iommu_v2.c b/drivers/iommu/amd/iommu_v2.c
index 264d9aa447c2..15f4d4bd1f63 100644
--- a/drivers/iommu/amd/iommu_v2.c
+++ b/drivers/iommu/amd/iommu_v2.c
@@ -336,6 +336,8 @@ static void free_pasid_states(struct device_state *dev_state)

 		put_pasid_state(pasid_state);

+		clear_pasid_state(dev_state, pasid);
+
 		/*
 		 * This will call the mn_release function and
 		 * unbind the PASID

> 
> Also this whole thing is just begging to be converted into an xarray..

Agree. Makes sense. We will look into it later.

-Vasant