Re: [PATCH v2] cxl: Fix possible deadlock when processing page faults from cxllib

[Laurent Dufour <ldufour@linux.vnet.ibm.com>]

On 03/04/2018 15:54, Frederic Barrat wrote:
> cxllib_handle_fault() is called by an external driver when it needs to
> have the host resolve page faults for a buffer. The buffer can cover
> several pages and VMAs. The function iterates over all the pages used
> by the buffer, based on the page size of the VMA.
> 
> To ensure some stability while processing the faults, the thread T1
> grabs the mm->mmap_sem semaphore with read access (R1). However, when
> processing a page fault for a single page, one of the underlying
> functions, copro_handle_mm_fault(), also grabs the same semaphore with
> read access (R2). So the thread T1 takes the semaphore twice.
> 
> If another thread T2 tries to access the semaphore in write mode W1
> (say, because it wants to allocate memory and calls 'brk'), then that
> thread T2 will have to wait because there's a reader (R1). If the
> thread T1 is processing a new page at that time, it won't get an
> automatic grant at R2, because there's now a writer thread
> waiting (T2). And we have a deadlock.
> 
> The timeline is:
> 1. thread T1 owns the semaphore with read access R1
> 2. thread T2 requests write access W1 and waits
> 3. thread T1 requests read access R2 and waits
> 
> The fix is for the thread T1 to release the semaphore R1 once it got
> the information it needs from the current VMA. The address space/VMAs
> could evolve while T1 iterates over the full buffer, but in the
> unlikely case where T1 misses a page, the external driver will raise a
> new page fault when retrying the memory access.
> 
> Fixes: 3ced8d730063 ("cxl: Export library to support IBM XSL")
> Cc: stable@vger.kernel.org # 4.13+
> Signed-off-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com>

What are the changes introduced in this v2 ?

> ---
>  drivers/misc/cxl/cxllib.c | 85 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/misc/cxl/cxllib.c b/drivers/misc/cxl/cxllib.c
> index 30ccba436b3b..55cd35d1a9cc 100644
> --- a/drivers/misc/cxl/cxllib.c
> +++ b/drivers/misc/cxl/cxllib.c
> @@ -208,49 +208,74 @@ int cxllib_get_PE_attributes(struct task_struct *task,
>  }
>  EXPORT_SYMBOL_GPL(cxllib_get_PE_attributes);
> 
> -int cxllib_handle_fault(struct mm_struct *mm, u64 addr, u64 size, u64 flags)
> +static int get_vma_info(struct mm_struct *mm, u64 addr,
> +			u64 *vma_start, u64 *vma_end,
> +			unsigned long *page_size)
>  {
> -	int rc;
> -	u64 dar;
>  	struct vm_area_struct *vma = NULL;
> -	unsigned long page_size;
> -
> -	if (mm == NULL)
> -		return -EFAULT;
> +	int rc = 0;
> 
>  	down_read(&mm->mmap_sem);
> 
>  	vma = find_vma(mm, addr);
>  	if (!vma) {
> -		pr_err("Can't find vma for addr %016llx\n", addr);
>  		rc = -EFAULT;
>  		goto out;
>  	}
> -	/* get the size of the pages allocated */
> -	page_size = vma_kernel_pagesize(vma);
> -
> -	for (dar = (addr & ~(page_size - 1)); dar < (addr + size); dar += page_size) {
> -		if (dar < vma->vm_start || dar >= vma->vm_end) {
> -			vma = find_vma(mm, addr);
> -			if (!vma) {
> -				pr_err("Can't find vma for addr %016llx\n", addr);
> -				rc = -EFAULT;
> -				goto out;
> -			}
> -			/* get the size of the pages allocated */
> -			page_size = vma_kernel_pagesize(vma);
> +	*page_size = vma_kernel_pagesize(vma);
> +	*vma_start = vma->vm_start;
> +	*vma_end = vma->vm_end;
> +out:
> +	up_read(&mm->mmap_sem);
> +	return rc;
> +}
> +
> +int cxllib_handle_fault(struct mm_struct *mm, u64 addr, u64 size, u64 flags)
> +{
> +	int rc;
> +	u64 dar, vma_start, vma_end;
> +	unsigned long page_size;
> +
> +	if (mm == NULL)
> +		return -EFAULT;
> +
> +	/*
> +	 * The buffer we have to process can extend over several pages
> +	 * and may also cover several VMAs.
> +	 * We iterate over all the pages. The page size could vary
> +	 * between VMAs.
> +	 */
> +	rc = get_vma_info(mm, addr, &vma_start, &vma_end, &page_size);
> +	if (rc)
> +		return rc;
> +
> +	for (dar = (addr & ~(page_size - 1)); dar < (addr + size);
> +	     dar += page_size) {
> +		if (dar < vma_start || dar >= vma_end) {
> +			/*
> +			 * We don't hold the mm->mmap_sem semaphore
> +			 * while iterating, since the semaphore is
> +			 * required by one of the lower-level page
> +			 * fault processing functions and it could
> +			 * create a deadlock.
> +			 *
> +			 * It means the VMAs can be altered between 2
> +			 * loop iterations and we could theoretically
> +			 * miss a page (however unlikely). But that's
> +			 * not really a problem, as the driver will
> +			 * retry access, get another page fault on the
> +			 * missing page and call us again.
> +			 */
> +			rc = get_vma_info(mm, dar, &vma_start, &vma_end,
> +					&page_size);
> +			if (rc)
> +				return rc;
>  		}
> 
>  		rc = cxl_handle_mm_fault(mm, flags, dar);
> -		if (rc) {
> -			pr_err("cxl_handle_mm_fault failed %d", rc);
> -			rc = -EFAULT;
> -			goto out;
> -		}
> +		if (rc)
> +			return -EFAULT;
>  	}
> -	rc = 0;
> -out:
> -	up_read(&mm->mmap_sem);
> -	return rc;
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(cxllib_handle_fault);
>