Re: [PATCH v2 (kindof)] fs: use __fput_sync in close(2)

[Christian Brauner]

On Tue, Aug 08, 2023 at 06:30:06PM +0200, Christian Brauner wrote:
> On Tue, Aug 08, 2023 at 05:07:22PM +0200, Mateusz Guzik wrote:
> > I slapped the following variant just for illustration purposes.
> > 
> > - adds __close_fd which returns a struct file
> > - adds __filp_close with a flag whether to fput
> > - makes close(2) use both
> > - transparent to everyone else
> > 
> > Downside is that __fput_sync still loses the assert. Instead of
> > losing, it could perhaps be extended with a hack to check syscall
> > number -- pass if either this is close (or binary compat close) or a
> > kthread, BUG out otherwise. Alternatively perhaps deref could be
> > opencoded along with a comment about real fput that this is taking
> > place. Or maybe some other cosmetic choice.
> > 
> > I cannot compile-test right now, so down below is a rough copy make
> > sure it is clear what I mean.
> > 
> > I feel compelled to note that simple patches get microbenchmarked all
> > the time, with these results being the only justification provided.
> > I'm confused why this patch is supposed to be an exception given its
> > simplicity.
> > 
> > Serious justification should be expected from tough calls --
> > complicated, invasive changes, maybe with numerous tradeoffs.
> > 
> > In contrast close(2) doing __fput_sync looks a clear cut thing to do,
> > at worst one can argue which way to do it.
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index 3fd003a8604f..c341b07533b0 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -651,20 +651,30 @@ static struct file *pick_file(struct
> > files_struct *files, unsigned fd)
> >         return file;
> >  }
> > 
> > -int close_fd(unsigned fd)
> > +struct file *__close_fd(unsigned fd, struct file_struct *files)
> >  {
> > -       struct files_struct *files = current->files;
> >         struct file *file;
> > 
> >         spin_lock(&files->file_lock);
> >         file = pick_file(files, fd);
> >         spin_unlock(&files->file_lock);
> > +
> > +       return file;
> > +}
> > +EXPORT_SYMBOL(__close_fd); /* for ksys_close() */
> > +
> > +int close_fd(unsigned fd)
> > +{
> > +       struct files_struct *files = current->files;
> > +       struct file *file;
> > +
> > +       file = __close_fd(fd, files);
> >         if (!file)
> >                 return -EBADF;
> > 
> >         return filp_close(file, files);
> >  }
> > -EXPORT_SYMBOL(close_fd); /* for ksys_close() */
> > +EXPORT_SYMBOL(close_fd);
> > 
> >  /**
> >   * last_fd - return last valid index into fd table
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index fc7d677ff5ad..b7461f0b73f4 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -463,6 +463,11 @@ void __fput_sync(struct file *file)
> >  {
> >         if (atomic_long_dec_and_test(&file->f_count)) {
> >                 struct task_struct *task = current;
> > +               /*
> > +                * I see 2 basic options
> > +                * 1. just remove the assert
> > +                * 2. demand the flag *or* that the caller is close(2)
> > +                */
> >                 BUG_ON(!(task->flags & PF_KTHREAD));
> >                 __fput(file);
> >         }
> > diff --git a/fs/open.c b/fs/open.c
> > index e6ead0f19964..b1602307c1c3 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -1533,7 +1533,16 @@ EXPORT_SYMBOL(filp_close);
> >   */
> >  SYSCALL_DEFINE1(close, unsigned int, fd)
> >  {
> > -       int retval = close_fd(fd);
> > +       struct files_struct *files = current->files;
> > +       struct file *file;
> > +       int retval;
> > +
> > +       file = __close_fd(fd);
> > +       if (!file)
> > +               return -EBADF;
> > +
> > +       retval = __filp_close(file, files, false);
> > +       __fput_sync(file);
> > 
> >         /* can't restart close syscall because file table entry was cleared */
> >         if (unlikely(retval == -ERESTARTSYS ||
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 562f2623c9c9..e64c0238a65f 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2388,7 +2388,11 @@ static inline struct file
> > *file_clone_open(struct file *file)
> >  {
> >         return dentry_open(&file->f_path, file->f_flags, file->f_cred);
> >  }
> > -extern int filp_close(struct file *, fl_owner_t id);
> > +extern int __filp_close(struct file *file, fl_owner_t id, bool dofput);
> > +static inline int filp_close(struct file *file, fl_owner_t id)
> > +{
> > +       return __filp_close(file, id, true);
> > +}
> > 
> >  extern struct filename *getname_flags(const char __user *, int, int *);
> >  extern struct filename *getname_uflags(const char __user *, int);
> 
> At least make this really dumb and obvious and keep the ugliness to
> internal.h and open.c

Sorry, I only sent a portion of the patch...