On 03/04/2018 11:43, Frederic Barrat wrote:
> cxllib_handle_fault() is called by an external driver when it needs to
> have the host process page faults for a buffer which may cover several
> pages. Currently the function holds the mm->mmap_sem semaphore with
> read access while iterating over the buffer, since it could spawn
> several VMAs. When calling a lower-level function to handle the page
> fault for a single page, the semaphore is accessed again in read
> mode. That is wrong and can lead to deadlocks if a writer tries to
> sneak in while a buffer of several pages is being processed.
>
> The fix is to release the semaphore once cxllib_handle_fault() got the
> information it needs from the current vma. The address space/VMAs
> could evolve while we iterate over the full buffer, but in the
> unlikely case where we miss a page, the driver will raise a new page
> fault when retrying.
>
> Fixes: 3ced8d730063 ("cxl: Export library to support IBM XSL")
> Cc: sta...@vger.kernel.org # 4.13+
> Signed-off-by: Frederic Barrat <fbar...@linux.vnet.ibm.com>
FWIW,
Reviewed-by : Laurent Dufour <lduf...@linux.vnet.ibm.com>
> ---
> 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);
>
