fs, proc: Introduce the /proc/<pid>/map_files/ directory v2
Subsystems (2)
RFC 1/1 fs, proc: Introduce the /proc/<pid>/map_files/ directory v2
From: Pavel Emelyanov <[email protected]>
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
This thing is aimed to help checkpointing processes.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73a00000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1b000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1c000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73e00000 -> /lib64/libc-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73f4e000 -> /lib64/libc-2.5.so
v2:
- /proc/<pid>/mfd changed to /proc/<pid>/map_files
- find_vma helper is used instead of linear search
- routines are re-grouped
- .d_revalidate is set now
--- fs/proc/base.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/proc_fs.h | 5 + 2 files changed, 195 insertions(+), 1 deletion(-)
Please review. This is a part of previous c/r patchset as well but I guess it might be treated separately.
Objections, complains, comments are highly appreciated!
fs/proc/base.c +191 -0
@@ -2170,6 +2170,196 @@ static const struct file_operations proc .llseek = default_llseek, }; +static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = pid_revalidate, + .d_delete = pid_delete_dentry, +}; + +static int proc_map_files_get_link (struct inode *inode,struct path *path) +{ +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +unsigned long vm_start; +int rc = -ENOENT ; + + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); + +if (!mm) + goto out; + + vm_start =PROC_I (inode)->vm_start; + +down_read (&mm->mmap_sem); + vma =find_vma (mm, vm_start); +if (vma && vma->vm_start == vm_start && vma->vm_file) { + *path = vma->vm_file->f_path; +path_get (path); + rc =0 ; + } +up_read (&mm->mmap_sem); + +mmput (mm); + +out: +return rc; +} + +static struct dentry * +proc_map_files_instantiate (struct inode *dir,struct dentry *dentry, +struct task_struct *task,const void *ptr) +{ +const struct vm_area_struct *vma = ptr; +struct file *file = vma->vm_file; +struct proc_inode *ei; +struct inode *inode; + +if (!file) +return ERR_PTR (-ENOENT ); + + inode =proc_pid_make_inode (dir->i_sb, task); +if (!inode) +return ERR_PTR (-ENOENT ); + + ei =PROC_I (inode); + ei->vm_start = vma->vm_start; + ei->op.proc_get_link = proc_map_files_get_link; + + inode->i_op = &proc_pid_link_inode_operations; + inode->i_size =64 ; + inode->i_mode =S_IFLNK ; + +if (file->f_mode &FMODE_READ ) + inode->i_mode |=S_IRUSR |S_IXUSR ; +if (file->f_mode &FMODE_WRITE ) + inode->i_mode |=S_IWUSR |S_IXUSR ; + +d_set_d_op (dentry, &tid_map_files_dentry_operations); +d_add (dentry, inode); + +return NULL ; +} + +static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +unsigned long vm_start; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct dentry *result; +char *endp; + + result =ERR_PTR (-ENOENT ); + + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + vm_start =simple_strtoul (dentry->d_name.name, &endp,16 ); +if (*endp !='\0' ) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_vma (mm, vm_start); +if (!vma || vma->vm_start != vm_start) + goto out_no_vma; + result =proc_map_files_instantiate (dir, dentry, task, vma); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +} + +static const struct inode_operations proc_map_files_inode_operations = { + .lookup = proc_map_files_lookup, + .setattr = proc_setattr, +}; + +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; +unsigned int vmai; +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 : + mm =get_task_mm (task); +if (!mm) + goto out; +down_read (&mm->mmap_sem); +for (vma = mm->mmap, vmai =2 ; vma; vma = vma->vm_next) { +char name[2 +16 +1 ]; +int len; + +if (!vma->vm_file) +continue ; + + vmai++; +if (vmai < filp->f_pos) +continue ; + + filp->f_pos++; + len =snprintf (name,sizeof (name),"0x%lx" , vma->vm_start); +if (proc_fill_cache (filp, dirent, filldir, + name, len, proc_map_files_instantiate, + task, vma) <0 ) +break ; + } +up_read (&mm->mmap_sem); +mmput (mm); + } + +out: +put_task_struct (task); +out_no_task: +return ret; +} + +static const struct file_operations proc_map_files_operations = { + .read = generic_read_dir, + .readdir = proc_map_files_readdir, + .llseek = default_llseek, +}; +/* * /proc/pid/fd needs a special permission handler so that a process can still * access /proc/self/fd after it has executed a setuid().
@@ -2785,6 +2975,7 @@ static const struct inode_operations prostatic const struct pid_entry tgid_base_stuff[] = {DIR ("task" ,S_IRUGO |S_IXUGO , proc_task_inode_operations, proc_task_operations),DIR ("fd" ,S_IRUSR |S_IXUSR , proc_fd_inode_operations, proc_fd_operations), +DIR ("map_files" ,S_IRUSR |S_IXUSR , proc_map_files_inode_operations, proc_map_files_operations),DIR ("fdinfo" ,S_IRUSR |S_IXUSR , proc_fdinfo_inode_operations, proc_fdinfo_operations),DIR ("ns" ,S_IRUSR |S_IXUGO , proc_ns_dir_inode_operations, proc_ns_dir_operations), #ifdef CONFIG_NET
include/linux/proc_fs.h +4 -1
@@ -265,7 +265,10 @@ struct ctl_table;struct proc_inode {struct pid *pid; -int fd; +union { +int fd; +unsigned long vm_start; + };union proc_op op;struct proc_dir_entry *pde;struct ctl_table_header *sysctl;
Quoted 8 lines · Cyrill Gorcunov
From: Pavel Emelyanov <[email protected]>
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.This thing is aimed to help checkpointing processes.
OK, but you really should explain _how_ this will help with checkpointing.
Pekka
Quoted 11 lines · Pekka Enberg
Quoted 8 lines · Cyrill Gorcunov
From: Pavel Emelyanov <[email protected]>
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.This thing is aimed to help checkpointing processes.
OK, but you really should explain _how_ this will help with checkpointing.
This helps in three ways:
1. When dumping a task mappings we do know exact file that is mapped by particular region. We do this by opening /proc/pid/map_files/address symlink the way we do with file descriptors.
2. This also helps in determining which anonymous shared mappings are shared with each other by comparing the inodes of them.
3. When restoring a set of process in case two of them has a mapping shared, we map the memory by the 1st one and then open its /proc/pid/map_files/address file and map it by the 2nd task.
Quoted 3 lines
Pekka
.
Hello,
fs/proc/base.c static const struct file_operations proc · line 2176
Quoted 4 lines · Cyrill Gorcunov
fs/proc/base.c +4
-0+static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = pid_revalidate, + .d_delete = pid_delete_dentry, +};
Why pid_revalidate? Shouldn't it be verifying the entry against the current vmas? vmas (of course) can change while the process is running.
Thanks.
Signature · 1 line
tejun
Quoted 12 lines · Tejun Heo
Hello,
fs/proc/base.c static const struct file_operations proc · line 2176
Quoted 4 lines · Cyrill Gorcunov
fs/proc/base.c +4
-0+static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = pid_revalidate, + .d_delete = pid_delete_dentry, +};Why
pid_revalidate? Shouldn't it be verifying the entry against the current vmas? vmas (of course) can change while the process is running.
Quoted 2 lines
Thanks.
Hmm, good point, letme check...
Cyrill
On Wed, Aug 24, 2011 at 01:37:52PM +0400, Cyrill Gorcunov wrote:
...
Quoted 3 lines
Hmm, good point, letme check...
Yes, Tejun, we need an own revalidate here! So'll update the patch anyway (will put Pavel's comments into changelog). I somehow forgot in first place that this entries might be referred even if process is not stopped. Thanks for chatching it!
Cyrill
On Wed, Aug 24, 2011 at 01:37:52PM +0400, Cyrill Gorcunov wrote:
...
Quoted 3 lines
Hmm, good point, letme check...
Yes, Tejun, we need an own revalidate here! So'll update the patch anyway (will put Pavel's comments into changelog). I somehow forgot in first place that this entries might be referred even if process is not stopped. Thanks for chatching it!
Cyrill
Hi Cyrill,
Quoted 28 lines · Cyrill Gorcunov
fs/proc/base.c +28
-0+static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +unsigned long vm_start; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct dentry *result; +char *endp; + + result =ERR_PTR (-ENOENT ); + + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + vm_start =simple_strtoul (dentry->d_name.name, &endp,16 ); +if (*endp !='\0' ) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_vma (mm, vm_start); +if (!vma || vma->vm_start != vm_start) + goto out_no_vma;
fs/proc/base.c static const struct file_operations proc · line 2285
Quoted 10 lines
fs/proc/base.c +10
-0+ result =proc_map_files_instantiate (dir, dentry, task, vma); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +}
You have no ptrace_may_access() check in ->lookup(), only in ->readdir().
Quoted 7 lines
fs/proc/base.c +7
-0+static const struct inode_operations proc_map_files_inode_operations = { + .lookup = proc_map_files_lookup, + .setattr = proc_setattr, +}; + +static int proc_map_files_readdir (struct file *filp,void *dirent,filldir_t filldir) +{
...
Quoted 3 lines
fs/proc/base.c +3
-0+ ret = -EPERM ; +if (!ptrace_may_access (task,PTRACE_MODE_READ )) + goto out;
...
Review
Quoted 1 line
(quoted diff) +1
-0+ }
Thanks,
Signature · 1 line
Vasiliy
On Wed, Aug 24, 2011 at 03:18:06PM +0400, Vasiliy Kulikov wrote:
...
Quoted 3 lines
You have no
ptrace_may_access()check in ->lookup(), only in ->readdir().
Thanks Vasiliy! I'm reworking it anyway ;)
Cyrill
On Wed, Aug 24, 2011 at 03:18:06PM +0400, Vasiliy Kulikov wrote:
...
Quoted 3 lines
You have no
ptrace_may_access()check in ->lookup(), only in ->readdir().
A huge thanks to all for feedback! Please review this updated version. The main changes are at v3 mark in changelog. Complains (as always) are welcome ;)
Cyrill
---
From: Pavel Emelyanov <[email protected]>
fs, proc: Introduce the /proc/<pid>/map_files/ directory v3
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73a00000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1b000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1c000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73e00000 -> /lib64/libc-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73f4e000 -> /lib64/libc-2.5.so
This helps checkpointing process in three ways:
1. When dumping a task mappings we do know exact file that is mapped by particular region. We do this by opening /proc/pid/map_files/address symlink the way we do with file descriptors.
2. This also helps in determining which anonymous shared mappings are shared with each other by comparing the inodes of them.
3. When restoring a set of process in case two of them has a mapping shared, we map the memory by the 1st one and then open its /proc/pid/map_files/address file and map it by the 2nd task.
v2:
- /proc/<pid>/mfd changed to /proc/<pid>/map_files
- find_vma helper is used instead of linear search
- routines are re-grouped
- d_revalidate is set now
v3:
- d_revalidate reworked, now it should drops no longer valid dentries
- ptrace_may_access added into proc_map_files_lookup
- because of filldir (which eventually might need to lock mmap_sem) the proc_map_files_readdir() was reworked to call proc_fill_cache() with unlocked mmap_sem
--- fs/proc/base.c | 278 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/proc_fs.h | 5 2 files changed, 282 insertions(+), 1 deletion(-)
fs/proc/base.c +278 -0
@@ -2170,6 +2170,283 @@ static const struct file_operations proc .llseek = default_llseek, }; +static struct vm_area_struct *find_exact_vma (struct mm_struct *mm,unsigned long vm_start) +{ +struct vm_area_struct *vma =find_vma (mm, vm_start); +if (vma && vma->vm_start != vm_start) + vma =NULL ; +return vma; +} + +static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct inode *inode; + +if (nd && nd->flags &LOOKUP_RCU ) +return -ECHILD ; + + inode = dentry->d_inode; + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm,PROC_I (inode)->vm_start); +up_read (&mm->mmap_sem); +mmput (mm); + +if (vma) +return 1 ; +out: +d_drop (dentry); +return 0 ; +} + +static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = map_files_d_revalidate, + .d_delete = pid_delete_dentry, +}; + +static int proc_map_files_get_link (struct inode *inode,struct path *path) +{ +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +int rc = -ENOENT ; + + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm,PROC_I (inode)->vm_start); +if (vma && vma->vm_file) { + *path = vma->vm_file->f_path; +path_get (path); + rc =0 ; + } +up_read (&mm->mmap_sem); + +mmput (mm); + +out: +return rc; +} + +struct map_files_info { +struct file *file; +unsigned long vm_start; +unsigned char name[24 ]; +unsigned long len; +}; + +static struct dentry * +proc_map_files_instantiate (struct inode *dir,struct dentry *dentry, +struct task_struct *task,const void *ptr) +{ +const struct map_files_info *info = ptr; +struct proc_inode *ei; +struct inode *inode; + +if (!info->file) +return ERR_PTR (-ENOENT ); + + inode =proc_pid_make_inode (dir->i_sb, task); +if (!inode) +return ERR_PTR (-ENOENT ); + + ei =PROC_I (inode); + ei->vm_start = info->vm_start; + ei->op.proc_get_link = proc_map_files_get_link; + + inode->i_op = &proc_pid_link_inode_operations; + inode->i_size =64 ; + inode->i_mode =S_IFLNK ; + +if (info->file->f_mode &FMODE_READ ) + inode->i_mode |=S_IRUSR |S_IXUSR ; +if (info->file->f_mode &FMODE_WRITE ) + inode->i_mode |=S_IWUSR |S_IXUSR ; + +d_set_d_op (dentry, &tid_map_files_dentry_operations); +d_add (dentry, inode); + +return NULL ; +} + +static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +unsigned long vm_start; +struct vm_area_struct *vma; +struct map_files_info info; +struct mm_struct *mm; +struct dentry *result; +char *endp; + + result =ERR_PTR (-ENOENT ); + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm; + + vm_start =simple_strtoul (dentry->d_name.name, &endp,16 ); +if (*endp !='\0' ) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start); +if (!vma) + goto out_no_vma; + +memset (&info,0 ,sizeof (info)); + info.file = vma->vm_file; + info.vm_start = vm_start; + + result =proc_map_files_instantiate (dir, dentry, task, &info); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +} + +static const struct inode_operations proc_map_files_inode_operations = { + .lookup = proc_map_files_lookup, + .setattr = proc_setattr, +}; + +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; +unsigned int vmai; +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, i; +struct map_files_info *info; + + mm =get_task_mm (task); +if (!mm) + goto out; +down_read (&mm->mmap_sem); + + nr_files =0 ; +for (vma = mm->mmap; vma; vma = vma->vm_next) { +if (vma->vm_file) + nr_files++; + } +if (!nr_files) + goto out; + + info =kmalloc (nr_files *sizeof (*info),GFP_KERNEL ); +if (!info) { + ret = -ENOMEM ; + goto out; + } + + used =0 ; +for (vma = mm->mmap, vmai =2 ; vma; vma = vma->vm_next) { +if (!vma->vm_file) +continue ; + vmai++; +if (vmai <= filp->f_pos) +continue ; + +get_file (vma->vm_file); + info[used].file = vma->vm_file; + info[used].vm_start= vma->vm_start; + + info[used].len =snprintf (info[used].name, +sizeof (info[used].name), +"0x%lx" , vma->vm_start); + used++; + } + +up_read (&mm->mmap_sem); + +for (i =0 ; i < used; i++) { + ret =proc_fill_cache (filp, dirent, filldir, + info[i].name, + info[i].len, + proc_map_files_instantiate, + task, &info[i]); +if (ret) +break ; + filp->f_pos++; + } + +for (i =0 ; i < used; i++) +put_filp (info[i].file); + +kfree (info); +mmput (mm); + } + } + +out: +put_task_struct (task); +out_no_task: +return ret; +} + +static const struct file_operations proc_map_files_operations = { + .read = generic_read_dir, + .readdir = proc_map_files_readdir, + .llseek = default_llseek, +}; +/* * /proc/pid/fd needs a special permission handler so that a process can still * access /proc/self/fd after it has executed a setuid().
@@ -2785,6 +3062,7 @@ static const struct inode_operations prostatic const struct pid_entry tgid_base_stuff[] = {DIR ("task" ,S_IRUGO |S_IXUGO , proc_task_inode_operations, proc_task_operations),DIR ("fd" ,S_IRUSR |S_IXUSR , proc_fd_inode_operations, proc_fd_operations), +DIR ("map_files" ,S_IRUSR |S_IXUSR , proc_map_files_inode_operations, proc_map_files_operations),DIR ("fdinfo" ,S_IRUSR |S_IXUSR , proc_fdinfo_inode_operations, proc_fdinfo_operations),DIR ("ns" ,S_IRUSR |S_IXUGO , proc_ns_dir_inode_operations, proc_ns_dir_operations), #ifdef CONFIG_NET
include/linux/proc_fs.h +4 -1
@@ -265,7 +265,10 @@ struct ctl_table;struct proc_inode {struct pid *pid; -int fd; +union { +int fd; +unsigned long vm_start; + };union proc_op op;struct proc_dir_entry *pde;struct ctl_table_header *sysctl;
Hello,
Quoted 5 lines · Cyrill Gorcunov
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73a00000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1b000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1c000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73e00000 -> /lib64/libc-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73f4e000 -> /lib64/libc-2.5.so
I would prefer if the filename included both start and end addresses so that it matches the first column of /proc/PID/maps and ls'ing the directory is more useful. What do other people think?
Quoted 17 lines
fs/proc/base.c +17
-0+static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct inode *inode; + +if (nd && nd->flags &LOOKUP_RCU ) +return -ECHILD ; + + inode = dentry->d_inode; + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task);
fs/proc/base.c static const struct file_operations proc · line 2207
Quoted 10 lines
fs/proc/base.c +10
-0+if (!mm) + goto out; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm,PROC_I (inode)->vm_start); +up_read (&mm->mmap_sem); +mmput (mm); + +if (vma) +return 1 ;
Hmm... don't we need the same credential update as tid_fd_revalidate()? If the task seteuid's, we want the permissions to change accordingly, right?
Thanks.
Signature · 1 line
tejun
Quoted 9 lines · Tejun Heo
Hello,
Quoted 5 lines · Cyrill Gorcunov
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73a00000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1b000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1c000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73e00000 -> /lib64/libc-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73f4e000 -> /lib64/libc-2.5.so
Quoted 3 lines
I would prefer if the filename included both start and end addresses so that it matches the first column of
/proc/PID/mapsand ls'ing the directory is more useful. What do other people think?
Without 0x at the beginning this is not very convenient, but once we add them it no longer matches the contents of the /proc/pid/maps.
Just an opinion, do not mind adding <end> to the name.
Quoted 34 lines
Quoted 17 lines
fs/proc/base.c +17
-0+static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct inode *inode; + +if (nd && nd->flags &LOOKUP_RCU ) +return -ECHILD ; + + inode = dentry->d_inode; + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task);fs/proc/base.c static const struct file_operations proc · line 2207
Quoted 10 lines
fs/proc/base.c +10
-0+if (!mm) + goto out; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm,PROC_I (inode)->vm_start); +up_read (&mm->mmap_sem); +mmput (mm); + +if (vma) +return 1 ;Hmm... don't we need the same credential update as
tid_fd_revalidate()? If the task seteuid's, we want the permissions to change accordingly, right?Thanks.
On Thu, Aug 25, 2011 at 09:05:22PM +0400, Pavel Emelyanov wrote:
...
Quoted 10 lines
Quoted 4 lines
I would prefer if the filename included both start and end addresses so that it matches the first column of
/proc/PID/mapsand ls'ing the directory is more useful. What do other people think?Without 0x at the beginning this is not very convenient, but once we add them it no longer matches the contents of the
/proc/pid/maps.Just an opinion, do not mind adding <end> to the name.
So Pavel, we can make the names to look as say "3d73a00000-3d73a01000" leaving proc-inode structure unchanged, that's an idea, right?
Cyrill
Quoted 7 lines · Cyrill Gorcunov
On Thu, Aug 25, 2011 at 09:05:22PM +0400, Pavel Emelyanov wrote:
...Quoted 5 lines
Quoted 4 lines
I would prefer if the filename included both start and end addresses so that it matches the first column of
/proc/PID/mapsand ls'ing the directory is more useful. What do other people think?
Quoted 8 lines
Quoted 5 lines
Without 0x at the beginning this is not very convenient, but once we add them it no longer matches the contents of the
/proc/pid/maps.Just an opinion, do not mind adding <end> to the name.
So Pavel, we can make the names to look as say "3d73a00000-3d73a01000" leaving proc-inode structure unchanged, that's an idea, right?
Not exactly - as I said if we leave the name without 0x (3d000000 instead of 0x3d000000) this makes it hard to use strtol() on these strings to convert names to addresses.
Not big deal, just my 5 cents on the "leaving links' names as is" side.
Quoted 3 lines
Cyrill
.
Hello,
Quoted 2 lines · Pavel Emelyanov
Not exactly - as I said if we leave the name without 0x (
3d000000instead of0x3d000000) this makes it hard to usestrtol()on these strings to convert names to addresses.
But strtol() has @base.
Thanks.
Signature · 1 line
tejun
Quoted 8 lines · Tejun Heo
Hello,
Quoted 2 lines · Pavel Emelyanov
Not exactly - as I said if we leave the name without 0x (
3d000000instead of0x3d000000) this makes it hard to usestrtol()on these strings to convert names to addresses.But
strtol()has @base.
That's ok. Tejun, i'll think a bit more (probably tomorrow already, since I'll be reworking it and adding a check for permissions nit you've pointed) and will check if there is a nice way to make the names longer ;)
Cyrill
Quoted 9 lines · Tejun Heo
Hello,
Quoted 5 lines · Cyrill Gorcunov
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73a00000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1b000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73c1c000 -> /lib64/ld-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73e00000 -> /lib64/libc-2.5.so | lr-x------ 1 cyrill cyrill 64 Aug 9 15:25 0x3d73f4e000 -> /lib64/libc-2.5.so
Quoted 4 lines
I would prefer if the filename included both start and end addresses so that it matches the first column of
/proc/PID/mapsand ls'ing the directory is more useful. What do other people think?
This will force us to bloat proc_inode a bit more, since at moment we have a union such as
struct proc_inode {
struct pid *pid;
union {
int fd;
unsigned long vm_start;
};
...also it will require to make a buffer for names twice bigger as well. If all this not a problem -- then sure we can do that ;)
Cyrill
Hello,
Quoted 11 lines · Cyrill Gorcunov
This will force us to bloat
proc_inodea bit more, since at moment we have a union such asstruct proc_inode { struct pid *pid; union { int fd; unsigned long vm_start; }; ...
Quoted 2 lines
also it will require to make a buffer for names twice bigger as well. If all this not a problem -- then sure we can do that ;)
I'm not too familiar with this area but I don't think bloating it a bit would be such a big problem. proc_inode is generated on the fly as proc directories and files are accessed, so it's not like they'll linearly hog down more memory with increasing number of entries (sysfs had this sort of problem in the past).
Another thing is, I don't really see why we need vm_start, or fd for that matter, in proc_inode at all. proc_inode is created on the fly only as dentry gets instantiated on demand, which means we always have d_name on hand to tell what the file is supposed to point to. In fact, the code already uses name_to_int() to extract fd from d_name. Hmmm... well yeah, it actually seems that proc_inode->fd is never used and we can simply remove it.
So, we can do the same thing w/ these mapping files too. We just need name_to_map_range() or something. Also, for most address ranges, the name sould fit inside the inline name in dentry (32 bytes on 64bit), so it really isn't gonna bloat anything.
Thanks.
Signature · 1 line
tejun
Hello, again.
Quoted 7 lines · Tejun Heo
Another thing is, I don't really see why we need
vm_start, or fd for that matter, inproc_inodeat all.proc_inodeis created on the fly only as dentry gets instantiated on demand, which means we always haved_nameon hand to tell what the file is supposed to point to. In fact, the code already usesname_to_int()to extract fd fromd_name. Hmmm... well yeah, it actually seems thatproc_inode->fdis never used and we can simply remove it.
Unfortunately, not quite as easy as I expected. The information still seems redundant but it seems we'll need to change proc_inode->get_link() to take dentry instead of inode before doing away with proc_inode->fd, but, at any rate, I don't think this is a big deal one way or the other.
Thanks.
Signature · 1 line
tejun
Quoted 11 lines · Tejun Heo
Hello, again.
Quoted 7 lines · Tejun Heo
Another thing is, I don't really see why we need
vm_start, or fd for that matter, inproc_inodeat all.proc_inodeis created on the fly only as dentry gets instantiated on demand, which means we always haved_nameon hand to tell what the file is supposed to point to. In fact, the code already usesname_to_int()to extract fd fromd_name. Hmmm... well yeah, it actually seems thatproc_inode->fdis never used and we can simply remove it.
Quoted 6 lines
Unfortunately, not quite as easy as I expected. The information still seems redundant but it seems we'll need to change
proc_inode->get_link()to take dentry instead of inode before doing away withproc_inode->fd, but, at any rate, I don't think this is a big deal one way or the other.
Hohum... picking up an additional reference to dentry might be dangerous I think. How exactly you imagine we would do that? (without this problem I guess we indeed may drop or rather not change proc-inode).
Cyrill
Hello,
Quoted 9 lines · Cyrill Gorcunov
Quoted 5 lines
Unfortunately, not quite as easy as I expected. The information still seems redundant but it seems we'll need to change
proc_inode->get_link()to take dentry instead of inode before doing away withproc_inode->fd, but, at any rate, I don't think this is a big deal one way or the other.Hohum... picking up an additional reference to dentry might be dangerous I think. How exactly you imagine we would do that? (without this problem I guess we indeed may drop or rather not change proc-inode).
Why would you need an extra reference? All these data structures are created dynamically on access and dentry is always available while any operation on the inode is in progress so it's guaranteed to be available and there's no reason to diddle with reference count. Anyways, we can deal with this optimization later, I think.
Thanks.
Signature · 1 line
tejun
Quoted 13 lines · Tejun Heo
Hello,
Quoted 9 lines · Cyrill Gorcunov
Quoted 5 lines
Unfortunately, not quite as easy as I expected. The information still seems redundant but it seems we'll need to change
proc_inode->get_link()to take dentry instead of inode before doing away withproc_inode->fd, but, at any rate, I don't think this is a big deal one way or the other.Hohum... picking up an additional reference to dentry might be dangerous I think. How exactly you imagine we would do that? (without this problem I guess we indeed may drop or rather not change proc-inode).
Quoted 6 lines
Why would you need an extra reference? All these data structures are created dynamically on access and dentry is always available while any operation on the inode is in progress so it's guaranteed to be available and there's no reason to diddle with reference count. Anyways, we can deal with this optimization later, I think.
Hi Tejun, yeah, I somehow missed that you propose to lift up proc_get_link a bit. Letme try such approach indeed. Thanks!
Cyrill
On Thu, Aug 25, 2011 at 11:39:31PM +0200, Tejun Heo wrote:
...
Quoted 7 lines
Why would you need an extra reference? All these data structures are created dynamically on access and dentry is always available while any operation on the inode is in progress so it's guaranteed to be available and there's no reason to diddle with reference count. Anyways, we can deal with this optimization later, I think.
Hi, thanks a huge for all feedback! Mind to give the below one a review shot? Hope this time I've addressed all concerns. Thanks. (please check map_files_d_revalidate() precisely). Complains are welcome, as always.
Cyrill
---
fs, proc: Introduce the /proc/<pid>/map_files/ directory v4
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so
This helps checkpointing process in three ways:
1. When dumping a task mappings we do know exact file that is mapped by particular region. We do this by opening /proc/pid/map_files/address symlink the way we do with file descriptors.
2. This also helps in determining which anonymous shared mappings are shared with each other by comparing the inodes of them.
3. When restoring a set of process in case two of them has a mapping shared, we map the memory by the 1st one and then open its /proc/pid/map_files/address file and map it by the 2nd task.
v2: (spotted by Tejun Heo)
- /proc/<pid>/mfd changed to /proc/<pid>/map_files
- find_vma helper is used instead of linear search
- routines are re-grouped
- d_revalidate is set now
v3:
- d_revalidate reworked, now it should drops no longer valid dentries (Tejun Heo)
- ptrace_may_access added into proc_map_files_lookup (Vasiliy Kulikov)
- because of filldir (which eventually might need to lock mmap_sem) the proc_map_files_readdir() was reworked to call proc_fill_cache() with unlocked mmap_sem
v4: (feedback by Tejun Heo and Vasiliy Kulikov)
- instead of saving data in proc_inode we rather make a dentry name to keep both vm_start and vm_end accordingly
- d_revalidate now honor task credentials
--- fs/proc/base.c | 326 +++++++++++++++++++++++++++++++++++++++++++++++- include/linux/proc_fs.h | 2 2 files changed, 321 insertions(+), 7 deletions(-)
fs/proc/base.c +320 -6
@@ -165,7 +165,7 @@ static int get_task_root(struct task_strreturn result; } -static int proc_cwd_link (struct inode *inode,struct path *path) +static int proc_cwd_link (struct dentry *dentry,struct inode *inode,struct path *path) {struct task_struct *task =get_proc_task (inode);int result = -ENOENT ;
@@ -182,7 +182,7 @@ static int proc_cwd_link(struct inode *ireturn result; } -static int proc_root_link (struct inode *inode,struct path *path) +static int proc_root_link (struct dentry *dentry,struct inode *inode,struct path *path) {struct task_struct *task =get_proc_task (inode);int result = -ENOENT ;
@@ -1580,7 +1580,7 @@ static const struct file_operations proc .release = single_release, }; -static int proc_exe_link (struct inode *inode,struct path *exe_path) +static int proc_exe_link (struct dentry *dentry,struct inode *inode,struct path *exe_path) {struct task_struct *task;struct mm_struct *mm;
@@ -1616,7 +1616,7 @@ static void *proc_pid_follow_link(structif (!proc_fd_access_allowed (inode)) goto out; - error =PROC_I (inode)->op.proc_get_link (inode, &nd->path); + error =PROC_I (inode)->op.proc_get_link (dentry, inode, &nd->path); out:return ERR_PTR (error); }
@@ -1655,7 +1655,7 @@ static int proc_pid_readlink(struct dentif (!proc_fd_access_allowed (inode)) goto out; - error =PROC_I (inode)->op.proc_get_link (inode, &path); + error =PROC_I (inode)->op.proc_get_link (dentry, inode, &path);if (error) goto out;
@@ -1947,7 +1947,7 @@ static int proc_fd_info(struct inode *inreturn -ENOENT ; } -static int proc_fd_link (struct inode *inode,struct path *path) +static int proc_fd_link (struct dentry *dentry,struct inode *inode,struct path *path) {return proc_fd_info (inode, path,NULL ); }
@@ -2170,6 +2170,319 @@ static const struct file_operations proc .llseek = default_llseek, }; +static struct vm_area_struct * +find_exact_vma (struct mm_struct *mm,unsigned long vm_start,unsigned long vm_end) +{ +struct vm_area_struct *vma =find_vma (mm, vm_start); +if (vma && (vma->vm_start != vm_start || vma->vm_end != vm_end)) + vma =NULL ; +return vma; +} + +static int map_name_to_addr (const unsigned char *name,unsigned long *start,unsigned long *end) +{ +int ret =-1 ; +char *endp; + +if (unlikely (!name)) + goto err; + + *start =simple_strtoul (name, &endp,16 ); +if (*endp !='-' ) + goto err; + *end =simple_strtoul (endp +1 , &endp,16 ); +if (*endp !=0 ) + goto err; + + ret =0 ; + +err: +return ret; +} + +static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{ +struct vm_area_struct *vma =NULL ; +unsigned long vm_start, vm_end; +struct task_struct *task; +const struct cred *cred; +struct mm_struct *mm; +struct inode *inode; + +if (nd && nd->flags &LOOKUP_RCU ) +return -ECHILD ; + + inode = dentry->d_inode; + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +if (!map_name_to_addr (dentry->d_name.name, &vm_start, &vm_end)) { +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +up_read (&mm->mmap_sem); + } + +mmput (mm); + +if (vma) { +if (task_dumpable (task)) { +rcu_read_lock (); + cred =__task_cred (task); + inode->i_uid = cred->euid; + inode->i_gid = cred->egid; +rcu_read_unlock (); + }else { + inode->i_uid =0 ; + inode->i_gid =0 ; + } +security_task_to_inode (task, inode); +return 1 ; + } +out: +d_drop (dentry); +return 0 ; +} + +static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = map_files_d_revalidate, + .d_delete = pid_delete_dentry, +}; + +static int proc_map_files_get_link (struct dentry *dentry,struct inode *inode,struct path *path) +{ +unsigned long vm_start, vm_end; +struct vm_area_struct *vma; +struct task_struct *task; +struct mm_struct *mm; +int rc = -ENOENT ; + + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_mmput; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +if (vma && vma->vm_file) { + *path = vma->vm_file->f_path; +path_get (path); + rc =0 ; + } +up_read (&mm->mmap_sem); + +out_mmput: +mmput (mm); +out: +return rc; +} + +struct map_files_info { +struct file *file; +unsigned char name[16 +16 +2 ];/* max: %016lx-%016lx\0 */ +unsigned long len; +}; + +static struct dentry * +proc_map_files_instantiate (struct inode *dir,struct dentry *dentry, +struct task_struct *task,const void *ptr) +{ +const struct file *file = ptr; +struct proc_inode *ei; +struct inode *inode; + +if (!file) +return ERR_PTR (-ENOENT ); + + inode =proc_pid_make_inode (dir->i_sb, task); +if (!inode) +return ERR_PTR (-ENOENT ); + + ei =PROC_I (inode); + ei->op.proc_get_link = proc_map_files_get_link; + + inode->i_op = &proc_pid_link_inode_operations; + inode->i_size =64 ; + inode->i_mode =S_IFLNK ; + +if (file->f_mode &FMODE_READ ) + inode->i_mode |=S_IRUSR |S_IXUSR ; +if (file->f_mode &FMODE_WRITE ) + inode->i_mode |=S_IWUSR |S_IXUSR ; + +d_set_d_op (dentry, &tid_map_files_dentry_operations); +d_add (dentry, inode); + +return NULL ; +} + +static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +unsigned long vm_start, vm_end; +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct dentry *result; + + result =ERR_PTR (-ENOENT ); + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm; + + result =ERR_PTR (-ENOENT ); +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +if (!vma) + goto out_no_vma; + + result =proc_map_files_instantiate (dir, dentry, task, vma->vm_file); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +} + +static const struct inode_operations proc_map_files_inode_operations = { + .lookup = proc_map_files_lookup, + .setattr = proc_setattr, +}; + +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; +unsigned int vmai; +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, i; +struct map_files_info *info; + + mm =get_task_mm (task); +if (!mm) + goto out; +down_read (&mm->mmap_sem); + + nr_files =0 ; +for (vma = mm->mmap; vma; vma = vma->vm_next) { +if (vma->vm_file) + nr_files++; + } +if (!nr_files) + goto out; + + info =kmalloc (nr_files *sizeof (*info),GFP_KERNEL ); +if (!info) { + ret = -ENOMEM ; + goto out; + } + + used =0 ; +for (vma = mm->mmap, vmai =2 ; vma; vma = vma->vm_next) { +if (!vma->vm_file) +continue ; + vmai++; +if (vmai <= filp->f_pos) +continue ; + +get_file (vma->vm_file); + info[used].file = vma->vm_file; + info[used].len =snprintf (info[used].name, +sizeof (info[used].name), +"%lx-%lx" , vma->vm_start, + vma->vm_end); + used++; + } + +up_read (&mm->mmap_sem); + +for (i =0 ; i < used; i++) { + ret =proc_fill_cache (filp, dirent, filldir, + info[i].name, + info[i].len, + proc_map_files_instantiate, + task, info[i].file); +if (ret) +break ; + filp->f_pos++; + } + +for (i =0 ; i < used; i++) +put_filp (info[i].file); + +kfree (info); +mmput (mm); + } + } + +out: +put_task_struct (task); +out_no_task: +return ret; +} + +static const struct file_operations proc_map_files_operations = { + .read = generic_read_dir, + .readdir = proc_map_files_readdir, + .llseek = default_llseek, +}; +/* * /proc/pid/fd needs a special permission handler so that a process can still * access /proc/self/fd after it has executed a setuid().
@@ -2785,6 +3098,7 @@ static const struct inode_operations prostatic const struct pid_entry tgid_base_stuff[] = {DIR ("task" ,S_IRUGO |S_IXUGO , proc_task_inode_operations, proc_task_operations),DIR ("fd" ,S_IRUSR |S_IXUSR , proc_fd_inode_operations, proc_fd_operations), +DIR ("map_files" ,S_IRUSR |S_IXUSR , proc_map_files_inode_operations, proc_map_files_operations),DIR ("fdinfo" ,S_IRUSR |S_IXUSR , proc_fdinfo_inode_operations, proc_fdinfo_operations),DIR ("ns" ,S_IRUSR |S_IXUGO , proc_ns_dir_inode_operations, proc_ns_dir_operations), #ifdef CONFIG_NET
include/linux/proc_fs.h +1 -1
@@ -253,7 +253,7 @@ extern const struct proc_ns_operations uextern const struct proc_ns_operations ipcns_operations;union proc_op { -int (*proc_get_link)(struct inode *,struct path *); +int (*proc_get_link)(struct dentry *,struct inode *,struct path *);int (*proc_read)(struct task_struct *task,char *page); int (*proc_show)(struct seq_file *m, struct pid_namespace *ns, struct pid *pid,
Quoted 77 lines · Cyrill Gorcunov
On Thu, Aug 25, 2011 at 11:39:31PM +0200, Tejun Heo wrote:
...Quoted 7 lines
Why would you need an extra reference? All these data structures are created dynamically on access and dentry is always available while any operation on the inode is in progress so it's guaranteed to be available and there's no reason to diddle with reference count. Anyways, we can deal with this optimization later, I think.
Hi, thanks a huge for all feedback! Mind to give the below one a review shot? Hope this time I've addressed all concerns. Thanks. (please check
map_files_d_revalidate()precisely). Complains are welcome, as always.Cyrill
---
fs, proc: Introduce the /proc/<pid>/map_files/ directory v4This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.soThis helps checkpointing process in three ways:
1. When dumping a task mappings we do know exact file that is mapped by particular region. We do this by opening
/proc/pid/map_files/addresssymlink the way we do with file descriptors.2. This also helps in determining which anonymous shared mappings are shared with each other by comparing the inodes of them.
3. When restoring a set of process in case two of them has a mapping shared, we map the memory by the 1st one and then open its
/proc/pid/map_files/addressfile and map it by the 2nd task.v2: (spotted by Tejun Heo)
- /proc/<pid>/mfd changed to /proc/<pid>/map_files
-find_vmahelper is used instead of linear search
- routines are re-grouped
-d_revalidateis set nowv3:
-d_revalidatereworked, now it should drops no longer valid dentries (Tejun Heo)
-ptrace_may_accessadded intoproc_map_files_lookup(Vasiliy Kulikov)
- because of filldir (which eventually might need to lockmmap_sem) theproc_map_files_readdir()was reworked to callproc_fill_cache()with unlockedmmap_semv4: (feedback by Tejun Heo and Vasiliy Kulikov)
- instead of saving data inproc_inodewe rather make a dentry name to keep bothvm_startandvm_endaccordingly
-d_revalidatenow honor task credentialsSigned-off-by Pavel Emelyanov <[email protected]>Signed-off-by Cyrill Gorcunov <[email protected]>CC Tejun Heo <[email protected]>CC Vasiliy Kulikov <segoon-cxoSlKxDwOJWk0Htik3J/[email protected]>--- fs/proc/base.c | 326 +++++++++++++++++++++++++++++++++++++++++++++++- include/linux/proc_fs.h | 2 2 files changed, 321 insertions(+), 7 deletions(-)Index:
linux-2.6.git/fs/proc/base.c
===================================================================
fs/proc/base.c static int get_task_root(struct task_str · line 168
Quoted 5 lines
fs/proc/base.c +1
-1@@ -165,7 +165,7 @@ static int get_task_root(struct task_strreturn result; } -static int proc_cwd_link (struct inode *inode,struct path *path) +static int proc_cwd_link (struct dentry *dentry,struct inode *inode,struct path *path)
Put proc_get_link() change in separate patch?
Quoted 298 lines
fs/proc/base.c +251
-5{struct task_struct *task =get_proc_task (inode);int result = -ENOENT ;@@ -182,7 +182,7 @@ static int proc_cwd_link(struct inode *ireturn result; } -static int proc_root_link (struct inode *inode,struct path *path) +static int proc_root_link (struct dentry *dentry,struct inode *inode,struct path *path) {struct task_struct *task =get_proc_task (inode);int result = -ENOENT ;@@ -1580,7 +1580,7 @@ static const struct file_operations proc .release = single_release, }; -static int proc_exe_link (struct inode *inode,struct path *exe_path) +static int proc_exe_link (struct dentry *dentry,struct inode *inode,struct path *exe_path) {struct task_struct *task;struct mm_struct *mm;@@ -1616,7 +1616,7 @@ static void *proc_pid_follow_link(structif (!proc_fd_access_allowed (inode)) goto out; - error =PROC_I (inode)->op.proc_get_link (inode, &nd->path); + error =PROC_I (inode)->op.proc_get_link (dentry, inode, &nd->path); out:return ERR_PTR (error); }@@ -1655,7 +1655,7 @@ static int proc_pid_readlink(struct dentif (!proc_fd_access_allowed (inode)) goto out; - error =PROC_I (inode)->op.proc_get_link (inode, &path); + error =PROC_I (inode)->op.proc_get_link (dentry, inode, &path);if (error) goto out;@@ -1947,7 +1947,7 @@ static int proc_fd_info(struct inode *inreturn -ENOENT ; } -static int proc_fd_link (struct inode *inode,struct path *path) +static int proc_fd_link (struct dentry *dentry,struct inode *inode,struct path *path) {return proc_fd_info (inode, path,NULL ); }@@ -2170,6 +2170,319 @@ static const struct file_operations proc .llseek = default_llseek, }; +static struct vm_area_struct * +find_exact_vma (struct mm_struct *mm,unsigned long vm_start,unsigned long vm_end) +{ +struct vm_area_struct *vma =find_vma (mm, vm_start); +if (vma && (vma->vm_start != vm_start || vma->vm_end != vm_end)) + vma =NULL ; +return vma; +} + +static int map_name_to_addr (const unsigned char *name,unsigned long *start,unsigned long *end) +{ +int ret =-1 ; +char *endp; + +if (unlikely (!name)) + goto err; + + *start =simple_strtoul (name, &endp,16 ); +if (*endp !='-' ) + goto err; + *end =simple_strtoul (endp +1 , &endp,16 ); +if (*endp !=0 ) + goto err; + + ret =0 ; + +err: +return ret; +} + +static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{ +struct vm_area_struct *vma =NULL ; +unsigned long vm_start, vm_end; +struct task_struct *task; +const struct cred *cred; +struct mm_struct *mm; +struct inode *inode; + +if (nd && nd->flags &LOOKUP_RCU ) +return -ECHILD ; + + inode = dentry->d_inode; + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +if (!map_name_to_addr (dentry->d_name.name, &vm_start, &vm_end)) { +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +up_read (&mm->mmap_sem); + } + +mmput (mm); + +if (vma) { +if (task_dumpable (task)) { +rcu_read_lock (); + cred =__task_cred (task); + inode->i_uid = cred->euid; + inode->i_gid = cred->egid; +rcu_read_unlock (); + }else { + inode->i_uid =0 ; + inode->i_gid =0 ; + } +security_task_to_inode (task, inode); +return 1 ; + } +out: +d_drop (dentry); +return 0 ; +} + +static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = map_files_d_revalidate, + .d_delete = pid_delete_dentry, +}; + +static int proc_map_files_get_link (struct dentry *dentry,struct inode *inode,struct path *path) +{ +unsigned long vm_start, vm_end; +struct vm_area_struct *vma; +struct task_struct *task; +struct mm_struct *mm; +int rc = -ENOENT ; + + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_mmput; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +if (vma && vma->vm_file) { + *path = vma->vm_file->f_path; +path_get (path); + rc =0 ; + } +up_read (&mm->mmap_sem); + +out_mmput: +mmput (mm); +out: +return rc; +} + +struct map_files_info { +struct file *file; +unsigned char name[16 +16 +2 ];/* max: %016lx-%016lx\0 */ +unsigned long len; +}; + +static struct dentry * +proc_map_files_instantiate (struct inode *dir,struct dentry *dentry, +struct task_struct *task,const void *ptr) +{ +const struct file *file = ptr; +struct proc_inode *ei; +struct inode *inode; + +if (!file) +return ERR_PTR (-ENOENT ); + + inode =proc_pid_make_inode (dir->i_sb, task); +if (!inode) +return ERR_PTR (-ENOENT ); + + ei =PROC_I (inode); + ei->op.proc_get_link = proc_map_files_get_link; + + inode->i_op = &proc_pid_link_inode_operations; + inode->i_size =64 ; + inode->i_mode =S_IFLNK ; + +if (file->f_mode &FMODE_READ ) + inode->i_mode |=S_IRUSR |S_IXUSR ; +if (file->f_mode &FMODE_WRITE ) + inode->i_mode |=S_IWUSR |S_IXUSR ; + +d_set_d_op (dentry, &tid_map_files_dentry_operations); +d_add (dentry, inode); + +return NULL ; +} + +static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +unsigned long vm_start, vm_end; +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct dentry *result; + + result =ERR_PTR (-ENOENT ); + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm; + + result =ERR_PTR (-ENOENT ); +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +if (!vma) + goto out_no_vma; + + result =proc_map_files_instantiate (dir, dentry, task, vma->vm_file); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +} + +static const struct inode_operations proc_map_files_inode_operations = { + .lookup = proc_map_files_lookup, + .setattr = proc_setattr, +}; + +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; +unsigned int vmai; +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, i; +struct map_files_info *info; + + mm =get_task_mm (task); +if (!mm)
fs/proc/base.c static const struct file_operations proc · line 2428
Quoted 10 lines
fs/proc/base.c +10
-0+ goto out; +down_read (&mm->mmap_sem); + + nr_files =0 ; +for (vma = mm->mmap; vma; vma = vma->vm_next) { +if (vma->vm_file) + nr_files++; + } +if (!nr_files) + goto out;
up_read(&mm->mmap_sem); ?
fs/proc/base.c static const struct file_operations proc · line 2433
Quoted 5 lines
fs/proc/base.c +5
-0+ + info =kmalloc (nr_files *sizeof (*info),GFP_KERNEL ); +if (!info) { + ret = -ENOMEM ; + goto out;
Ditto.
Quoted 80 lines
fs/proc/base.c +53
-0+ } + + used =0 ; +for (vma = mm->mmap, vmai =2 ; vma; vma = vma->vm_next) { +if (!vma->vm_file) +continue ; + vmai++; +if (vmai <= filp->f_pos) +continue ; + +get_file (vma->vm_file); + info[used].file = vma->vm_file; + info[used].len =snprintf (info[used].name, +sizeof (info[used].name), +"%lx-%lx" , vma->vm_start, + vma->vm_end); + used++; + } + +up_read (&mm->mmap_sem); + +for (i =0 ; i < used; i++) { + ret =proc_fill_cache (filp, dirent, filldir, + info[i].name, + info[i].len, + proc_map_files_instantiate, + task, info[i].file); +if (ret) +break ; + filp->f_pos++; + } + +for (i =0 ; i < used; i++) +put_filp (info[i].file); + +kfree (info); +mmput (mm); + } + } + +out: +put_task_struct (task); +out_no_task: +return ret; +} + +static const struct file_operations proc_map_files_operations = { + .read = generic_read_dir, + .readdir = proc_map_files_readdir, + .llseek = default_llseek, +}; +/* * /proc/pid/fd needs a special permission handler so that a process can still * access /proc/self/fd after it has executed a setuid(). @@ -2785,6 +3098,7 @@ static const struct inode_operations prostatic const struct pid_entry tgid_base_stuff[] = {DIR ("task" ,S_IRUGO |S_IXUGO , proc_task_inode_operations, proc_task_operations),DIR ("fd" ,S_IRUSR |S_IXUSR , proc_fd_inode_operations, proc_fd_operations), +DIR ("map_files" ,S_IRUSR |S_IXUSR , proc_map_files_inode_operations, proc_map_files_operations),DIR ("fdinfo" ,S_IRUSR |S_IXUSR , proc_fdinfo_inode_operations, proc_fdinfo_operations),DIR ("ns" ,S_IRUSR |S_IXUGO , proc_ns_dir_inode_operations, proc_ns_dir_operations), #ifdef CONFIG_NETIndex:
linux-2.6.git/include/linux/proc_fs.h
===================================================================include/linux/proc_fs.h +1
-1@@ -253,7 +253,7 @@ extern const struct proc_ns_operations uextern const struct proc_ns_operations ipcns_operations;union proc_op { -int (*proc_get_link)(struct inode *,struct path *); +int (*proc_get_link)(struct dentry *,struct inode *,struct path *);int (*proc_read)(struct task_struct *task,char *page); int (*proc_show)(struct seq_file *m, struct pid_namespace *ns, struct pid *pid,_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers
Signature · 1 line
Kirill A. Shutemov
On Fri, Aug 26, 2011 at 03:28:51PM +0300, Kirill A. Shutemov wrote:
...
Quoted 6 lines
fs/proc/base.c static int get_task_root(struct task_str · line 168
Quoted 3 lines
fs/proc/base.c +1
-1-static int proc_cwd_link (struct inode *inode,struct path *path) +static int proc_cwd_link (struct dentry *dentry,struct inode *inode,struct path *path)Put
proc_get_link()change in separate patch?
Will do, but let leave it to a final pass, ie when all get agreed on idea and details, ok? Since I'll be re-sending the final version seraparely anyway.
...
Quoted 12 lines
fs/proc/base.c static const struct file_operations proc · line 2428
Quoted 9 lines
fs/proc/base.c +9
-0+down_read (&mm->mmap_sem); + + nr_files =0 ; +for (vma = mm->mmap; vma; vma = vma->vm_next) { +if (vma->vm_file) + nr_files++; + } +if (!nr_files) + goto out;
up_read(&mm->mmap_sem); ?
Quoted 8 lines
fs/proc/base.c static const struct file_operations proc · line 2433
Quoted 5 lines
fs/proc/base.c +5
-0+ + info =kmalloc (nr_files *sizeof (*info),GFP_KERNEL ); +if (!info) { + ret = -ENOMEM ; + goto out;Ditto.
Yeah, thanks Kirill!
On Fri, Aug 26, 2011 at 03:28:51PM +0300, Kirill A. Shutemov wrote:
...
Quoted 16 lines
Review
Quoted 13 lines
(quoted diff) +13
-0+ + mm = get_task_mm(task); + if (!mm) + goto out; + down_read(&mm->mmap_sem); + + nr_files = 0; + for (vma = mm->mmap; vma; vma = vma->vm_next) { + if (vma->vm_file) + nr_files++; + } + if (!nr_files) + goto out;
up_read(&mm->mmap_sem); ?
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + info = kmalloc(nr_files * sizeof(*info), GFP_KERNEL); + if (!info) { + ret = -ENOMEM; + goto out;Ditto.
So the final (I hope) version is the below. Please check it. If all get agreed on it I'll split it up and resend later. Sorry for such massive change in one patch.
Cyrill
---
fs, proc: Introduce the /proc/<pid>/map_files/ directory v5
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is "vma->vm_start-vma->vm_end", the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 root root 64 Aug 26 06:40 7f8f80403000-7f8f80404000 -> /lib64/libc-2.5.so | lr-x------ 1 root root 64 Aug 26 06:40 7f8f8061e000-7f8f80620000 -> /lib64/libselinux.so.1 | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80826000-7f8f80827000 -> /lib64/libacl.so.1.1.0 | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a2f000-7f8f80a30000 -> /lib64/librt-2.5.so | lr-x------ 1 root root 64 Aug 26 06:40 7f8f80a30000-7f8f80a4c000 -> /lib64/ld-2.5.so
This helps checkpointing process in three ways:
1. When dumping a task mappings we do know exact file that is mapped by particular region. We do this by opening /proc/pid/map_files/address symlink the way we do with file descriptors.
2. This also helps in determining which anonymous shared mappings are shared with each other by comparing the inodes of them.
3. When restoring a set of process in case two of them has a mapping shared, we map the memory by the 1st one and then open its /proc/pid/map_files/address file and map it by the 2nd task.
v2: (spotted by Tejun Heo)
- /proc/<pid>/mfd changed to /proc/<pid>/map_files
- find_vma helper is used instead of linear search
- routines are re-grouped
- d_revalidate is set now
v3:
- d_revalidate reworked, now it should drops no longer valid dentries (Tejun Heo)
- ptrace_may_access added into proc_map_files_lookup (Vasiliy Kulikov)
- because of filldir (which eventually might need to lock mmap_sem) the proc_map_files_readdir() was reworked to call proc_fill_cache() with unlocked mmap_sem
v4: (feedback by Tejun Heo and Vasiliy Kulikov)
- instead of saving data in proc_inode we rather make a dentry name to keep both vm_start and vm_end accordingly
- d_revalidate now honor task credentials
v5: (feedback by Kirill A. Shutemov)
- don't forget to release mmap_sem on error path
--- fs/proc/base.c | 323 +++++++++++++++++++++++++++++++++++++++++++++++- include/linux/proc_fs.h | 2 2 files changed, 318 insertions(+), 7 deletions(-)
fs/proc/base.c +317 -6
@@ -165,7 +165,7 @@ static int get_task_root(struct task_strreturn result; } -static int proc_cwd_link (struct inode *inode,struct path *path) +static int proc_cwd_link (struct dentry *dentry,struct inode *inode,struct path *path) {struct task_struct *task =get_proc_task (inode);int result = -ENOENT ;
@@ -182,7 +182,7 @@ static int proc_cwd_link(struct inode *ireturn result; } -static int proc_root_link (struct inode *inode,struct path *path) +static int proc_root_link (struct dentry *dentry,struct inode *inode,struct path *path) {struct task_struct *task =get_proc_task (inode);int result = -ENOENT ;
@@ -1580,7 +1580,7 @@ static const struct file_operations proc .release = single_release, }; -static int proc_exe_link (struct inode *inode,struct path *exe_path) +static int proc_exe_link (struct dentry *dentry,struct inode *inode,struct path *exe_path) {struct task_struct *task;struct mm_struct *mm;
@@ -1616,7 +1616,7 @@ static void *proc_pid_follow_link(structif (!proc_fd_access_allowed (inode)) goto out; - error =PROC_I (inode)->op.proc_get_link (inode, &nd->path); + error =PROC_I (inode)->op.proc_get_link (dentry, inode, &nd->path); out:return ERR_PTR (error); }
@@ -1655,7 +1655,7 @@ static int proc_pid_readlink(struct dentif (!proc_fd_access_allowed (inode)) goto out; - error =PROC_I (inode)->op.proc_get_link (inode, &path); + error =PROC_I (inode)->op.proc_get_link (dentry, inode, &path);if (error) goto out;
@@ -1947,7 +1947,7 @@ static int proc_fd_info(struct inode *inreturn -ENOENT ; } -static int proc_fd_link (struct inode *inode,struct path *path) +static int proc_fd_link (struct dentry *dentry,struct inode *inode,struct path *path) {return proc_fd_info (inode, path,NULL ); }
@@ -2170,6 +2170,316 @@ static const struct file_operations proc .llseek = default_llseek, }; +static struct vm_area_struct * +find_exact_vma (struct mm_struct *mm,unsigned long vm_start,unsigned long vm_end) +{ +struct vm_area_struct *vma =find_vma (mm, vm_start); +if (vma && (vma->vm_start != vm_start || vma->vm_end != vm_end)) + vma =NULL ; +return vma; +} + +static int map_name_to_addr (const unsigned char *name,unsigned long *start,unsigned long *end) +{ +int ret =-1 ; +char *endp; + +if (unlikely (!name)) + goto err; + + *start =simple_strtoul (name, &endp,16 ); +if (*endp !='-' ) + goto err; + *end =simple_strtoul (endp +1 , &endp,16 ); +if (*endp !=0 ) + goto err; + + ret =0 ; + +err: +return ret; +} + +static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{ +struct vm_area_struct *vma =NULL ; +unsigned long vm_start, vm_end; +struct task_struct *task; +const struct cred *cred; +struct mm_struct *mm; +struct inode *inode; + +if (nd && nd->flags &LOOKUP_RCU ) +return -ECHILD ; + + inode = dentry->d_inode; + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +if (!map_name_to_addr (dentry->d_name.name, &vm_start, &vm_end)) { +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +up_read (&mm->mmap_sem); + } + +mmput (mm); + +if (vma) { +if (task_dumpable (task)) { +rcu_read_lock (); + cred =__task_cred (task); + inode->i_uid = cred->euid; + inode->i_gid = cred->egid; +rcu_read_unlock (); + }else { + inode->i_uid =0 ; + inode->i_gid =0 ; + } +security_task_to_inode (task, inode); +return 1 ; + } +out: +d_drop (dentry); +return 0 ; +} + +static const struct dentry_operations tid_map_files_dentry_operations = { + .d_revalidate = map_files_d_revalidate, + .d_delete = pid_delete_dentry, +}; + +static int proc_map_files_get_link (struct dentry *dentry,struct inode *inode,struct path *path) +{ +unsigned long vm_start, vm_end; +struct vm_area_struct *vma; +struct task_struct *task; +struct mm_struct *mm; +int rc = -ENOENT ; + + task =get_proc_task (inode); +if (!task) + goto out; + + mm =get_task_mm (task); +put_task_struct (task); +if (!mm) + goto out; + +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_mmput; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +if (vma && vma->vm_file) { + *path = vma->vm_file->f_path; +path_get (path); + rc =0 ; + } +up_read (&mm->mmap_sem); + +out_mmput: +mmput (mm); +out: +return rc; +} + +struct map_files_info { +struct file *file; +unsigned char name[16 +16 +2 ];/* max: %016lx-%016lx\0 */ +unsigned long len; +}; + +static struct dentry * +proc_map_files_instantiate (struct inode *dir,struct dentry *dentry, +struct task_struct *task,const void *ptr) +{ +const struct file *file = ptr; +struct proc_inode *ei; +struct inode *inode; + +if (!file) +return ERR_PTR (-ENOENT ); + + inode =proc_pid_make_inode (dir->i_sb, task); +if (!inode) +return ERR_PTR (-ENOENT ); + + ei =PROC_I (inode); + ei->op.proc_get_link = proc_map_files_get_link; + + inode->i_op = &proc_pid_link_inode_operations; + inode->i_size =64 ; + inode->i_mode =S_IFLNK ; + +if (file->f_mode &FMODE_READ ) + inode->i_mode |=S_IRUSR |S_IXUSR ; +if (file->f_mode &FMODE_WRITE ) + inode->i_mode |=S_IWUSR |S_IXUSR ; + +d_set_d_op (dentry, &tid_map_files_dentry_operations); +d_add (dentry, inode); + +return NULL ; +} + +static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +unsigned long vm_start, vm_end; +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct dentry *result; + + result =ERR_PTR (-ENOENT ); + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm; + + result =ERR_PTR (-ENOENT ); +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start, vm_end); +if (!vma) + goto out_no_vma; + + result =proc_map_files_instantiate (dir, dentry, task, vma->vm_file); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +} + +static const struct inode_operations proc_map_files_inode_operations = { + .lookup = proc_map_files_lookup, + .setattr = proc_setattr, +}; + +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; +unsigned int vmai; +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 : + { +struct map_files_info *info =NULL ; +unsigned long nr_files, used, i; + + mm =get_task_mm (task); +if (!mm) + goto out; +down_read (&mm->mmap_sem); + + nr_files =0 ; + used =0 ; + +for (vma = mm->mmap; vma; vma = vma->vm_next) { +if (vma->vm_file) + nr_files++; + } + +if (nr_files) { + info =kmalloc (nr_files *sizeof (*info),GFP_KERNEL ); +if (!info) + ret = -ENOMEM ; +for (vma = mm->mmap, vmai =2 ; vma && info; vma = vma->vm_next) { +if (!vma->vm_file) +continue ; + vmai++; +if (vmai <= filp->f_pos) +continue ; + +get_file (vma->vm_file); + info[used].file = vma->vm_file; + info[used].len =snprintf (info[used].name, +sizeof (info[used].name), +"%lx-%lx" , vma->vm_start, + vma->vm_end); + used++; + } + } + +up_read (&mm->mmap_sem); + +for (i =0 ; i < used; i++) { + ret =proc_fill_cache (filp, dirent, filldir, + info[i].name, info[i].len, + proc_map_files_instantiate, + task, info[i].file); +if (ret) +break ; + filp->f_pos++; + } + +for (i =0 ; i < used; i++) +put_filp (info[i].file); + +kfree (info); +mmput (mm); + } + } + +out: +put_task_struct (task); +out_no_task: +return ret; +} + +static const struct file_operations proc_map_files_operations = { + .read = generic_read_dir, + .readdir = proc_map_files_readdir, + .llseek = default_llseek, +}; +/* * /proc/pid/fd needs a special permission handler so that a process can still * access /proc/self/fd after it has executed a setuid().
@@ -2785,6 +3095,7 @@ static const struct inode_operations prostatic const struct pid_entry tgid_base_stuff[] = {DIR ("task" ,S_IRUGO |S_IXUGO , proc_task_inode_operations, proc_task_operations),DIR ("fd" ,S_IRUSR |S_IXUSR , proc_fd_inode_operations, proc_fd_operations), +DIR ("map_files" ,S_IRUSR |S_IXUSR , proc_map_files_inode_operations, proc_map_files_operations),DIR ("fdinfo" ,S_IRUSR |S_IXUSR , proc_fdinfo_inode_operations, proc_fdinfo_operations),DIR ("ns" ,S_IRUSR |S_IXUGO , proc_ns_dir_inode_operations, proc_ns_dir_operations), #ifdef CONFIG_NET
include/linux/proc_fs.h +1 -1
@@ -253,7 +253,7 @@ extern const struct proc_ns_operations uextern const struct proc_ns_operations ipcns_operations;union proc_op { -int (*proc_get_link)(struct inode *,struct path *); +int (*proc_get_link)(struct dentry *,struct inode *,struct path *);int (*proc_read)(struct task_struct *task,char *page); int (*proc_show)(struct seq_file *m, struct pid_namespace *ns, struct pid *pid,
Hello, Cyrill.
fs/proc/base.c static int get_task_root(struct task_str · line 168
Quoted 10 lines · Cyrill Gorcunov
Index:
linux-2.6.git/fs/proc/base.c
===================================================================fs/proc/base.c +1
-1@@ -165,7 +165,7 @@ static int get_task_root(struct task_strreturn result; } -static int proc_cwd_link (struct inode *inode,struct path *path) +static int proc_cwd_link (struct dentry *dentry,struct inode *inode,struct path *path)
I don't think it's necessary to pass around both @dentry and @inode. @inode can always be dereferenced from @dentry.
fs/proc/base.c static const struct file_operations proc · line 2184
Quoted 3 lines
fs/proc/base.c +3
-0+static int map_name_to_addr (const unsigned char *name,unsigned long *start,unsigned long *end) +{ +int ret =-1 ;
Very minor but functions returning -1 for error bugs me a bit. Maybe -EINVAL is better? Not a big deal either way.
Quoted 2 lines
fs/proc/base.c +2
-0+static int map_files_d_revalidate (struct dentry *dentry,struct nameidata *nd) +{
...
fs/proc/base.c static const struct file_operations proc · line 2245
Quoted 13 lines
fs/proc/base.c +13
-0+if (vma) { +if (task_dumpable (task)) { +rcu_read_lock (); + cred =__task_cred (task); + inode->i_uid = cred->euid; + inode->i_gid = cred->egid; +rcu_read_unlock (); + }else { + inode->i_uid =0 ; + inode->i_gid =0 ; + } +security_task_to_inode (task, inode); +return 1 ;
Another minor point. Maybe it would be nice to factor this into a function and share it w/ the fd one (and maybe sprinkle some comments while at it too :)
fs/proc/base.c static const struct file_operations proc · line 2297
Quoted 5 lines
fs/proc/base.c +5
-0+struct map_files_info { +struct file *file; +unsigned char name[16 +16 +2 ];/* max: %016lx-%016lx\0 */ +unsigned long len; +};
That's slightly above 50 bytes.
Quoted 12 lines
fs/proc/base.c +12
-0+static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +unsigned long vm_start, vm_end; +struct task_struct *task; +struct vm_area_struct *vma; +struct mm_struct *mm; +struct dentry *result; + + result =ERR_PTR (-ENOENT ); + task =get_proc_task (dir); +if (!task)
fs/proc/base.c static const struct file_operations proc · line 2353
Quoted 10 lines
fs/proc/base.c +10
-0+ goto out_no_task; + + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm; + + result =ERR_PTR (-ENOENT ); +if (map_name_to_addr (dentry->d_name.name, + &vm_start, &vm_end)) + goto out_no_mm;
Another nitpick: Maybe factor out the above three steps?
Quoted 2 lines
fs/proc/base.c +2
-0+static int proc_map_files_readdir (struct file *filp,void *dirent,filldir_t filldir) +{
...
fs/proc/base.c static const struct file_operations proc · line 2431
Quoted 2 lines
fs/proc/base.c +2
-0+if (nr_files) { + info =kmalloc (nr_files *sizeof (*info),GFP_KERNEL );
I think we can just put this on stack.
All my comments are pretty cosmetic, so it generally looks good to me. Andrew, what do you think?
Thanks.
Signature · 1 line
tejun
Quoted 7 lines · Tejun Heo
fs/proc/base.c static const struct file_operations proc · line 2297
Quoted 5 lines
fs/proc/base.c +5
-0+struct map_files_info { +struct file *file; +unsigned char name[16 +16 +2 ];/* max: %016lx-%016lx\0 */ +unsigned long len; +};That's slightly above 50 bytes.
We can use 4 * sizeof(unsigned long) + 2 for name to save a bit on 32 bit architectures.
BTW, show_map_vma() uses "%08lx-%08lx" to print ->start, ->end. I think it's good idea to use this here as well to be consistent with /proc/$PID/{s,}maps
Signature · 1 line
Kirill A. Shutemov
Hello,
Quoted 11 lines · Kirill A. Shutemov
Quoted 7 lines · Tejun Heo
fs/proc/base.c static const struct file_operations proc · line 2297
Quoted 5 lines
fs/proc/base.c +5
-0+struct map_files_info { +struct file *file; +unsigned char name[16 +16 +2 ];/* max: %016lx-%016lx\0 */ +unsigned long len; +};That's slightly above 50 bytes.
We can use 4 * sizeof(unsigned long) + 2 for name to save a bit on 32 bit architectures.
Ooh, I wasn't trying to say it was too big. I was trying to say it would be okay to have that on stack. :)
Quoted 3 lines
BTW,
show_map_vma()uses "%08lx-%08lx" to print ->start, ->end. I think it's good idea to use this here as well to be consistent with/proc/$PID/{s,}maps
I'm not too sure about this one. First of all, I think %08lx is more of an accident than intention (just forgot to update it while transitioning to 64bit) and when ls'd the output can be a bit messy without leading 0's, but again my opinion here isn't very strong.
Thanks.
Signature · 1 line
tejun
On Thu, Aug 25, 2011 at 07:01:47PM +0200, Tejun Heo wrote:
...
Quoted 7 lines
Hmm... don't we need the same credential update as
tid_fd_revalidate()? If the task seteuid's, we want the permissions to change accordingly, right?Thanks.
Exactly! Thanks Tejun. I'll be reworking it anyway (acoording to what we decide to do with start-end range in names).
Cyrill
Quoted 9 lines · Cyrill Gorcunov
fs/proc/base.c +9
-0+static struct dentry *proc_map_files_lookup (struct inode *dir, +struct dentry *dentry,struct nameidata *nd) +{ +struct task_struct *task; +unsigned long vm_start; +struct vm_area_struct *vma; +struct map_files_info info; +struct mm_struct *mm; +struct dentry *result;
fs/proc/base.c static const struct file_operations proc · line 2308
Quoted 10 lines
fs/proc/base.c +10
-0+char *endp; + + result =ERR_PTR (-ENOENT ); + task =get_proc_task (dir); +if (!task) + goto out_no_task; + + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm;
Probably
result = ERR_PTR(-ENOENT);
belongs here?
Quoted 27 lines
fs/proc/base.c +27
-0+ vm_start =simple_strtoul (dentry->d_name.name, &endp,16 ); +if (*endp !='\0' ) + goto out_no_mm; + + mm =get_task_mm (task); +if (!mm) + goto out_no_mm; + +down_read (&mm->mmap_sem); + vma =find_exact_vma (mm, vm_start); +if (!vma) + goto out_no_vma; + +memset (&info,0 ,sizeof (info)); + info.file = vma->vm_file; + info.vm_start = vm_start; + + result =proc_map_files_instantiate (dir, dentry, task, &info); + +out_no_vma: +up_read (&mm->mmap_sem); +mmput (mm); +out_no_mm: +put_task_struct (task); +out_no_task: +return result; +}
Signature · 2 lines
Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
On Thu, Aug 25, 2011 at 09:36:42PM +0400, Vasiliy Kulikov wrote:
...
Quoted 11 lines
fs/proc/base.c static const struct file_operations proc · line 2308
Quoted 4 lines
fs/proc/base.c +4
-0+ + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm;Probably
result = ERR_PTR(-ENOENT);belongs here?
Don't think so, it should point that permission failed rather no such entry. Or this might break some tools?
Cyrill
Quoted 9 lines · Cyrill Gorcunov
On Thu, Aug 25, 2011 at 09:36:42PM +0400, Vasiliy Kulikov wrote:
...Quoted 7 lines
fs/proc/base.c static const struct file_operations proc · line 2308
Quoted 4 lines
fs/proc/base.c +4
-0+ + result =ERR_PTR (-EPERM ); +if (!ptrace_may_access (task,PTRACE_MODE_READ )); + goto out_no_mm;Probably
Quoted 7 lines
Quoted 4 lines
result = ERR_PTR(-ENOENT);belongs here?
Don't think so, it should point that permission failed rather no such entry. Or this might break some tools?
I mean the place exactly where I've put it to indicate simple_strtoul(), get_task_mm(), or find_exact_vma() error. EPERM for ptrace_may_access() is OK.
Thanks,
Signature · 2 lines
Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments
On Thu, Aug 25, 2011 at 09:54:38PM +0400, Vasiliy Kulikov wrote:
...
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
Don't think so, it should point that permission failed rather no such entry. Or this might break some tools?
I mean the place exactly where I've put it to indicate
simple_strtoul(),get_task_mm(), orfind_exact_vma()error. EPERM forptrace_may_access()is OK.
Ah, yeah! ENOENT is really would be correct option here, will fix in next iteration, thanks a huge for spotting it!
Cyrill
Quoted 7 lines · Cyrill Gorcunov
From: Pavel Emelyanov <[email protected]>
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
Quoted 5 lines
This thing is aimed to help checkpointing processes.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25
0x3d73a00000->/lib64/ld-2.5.so
[snip]
Just curious: How do these symlinks work when the process reading a /proc file is in a chroot or a different namespace?
For example, a chroot environment might have independent copies of /lib64/ld-2.5.so and a bind mount of /proc. Does the symlink then point to the wrong file?
Quoted 13 lines · Zan Lynx
Quoted 7 lines · Cyrill Gorcunov
From: Pavel Emelyanov <[email protected]>
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.Quoted 5 lines
This thing is aimed to help checkpointing processes.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25
0x3d73a00000->/lib64/ld-2.5.so
Quoted 8 lines
[snip]
Just curious: How do these symlinks work when the process reading a /proc file is in a chroot or a different namespace?
For example, a chroot environment might have independent copies of
/lib64/ld-2.5.soand a bind mount of /proc. Does the symlink then point to the wrong file?
No and this is the trick - when you readlink it - it give you trash, but when you open one - you get exactly the same file as the map points to.
Quoted 3 lines · Pavel Emelyanov
No and this is the trick - when you readlink it - it give you trash, but when you open one - you get exactly the same file as the map points to.
Isn't that a minor security hole?
For example if I pass a file descriptor into a chroot process for reading, and with this interface you can open it for writing too. I could see this causing problems.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only
Quoted 10 lines · Andi Kleen
Quoted 3 lines · Pavel Emelyanov
No and this is the trick - when you readlink it - it give you trash, but when you open one - you get exactly the same file as the map points to.
Isn't that a minor security hole?
For example if I pass a file descriptor into a chroot process for reading, and with this interface you can open it for writing too. I could see this causing problems.
How does it differ from the /proc/pid/fd links?
Quoted 2 lines
-Andi
Quoted 6 lines · Pavel Emelyanov
Quoted 5 lines · Andi Kleen
Quoted 3 lines · Pavel Emelyanov
No and this is the trick - when you readlink it - it give you trash, but when you open one - you get exactly the same file as the map points to.
Quoted 7 lines
Quoted 5 lines
Isn't that a minor security hole?
For example if I pass a file descriptor into a chroot process for reading, and with this interface you can open it for writing too. I could see this causing problems.
How does it differ from the
/proc/pid/fdlinks?
Those cannot be opened I thought.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
On Thu, Aug 25, 2011 at 04:04:35PM +0200, Andi Kleen wrote:
...
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
For example if I pass a file descriptor into a chroot process for reading, and with this interface you can open it for writing too. I could see this causing problems.
How does it differ from the
/proc/pid/fdlinks?Those cannot be opened I thought.
-Andi
We have ptrace_may_access in lookup so I don't think you'll be able to open it from another namespace or/and if you have not enough credentials to.
Cyrill
Quoted 9 lines · Andi Kleen
Quoted 8 lines · Pavel Emelyanov
Quoted 7 lines · Andi Kleen
Quoted 3 lines · Pavel Emelyanov
No and this is the trick - when you readlink it - it give you trash, but when you open one - you get exactly the same file as the map points to.
Isn't that a minor security hole?
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
For example if I pass a file descriptor into a chroot process for reading, and with this interface you can open it for writing too. I could see this causing problems.
How does it differ from the
/proc/pid/fdlinks?Those cannot be opened I thought.
Neither can be these links then - I use the same access checks in my code.
Quoted 2 lines
-Andi
Quoted 13 lines · Zan Lynx
Quoted 7 lines · Cyrill Gorcunov
From: Pavel Emelyanov <[email protected]>
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.Quoted 5 lines
This thing is aimed to help checkpointing processes.
For example the ls -l of some arbitrary /proc/<pid>/map_files/
| lr-x------ 1 cyrill cyrill 64 Aug 9 15:25
0x3d73a00000->/lib64/ld-2.5.so
Quoted 8 lines
[snip]
Just curious: How do these symlinks work when the process reading a /proc file is in a chroot or a different namespace?
For example, a chroot environment might have independent copies of
/lib64/ld-2.5.soand a bind mount of /proc. Does the symlink then point to the wrong file?
To be fair, I personally didn't tried such scenario but it should be the same behaviour as for /proc/$pid/fd since data is taken from a task pointed by pid. I'll check though, good question.
Cyrill
Hi!
Quoted 4 lines
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.
Is it good idea security-wise? It looks like symlink but does not behave like one. (And yes, I know we already have similar problems in /proc..)
ptrace-may-trace is not good enough protection; I can do this on my own thread to get around read-only protection on fd. (File can be protected from me by directory permissions.)
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 10 lines · Pavel Machek
Hi!
Quoted 4 lines
This one behaves similarly to the /proc/<pid>/fd/ one - it contains symlinks one for each mapping with file, the name of a symlink is
vma->vm_start, the target is the file. Opening a symlink results in a file that point exactly to the same inode as them vma's one.Is it good idea security-wise? It looks like symlink but does not behave like one. (And yes, I know we already have similar problems in /proc..)
What exactly doesn't behave like symlink, can you elaborate, please?
Quoted 3 lines
ptrace-may-trace is not good enough protection; I can do this on my own thread to get around read-only protection on fd. (File can be protected from me by directory permissions.)
I think this issue worth separate discussion and if it turns out there is a problem with that we can fix it together with /proc/pid/fd and other stuff.
Thanks,
Pavel