linux-kernel - Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

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: <20250401-fernhalten-lockvogel-ba56b2b108d2@brauner>
Date: Tue, 1 Apr 2025 12:30:57 +0200
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: [PATCH 2/2] fs: cache the string generated by reading
 /proc/filesystems

On Sat, Mar 29, 2025 at 08:28:21PM +0100, Mateusz Guzik wrote:
> It is being read surprisingly often (e.g., by mkdir, ls and even sed!).
> 
> This is lock-protected pointer chasing over a linked list to pay for
> sprintf for every fs (32 on my boxen).
> 
> Instead cache the result.
> 
> While here mark the file as permanent to avoid atomic refs on each op
> (which can also degrade to taking a spinlock).
> 
> open+read+close cycle single-threaded (ops/s):
> before:	450929
> after:	982308 (+117%)
> 
> Here the main bottleneck is memcg.
> 
> open+read+close cycle with 20 processes (ops/s):
> before:	578654
> after:	3163961 (+446%)
> 
> The main bottleneck *before* is spinlock acquire in procfs eliminated by
> marking the file as permanent. The main bottleneck *after* is the
> spurious lockref trip on open.
> 
> The file looks like a sterotypical C from the 90s, right down to an
> open-code and slightly obfuscated linked list. I intentionally did not
> clean up any of it -- I think the file will be best served by a Rust
> rewrite when the time comes.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> ---

> +	pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> +	proc_make_permanent(pde);
>  	return 0;

This all looks good enough for me especially if it's really that much of
a bottleneck for some workloads. But the above part is broken because
proc_create_single() may return NULL afair and that means
proc_make_permanent()->pde_make_permanent() will NULL-deref.

I'll fix that up locally.