Re: [PATCH v6 1/2] proc: pass file instead of inode to proc_mem_open

[Adrian Ratiu]

On Monday, June 17, 2024 11:48 EEST, Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: Jeff Xu <jeffxu@xxxxxxxxxx>
> > Signed-off-by: Adrian Ratiu <adrian.ratiu@xxxxxxxxxxxxx>
> > Reviewed-by: Kees Cook <kees@xxxxxxxxxx>
> > ---
> 
> 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().