Re: [PATCH v2 5/6] proc: use down_read_killable mmap_sem for /proc/pid/map_files

[Andrew Morton]

On Wed, 12 Jun 2019 16:14:28 -0700 Andrei Vagin <ava...@gmail.com> wrote:

> On Sun, Jun 09, 2019 at 01:09:00PM +0300, Konstantin Khlebnikov wrote:
> > Do not stuck forever if something wrong.
> > Killable lock allows to cleanup stuck tasks and simplifies investigation.
> 
> This patch breaks the CRIU project, because stat() returns EINTR instead
> of ENOENT:
> 
> [root@fc24 criu]# stat /proc/self/map_files/0-0
> stat: cannot stat '/proc/self/map_files/0-0': Interrupted system call
> 
> Here is one inline comment with the fix for this issue.
> 
> > @@ -2107,7 +2113,10 @@ static struct dentry *proc_map_files_lookup(struct 
> > inode *dir,
> >     if (!mm)
> >             goto out_put_task;
> >  
> > -   down_read(&mm->mmap_sem);
> > +   result = ERR_PTR(-EINTR);
> > +   if (down_read_killable(&mm->mmap_sem))
> > +           goto out_put_mm;
> > +
> 
>       result = ERR_PTR(-ENOENT);
> 

yes, thanks.

--- 
a/fs/proc/base.c~proc-use-down_read_killable-mmap_sem-for-proc-pid-map_files-fix
+++ a/fs/proc/base.c
@@ -2117,6 +2117,7 @@ static struct dentry *proc_map_files_loo
        if (down_read_killable(&mm->mmap_sem))
                goto out_put_mm;
 
+       result = ERR_PTR(-ENOENT);
        vma = find_exact_vma(mm, vm_start, vm_end);
        if (!vma)
                goto out_no_vma;



We started doing this trick of using

        ret = -EFOO;
        if (something)
                goto out;

decades ago because it generated slightly better code.  I rather doubt
whether that's still the case.

In fact this:

--- a/fs/proc/base.c~a
+++ a/fs/proc/base.c
@@ -2096,35 +2096,45 @@ static struct dentry *proc_map_files_loo
        struct dentry *result;
        struct mm_struct *mm;
 
-       result = ERR_PTR(-ENOENT);
        task = get_proc_task(dir);
-       if (!task)
+       if (!task) {
+               result = ERR_PTR(-ENOENT);
                goto out;
+       }
 
-       result = ERR_PTR(-EACCES);
-       if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS))
+       if (!ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS)) {
+               result = ERR_PTR(-EACCES);
                goto out_put_task;
+       }
 
-       result = ERR_PTR(-ENOENT);
-       if (dname_to_vma_addr(dentry, &vm_start, &vm_end))
+       if (dname_to_vma_addr(dentry, &vm_start, &vm_end)) {
+               result = ERR_PTR(-ENOENT);
                goto out_put_task;
+       }
 
        mm = get_task_mm(task);
-       if (!mm)
+       if (!mm) {
+               result = ERR_PTR(-ENOENT);
                goto out_put_task;
+       }
 
-       result = ERR_PTR(-EINTR);
-       if (down_read_killable(&mm->mmap_sem))
+       if (down_read_killable(&mm->mmap_sem)) {
+               result = ERR_PTR(-EINTR);
                goto out_put_mm;
+       }
 
-       result = ERR_PTR(-ENOENT);
        vma = find_exact_vma(mm, vm_start, vm_end);
-       if (!vma)
+       if (!vma) {
+               result = ERR_PTR(-ENOENT);
                goto out_no_vma;
+       }
 
-       if (vma->vm_file)
+       if (vma->vm_file) {
                result = proc_map_files_instantiate(dentry, task,
                                (void *)(unsigned long)vma->vm_file->f_mode);
+       } else {
+               result = ERR_PTR(-ENOENT);
+       }
 
 out_no_vma:
        up_read(&mm->mmap_sem);

makes no change to the generated assembly with gcc-7.2.0.

And I do think that the above style is clearer and more reliable, as
this bug demonstrates.