| Message ID | 20240613133937.2352724-1-adrian.ratiu@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/2] proc: pass file instead of inode to proc_mem_open | expand |
On Thu, Jun 13, 2024 at 04:39:36PM GMT, Adrian Ratiu wrote: > The file struct is required in proc_mem_open() so its > f_mode can be checked when deciding whether to allow or > deny /proc/*/mem open requests via the new read/write > and foll_force restriction mechanism. > > Thus instead of directly passing the inode to the fun, > we pass the file and get the inode inside it. > > Cc: Jann Horn <jannh@google.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Christian Brauner <brauner@kernel.org> > Cc: Jeff Xu <jeffxu@google.com> > Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> > Reviewed-by: Kees Cook <kees@kernel.org> > --- I've tentatively applies this patch to #vfs.procfs. One comment, one question: > No changes in v6 > --- > fs/proc/base.c | 6 +++--- > fs/proc/internal.h | 2 +- > fs/proc/task_mmu.c | 6 +++--- > fs/proc/task_nommu.c | 2 +- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 72a1acd03675..4c607089f66e 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -794,9 +794,9 @@ static const struct file_operations proc_single_file_operations = { > }; > > > -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) > +struct mm_struct *proc_mem_open(struct file *file, unsigned int mode) > { > - struct task_struct *task = get_proc_task(inode); > + struct task_struct *task = get_proc_task(file->f_inode); Comment: This should use file_inode(file) but I've just fixed that when I applied. Question: Is this an equivalent transformation. So is the inode that was passed to proc_mem_open() always the same inode as file_inode(file)? > struct mm_struct *mm = ERR_PTR(-ESRCH); > > if (task) { > @@ -816,7 +816,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) > > static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) > { > - struct mm_struct *mm = proc_mem_open(inode, mode); > + struct mm_struct *mm = proc_mem_open(file, mode); > > if (IS_ERR(mm)) > return PTR_ERR(mm); > diff --git a/fs/proc/internal.h b/fs/proc/internal.h > index a71ac5379584..d38b2eea40d1 100644 > --- a/fs/proc/internal.h > +++ b/fs/proc/internal.h > @@ -295,7 +295,7 @@ struct proc_maps_private { > #endif > } __randomize_layout; > > -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode); > +struct mm_struct *proc_mem_open(struct file *file, unsigned int mode); > > extern const struct file_operations proc_pid_maps_operations; > extern const struct file_operations proc_pid_numa_maps_operations; > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c > index f8d35f993fe5..fe3b2182b0aa 100644 > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -210,7 +210,7 @@ static int proc_maps_open(struct inode *inode, struct file *file, > return -ENOMEM; > > priv->inode = inode; > - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); > + priv->mm = proc_mem_open(file, PTRACE_MODE_READ); > if (IS_ERR(priv->mm)) { > int err = PTR_ERR(priv->mm); > > @@ -1030,7 +1030,7 @@ static int smaps_rollup_open(struct inode *inode, struct file *file) > goto out_free; > > priv->inode = inode; > - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); > + priv->mm = proc_mem_open(file, PTRACE_MODE_READ); > if (IS_ERR(priv->mm)) { > ret = PTR_ERR(priv->mm); > > @@ -1754,7 +1754,7 @@ static int pagemap_open(struct inode *inode, struct file *file) > { > struct mm_struct *mm; > > - mm = proc_mem_open(inode, PTRACE_MODE_READ); > + mm = proc_mem_open(file, PTRACE_MODE_READ); > if (IS_ERR(mm)) > return PTR_ERR(mm); > file->private_data = mm; > diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c > index bce674533000..a8ab182a4ed1 100644 > --- a/fs/proc/task_nommu.c > +++ b/fs/proc/task_nommu.c > @@ -259,7 +259,7 @@ static int maps_open(struct inode *inode, struct file *file, > return -ENOMEM; > > priv->inode = inode; > - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); > + priv->mm = proc_mem_open(file, PTRACE_MODE_READ); > if (IS_ERR(priv->mm)) { > int err = PTR_ERR(priv->mm); > > -- > 2.44.2 >
On Monday, June 17, 2024 11:48 EEST, Christian Brauner <brauner@kernel.org> wrote: > On Thu, Jun 13, 2024 at 04:39:36PM GMT, Adrian Ratiu wrote: > > The file struct is required in proc_mem_open() so its > > f_mode can be checked when deciding whether to allow or > > deny /proc/*/mem open requests via the new read/write > > and foll_force restriction mechanism. > > > > Thus instead of directly passing the inode to the fun, > > we pass the file and get the inode inside it. > > > > Cc: Jann Horn <jannh@google.com> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Christian Brauner <brauner@kernel.org> > > Cc: Jeff Xu <jeffxu@google.com> > > Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> > > Reviewed-by: Kees Cook <kees@kernel.org> > > --- > > I've tentatively applies this patch to #vfs.procfs. > One comment, one question: > > > No changes in v6 > > --- > > fs/proc/base.c | 6 +++--- > > fs/proc/internal.h | 2 +- > > fs/proc/task_mmu.c | 6 +++--- > > fs/proc/task_nommu.c | 2 +- > > 4 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index 72a1acd03675..4c607089f66e 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -794,9 +794,9 @@ static const struct file_operations proc_single_file_operations = { > > }; > > > > > > -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) > > +struct mm_struct *proc_mem_open(struct file *file, unsigned int mode) > > { > > - struct task_struct *task = get_proc_task(inode); > > + struct task_struct *task = get_proc_task(file->f_inode); > > Comment: This should use file_inode(file) but I've just fixed that when I > applied. > > Question: Is this an equivalent transformation. So is the inode that was > passed to proc_mem_open() always the same inode as file_inode(file)? Thank you! Yes, the inode associated with the file struct should be always the same while the file is opened, so the link set during the top-level mem_open() callback should still hold while it itself calls into its sub-functions like proc_mem_open().
diff --git a/fs/proc/base.c b/fs/proc/base.c index 72a1acd03675..4c607089f66e 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -794,9 +794,9 @@ static const struct file_operations proc_single_file_operations = { }; -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) +struct mm_struct *proc_mem_open(struct file *file, unsigned int mode) { - struct task_struct *task = get_proc_task(inode); + struct task_struct *task = get_proc_task(file->f_inode); struct mm_struct *mm = ERR_PTR(-ESRCH); if (task) { @@ -816,7 +816,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode) static int __mem_open(struct inode *inode, struct file *file, unsigned int mode) { - struct mm_struct *mm = proc_mem_open(inode, mode); + struct mm_struct *mm = proc_mem_open(file, mode); if (IS_ERR(mm)) return PTR_ERR(mm); diff --git a/fs/proc/internal.h b/fs/proc/internal.h index a71ac5379584..d38b2eea40d1 100644 --- a/fs/proc/internal.h +++ b/fs/proc/internal.h @@ -295,7 +295,7 @@ struct proc_maps_private { #endif } __randomize_layout; -struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode); +struct mm_struct *proc_mem_open(struct file *file, unsigned int mode); extern const struct file_operations proc_pid_maps_operations; extern const struct file_operations proc_pid_numa_maps_operations; diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index f8d35f993fe5..fe3b2182b0aa 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -210,7 +210,7 @@ static int proc_maps_open(struct inode *inode, struct file *file, return -ENOMEM; priv->inode = inode; - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); + priv->mm = proc_mem_open(file, PTRACE_MODE_READ); if (IS_ERR(priv->mm)) { int err = PTR_ERR(priv->mm); @@ -1030,7 +1030,7 @@ static int smaps_rollup_open(struct inode *inode, struct file *file) goto out_free; priv->inode = inode; - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); + priv->mm = proc_mem_open(file, PTRACE_MODE_READ); if (IS_ERR(priv->mm)) { ret = PTR_ERR(priv->mm); @@ -1754,7 +1754,7 @@ static int pagemap_open(struct inode *inode, struct file *file) { struct mm_struct *mm; - mm = proc_mem_open(inode, PTRACE_MODE_READ); + mm = proc_mem_open(file, PTRACE_MODE_READ); if (IS_ERR(mm)) return PTR_ERR(mm); file->private_data = mm; diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c index bce674533000..a8ab182a4ed1 100644 --- a/fs/proc/task_nommu.c +++ b/fs/proc/task_nommu.c @@ -259,7 +259,7 @@ static int maps_open(struct inode *inode, struct file *file, return -ENOMEM; priv->inode = inode; - priv->mm = proc_mem_open(inode, PTRACE_MODE_READ); + priv->mm = proc_mem_open(file, PTRACE_MODE_READ); if (IS_ERR(priv->mm)) { int err = PTR_ERR(priv->mm);