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...
