On Thu, 8 Sep 2011 01:53:29 +0400 Cyrill Gorcunov <gorcunov@xxxxxxxxx> wrote: > On Wed, Sep 07, 2011 at 03:23:01PM +0400, Vasiliy Kulikov wrote: > > Hi, > > > > On Wed, Sep 07, 2011 at 02:33 +0900, Tejun Heo wrote: > > > On Tue, Sep 06, 2011 at 09:29:52PM +0400, Vasiliy Kulikov wrote: > > > > I agree with you. I don't think that showing system-global debug > > > > information to all users by default is the right thing. But some people > > > > doesn't agree with this point of view: > > > > > > > > http://thread.gmane.org/gmane.linux.kernel/1108378 > > > > > > Yeap, I know there are two sides of the discussion but if one takes > > > the position that hiding such global debug info is more harmful, it's > > > only crazier to hide such information from each individual users of > > > the said global facility. So, let's just forget about information > > > leak via freeing or not freeing here. It's the wrong battle field. > > > > Andrew, are you OK with closing the hole with pid_no_revalidate() > > and 0600 /proc/slabinfo? If so, I feel I have to start this discussion > > with people participating in the discussion above: Theodore, Dan, Linus, etc. I fell asleep a long time ago and don't know what pid_no_revalidate() and slabinfo permissions have to do with this. Perhaps summarising the issues in the changelog would be appropriate, dunno. > > By Andrew Morton > > > > But do we *really* need to do it in two passes? Avoiding the temporary > > storage would involve doing more work under mmap_sem, and a put_filp() > > under mmap_sem might be problematic. > > I fear we still need to use two passes in proc_map_files_readdir, I found no way > to escape lockdep complains when doing all work in one pass with mmap_sem taken. > The /maps does the same thing -- ie it fills maps file with mmap_sem taken to produce > robust data. The code's using three passes. > And I'm not really sure what you mean with problematic put_filp? I was thinking fput(), which can do a hell of a lot of stuff if it's the final put on the inode. > > ... > > +static int proc_map_files_readdir(struct file *filp, void *dirent, filldir_t filldir) > +{ > + struct dentry *dentry = filp->f_path.dentry; > + struct inode *inode = dentry->d_inode; > + struct vm_area_struct *vma; > + struct task_struct *task; > + struct mm_struct *mm; > + ino_t ino; > + int ret; > + > + ret = -ENOENT; > + task = get_proc_task(inode); > + if (!task) > + goto out_no_task; > + > + ret = -EPERM; > + if (!ptrace_may_access(task, PTRACE_MODE_READ)) > + goto out; > + > + ret = 0; > + switch (filp->f_pos) { > + case 0: > + ino = inode->i_ino; > + if (filldir(dirent, ".", 1, 0, ino, DT_DIR) < 0) > + goto out; > + filp->f_pos++; > + case 1: > + ino = parent_ino(dentry); > + if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0) > + goto out; > + filp->f_pos++; > + default: > + { > + unsigned long nr_files, used, pos, i; > + struct flex_array *fa = NULL; > + struct map_files_info info; > + struct map_files_info *p; > + > + mm = get_task_mm(task); > + if (!mm) > + goto out; > + down_read(&mm->mmap_sem); > + > + nr_files = 0; > + used = 0; > + > + /* > + * We need two passes here: > + * > + * 1) Collect vmas of mapped files with mmap_sem taken > + * 2) Release mmap_sem and instantiate entries > + * > + * otherwise we get lockdep complained, since filldir() > + * routine might require mmap_sem taken in might_fault(). > + */ > + > + for (vma = mm->mmap; vma; vma = vma->vm_next) { > + if (vma->vm_file) > + nr_files++; > + } > + > + if (nr_files) { > + ret = -ENOMEM; > + fa = flex_array_alloc(sizeof(info), nr_files, GFP_KERNEL); > + if (!fa) > + goto err; > + if (flex_array_prealloc(fa, 0, nr_files, GFP_KERNEL)) > + goto err; > + for (vma = mm->mmap, pos = 2; vma; vma = vma->vm_next) { > + if (!vma->vm_file) > + continue; > + if (++pos <= filp->f_pos) > + continue; > + > + get_file(vma->vm_file); > + info.file = vma->vm_file; > + info.len = snprintf(info.name, sizeof(info.name), > + "%lx-%lx", vma->vm_start, > + vma->vm_end); > + if (flex_array_put(fa, used, &info, GFP_KERNEL)) { > + /* > + * This must never happen on preallocated array, > + * but just to be sure. > + */ > + WARN_ON_ONCE(1); > + put_filp(vma->vm_file); > + goto err; > + } > + used++; > + } > + ret = 0; > + } > +err: > + up_read(&mm->mmap_sem); > + > + for (i = 0; i < used && !ret; i++) { The "&& !ret" is unneeded? > + p = flex_array_get(fa, i); > + ret = proc_fill_cache(filp, dirent, filldir, > + p->name, p->len, > + proc_map_files_instantiate, > + task, p->file); > + if (ret) > + break; > + filp->f_pos++; > + put_filp(p->file); > + } > + > + for (; i < used; i++) { > + p = flex_array_get(fa, i); > + put_filp(p->file); > + } Still unclear why we need the third loop. > + if (fa) > + flex_array_free(fa); > + > + mmput(mm); > + } > + } > + > +out: > + put_task_struct(task); > +out_no_task: > + return ret; > +} > > ... > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
