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