Messages in this thread | | | Date | Tue, 13 Mar 2012 16:43:37 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file v3 |
| |
On 03/13, Cyrill Gorcunov wrote: > > Matt, Oleg, there is a final version I hope, > which should fit everyone.
Well, this version looks correct, but you are checking the same condition twice, with the different comments. This is confusing.
> +static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd) > +{ > + struct file *exe_file; > + struct dentry *dentry; > + int err; > + > + /* > + * Setting new mm::exe_file is only allowed > + * when no VM_EXECUTABLE vma's left. This is > + * a special C/R case when a restored program > + * need to change own /proc/$pid/exe symlink. > + * After this call mm::num_exe_file_vmas become > + * meaningless. > + */ > + if (mm->num_exe_file_vmas) > + return -EBUSY; > + > + exe_file = fget(fd); > + if (!exe_file) > + return -EBADF; > + > + dentry = exe_file->f_path.dentry; > + > + /* > + * Because the original mm->exe_file > + * points to executable file, make sure > + * this one is executable as well to not > + * break an overall picture. > + */ > + err = -EACCES; > + if (!S_ISREG(dentry->d_inode->i_mode) || > + exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC) > + goto exit; > + > + err = inode_permission(dentry->d_inode, MAY_EXEC); > + if (err) > + goto exit; > + > + /* > + * For security reason changing mm->exe_file > + * is one-shot action. > + */ > + down_write(&mm->mmap_sem); > + if (likely(!mm->exe_file))
This means that the num_exe_file_vmas check at the start is not needed. If you want it as a "fast-path" check, please fix the comment. Or just remove it. Otherwise the code looks as if we have to check them both.
Matt, is it really possible to hit mm->exe_file = NULL in removed_exe_file_vma ? Unless I missed something, this check just hides the potentional problem, no?
IOW, shouldn't it do
void removed_exe_file_vma(struct mm_struct *mm) { WARN_ON(!mm->exe_file); WARN_ON(mm->num_exe_file_vmas <= 0);
if (!--mm->num_exe_file_vmas) { fput(mm->exe_file); mm->exe_file = NULL; } }
?
Oleg.
| |