Re: [PATCH] mm/migrate: Avoid possible unnecessary ptrace_may_access() call in kernel_move_pages()

[Matthew Wilcox]

On Mon, Aug 17, 2020 at 07:59:33AM -0400, Miaohe Lin wrote:
> There is no need to check if this process has the right to modify the
> specified process when they are same.

We should probably also skip the security hook call if a process is
modifying its own pages.

How about this instead?

diff --git a/mm/migrate.c b/mm/migrate.c
index f37729673558..84dc1f42161d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1797,33 +1797,22 @@ static int do_pages_stat(struct mm_struct *mm, unsigned 
long nr_pages,
        return nr_pages ? -EFAULT : 0;
 }
 
-/*
- * Move a list of pages in the address space of the currently executing
- * process.
- */
-static int kernel_move_pages(pid_t pid, unsigned long nr_pages,
-                            const void __user * __user *pages,
-                            const int __user *nodes,
-                            int __user *status, int flags)
+static struct mm_struct *find_mm_struct(pid_t pid, nodemask_t *mem_nodes)
 {
        struct task_struct *task;
        struct mm_struct *mm;
-       int err;
-       nodemask_t task_nodes;
-
-       /* Check flags */
-       if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
-               return -EINVAL;
 
-       if ((flags & MPOL_MF_MOVE_ALL) && !capable(CAP_SYS_NICE))
-               return -EPERM;
+       if (!pid) {
+               mmget(current->mm);
+               *mem_nodes = cpuset_mems_allowed(current);
+               return current->mm;
+       }
 
-       /* Find the mm_struct */
        rcu_read_lock();
-       task = pid ? find_task_by_vpid(pid) : current;
+       task = find_task_by_vpid(pid);
        if (!task) {
                rcu_read_unlock();
-               return -ESRCH;
+               return ERR_PTR(-ESRCH);
        }
        get_task_struct(task);
 
@@ -1833,22 +1822,49 @@ static int kernel_move_pages(pid_t pid, unsigned long 
nr_pages,
         */
        if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
                rcu_read_unlock();
-               err = -EPERM;
+               mm = ERR_PTR(-EPERM);
                goto out;
        }
        rcu_read_unlock();
 
-       err = security_task_movememory(task);
-       if (err)
+       mm = ERR_PTR(security_task_movememory(task));
+       if (IS_ERR(mm))
                goto out;
 
-       task_nodes = cpuset_mems_allowed(task);
+       *mem_nodes = cpuset_mems_allowed(task);
        mm = get_task_mm(task);
+out:
        put_task_struct(task);
 
        if (!mm)
+               mm = ERR_PTR(-EINVAL);
+       return mm;
+}
+
+/*
+ * Move a list of pages in the address space of the specified
+ * process (pid of 0 means current).
+ */
+static int kernel_move_pages(pid_t pid, unsigned long nr_pages,
+                            const void __user * __user *pages,
+                            const int __user *nodes,
+                            int __user *status, int flags)
+{
+       struct mm_struct *mm;
+       int err;
+       nodemask_t task_nodes;
+
+       /* Check flags */
+       if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
                return -EINVAL;
 
+       if ((flags & MPOL_MF_MOVE_ALL) && !capable(CAP_SYS_NICE))
+               return -EPERM;
+
+       mm = find_mm_struct(pid, &task_nodes);
+       if (IS_ERR(mm))
+               return PTR_ERR(mm);
+
        if (nodes)
                err = do_pages_move(mm, task_nodes, nr_pages, pages,
                                    nodes, status, flags);
@@ -1857,10 +1873,6 @@ static int kernel_move_pages(pid_t pid, unsigned long 
nr_pages,
 
        mmput(mm);
        return err;
-
-out:
-       put_task_struct(task);
-       return err;
 }
 
 SYSCALL_DEFINE6(move_pages, pid_t, pid, unsigned long, nr_pages,