On Wed, 27 Sept 2023 at 07:10, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> No need to resend I can massage this well enough in-tree.
Hmm. Please do, but here's some more food for thought for at least the
commit message.
Because there's more than the "__fput_sync()" issue at hand, we have
another delayed thing that this patch ends up short-circuiting, which
wasn't obvious from the original description.
I'm talking about the fact that our existing "file_free()" ends up
doing the actual release with
call_rcu(&f->f_rcuhead, file_free_rcu);
and the patch under discussion avoids that part too.
And I actually like that it avoids it, I just think it should be
mentioned explicitly, because it wasn't obvious to me until I actually
looked at the old __fput() path. Particularly since it means that the
f_creds are free'd synchronously now.
I do think that's fine, although I forget what path it was that
required that rcu-delayed cred freeing. Worth mentioning, and maybe
worth thinking about.
However, when I *did* look at it, it strikes me that we could do this
differently.
Something like this (ENTIRELY UNTESTED) patch, which just moves this
logic into fput() itself.
Again: ENTIRELY UNTESTED, and I might easily have screwed up. But it
looks simpler and more straightforward to me. But again: that may be
because I missed something.
Linus
fs/file_table.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/fs/file_table.c b/fs/file_table.c
index ee21b3da9d08..4fb87a0382d9 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -430,11 +430,33 @@ EXPORT_SYMBOL_GPL(flush_delayed_fput);
static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput);
+/*
+ * Called for files that were never fully opened, and
+ * don't need the RCU-delayed freeing: they have never
+ * been accessed in any other context.
+ */
+static void fput_immediate(struct file *f)
+{
+ security_file_free(f);
+ put_cred(f->f_cred);
+ if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
+ percpu_counter_dec(&nr_files);
+ if (unlikely(f->f_mode & FMODE_BACKING)) {
+ path_put(backing_file_real_path(f));
+ kfree(backing_file(f));
+ } else {
+ kmem_cache_free(filp_cachep, f);
+ }
+}
+
void fput(struct file *file)
{
if (atomic_long_dec_and_test(&file->f_count)) {
struct task_struct *task = current;
+ if (unlikely(!(file->f_mode & FMODE_OPENED)))
+ return fput_immediate(file);
+
if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
init_task_work(&file->f_rcuhead, ____fput);
if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
