linux-kernel - Re: [RFC PATCH v v2 0/4] avoid the extra atomic on a ref when closing a fd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250305-sofern-visite-70a6134399cb@brauner>
Date: Wed, 5 Mar 2025 11:38:47 +0100
From: Christian Brauner <brauner@...nel.org>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: viro@...iv.linux.org.uk, jack@...e.cz, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [RFC PATCH v v2 0/4] avoid the extra atomic on a ref when
 closing a fd

On Tue, Mar 04, 2025 at 07:35:02PM +0100, Mateusz Guzik wrote:
> The stock kernel transitioning the file to no refs held penalizes the
> caller with an extra atomic to block any increments.
> 
> For cases where the file is highly likely to be going away this is
> easily avoidable.
> 
> In the open+close case the win is very modest because of the following
> problems:
> - kmem and memcg having terrible performance

I thought that was going to be addressed by Vlastimil, i.e., the mm guys
to provide a new memcg api.

> - putname using an atomic (I have a wip to whack that)
> - open performing an extra ref/unref on the dentry (there are patches to
>   do it, including by Al. I mailed about them in [1])
> - creds using atomics (I have a wip to whack that)
> - apparmor using atomics (ditto, same mechanism)
> 
> On top of that I have a WIP patch to dodge some of the work at lookup
> itself.
> 
> All in all there is several % avoidably lost here.
> 
> stats colected during a kernel build with:
> bpftrace -e 'kprobe:filp_close,kprobe:fput,kprobe:fput_close* { @[probe] = hist(((struct file *)arg0)->f_ref.refcnt.counter > 0); }'
> 
> @[kprobe:filp_close]:
> [0]                32195 |@@@@@@@@@@                                          |
> [1]               164567 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> 
> @[kprobe:fput]:
> [0]               339240 |@@@@@@                                              |
> [1]              2888064 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> 
> @[kprobe:fput_close]:
> [0]              5116767 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [1]               164544 |@                                                   |
> 
> @[kprobe:fput_close_sync]:
> [0]              5340660 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [1]               358943 |@@@                                                 |
> 
> 
> 0 indicates the last reference, 1 that there is more.
> 
> filp_close is largely skewed because of close_on_exec.
> 
> vast majority of last fputs are from remove_vma. I think that code wants
> to be patched to batch them (as in something like fput_many should be
> added -- something for later).

We used to have that for io_uring and got rid of it. The less fput()
primitives the better tbh. But let's see.

> 
> [1] https://lore.kernel.org/linux-fsdevel/20250304165728.491785-1-mjguzik@gmail.com/T/#u
> 
> v2:
> - patch filp_close
> - patch failing open
> 
> Mateusz Guzik (4):
>   file: add fput and file_ref_put routines optimized for use when
>     closing a fd
>   fs: use fput_close_sync() in close()
>   fs: use fput_close() in filp_close()
>   fs: use fput_close() in path_openat()
> 
>  fs/file.c                | 75 ++++++++++++++++++++++++++++++----------
>  fs/file_table.c          | 72 +++++++++++++++++++++++++++-----------
>  fs/namei.c               |  2 +-
>  fs/open.c                |  4 +--
>  include/linux/file.h     |  2 ++
>  include/linux/file_ref.h |  1 +
>  6 files changed, 114 insertions(+), 42 deletions(-)
> 
> -- 
> 2.43.0
>