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

[Jason Gunthorpe <jgg@nvidia.com>]

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