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

[Frederic Barrat]

Le 03/04/2018 à 17:31, Aneesh Kumar K.V a écrit :
On 04/03/2018 08:10 PM, Aneesh Kumar K.V wrote:
On 04/03/2018 03:13 PM, 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>
---
  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) {


IIUC, we are fetching the vma to get just the page_size with which it 
is mapped? Can't we iterate with PAGE_SIZE? Considering hugetlb page 
size will be larger than PAGE_SIZE, we might call into 
cxl_handle_mm_fault multiple times for a hugetlb page. Does that cause 
any issue? Also can cxl be used with hugetlb mappings?

Can you also try to use a helper like below. That will clarify the need 
of find_vma there.

static int __cxllib_handle_fault(struct mm_struct *mm, unsigned long 
start, unsigned long end,
                  unsigned long mapping_psize, u64 flags)
{
     int rc;
     unsigned long dar;

     for (dar = start; dar < end; dar += mapping_psize) {
         rc = cxl_handle_mm_fault(mm, flags, dar);
         if (rc) {
             rc = -EFAULT;
             goto out;
         }
     }
     rc = 0;
out:
     return rc;
}


I'm struggling to make good use of it. I see your point, it makes 
touching all the pages for a given VMA easy (same page size).
But I'm given a buffer, which can span several VMAs, and we can have a 
varying page size. So I now need an outside loop iterating over all the 
VMAs and call the helper for a subset of the VMA. IMHO, it's not making 
the code any easier, i.e what I gain in the helper is lost in the 
outside VMA loop.
The current code, with a single loop and a varying increment based on 
the page size, doesn't look that bad to me.

  Fred