linux-kernel - Re: [PATCH] eventfs: Stop using dcache

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]

Date: Wed, 3 Jan 2024 22:14:44 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Christian Brauner <brauner@...nel.org>,
	linux-fsdevel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] eventfs: Stop using dcache_readdir() for getdents()

On Wed, Jan 03, 2024 at 01:54:36PM -0800, Linus Torvalds wrote:

> Again: UNTESTED, and meant as a "this is another way to avoid messing
> with the dentry tree manually, and just using the VFS interfaces we
> already have"

That would break chown(), though.  From conversation back in November:

17:50 #kernel: < viro> while we are at it, why not simply supply ->permission() and ->getattr() that would pick gid from superblock 
 and shove them into ->i_gid?
17:50 #kernel: < viro> and called the default variants
17:50 #kernel: < viro> no need to scan the tree, etc.
17:51 #kernel: < viro> how many place in VFS or VM give a fuck about GID of inode?
17:53 #kernel: < viro> stat() and permission checks
17:56 #kernel: < viro> but that boils down to "well, generic getattr and permission use that field and on-disk filesystems use it to keep track of what value to put on disk"
17:56 #kernel: < viro> you can trivially override the defaults for ->permission() and ->getattr()
17:57 #kernel: < viro> and have them set the right ->i_gid whenever called

17:58 #kernel: < viro> what do you want to happen for chown() + remount?
17:58 #kernel: < viro> any group changes from the former lost on the latter?
18:00 #kernel: < viro> if you want the current semantics, slap generation counter in superblock (bumped on remount)
18:00 #kernel: < viro> sample it into inode on ->setattr()
18:01 #kernel: < viro> and have ->permission() and ->getattr() compare inode and superblock gen counts, picking ->i_gid from superblock if it's more recent there
18:02 #kernel: < viro> if you want the result of chown() to stick, have it stuff ~0U into inode's gen counter instead of sampling the superblock's counter there

18:17 #kernel: < viro> OK... so we need to filter SB_I_VERSION out of flags on mount/remount, lest the timestamp updates start playing silly buggers with it
18:18 #kernel: < viro> and use inode_..._iversion_raw() for access
18:19 #kernel: < viro> or use ->i_generation, perhaps...
18:20 #kernel: < viro> 32bit, but if somebody does 4G mount -o remount, they are deliberately asking for trouble

21:37 #kernel: < viro> hmm...
21:37 #kernel: < viro> ->d_revalidate() as well, probably
21:39 #kernel: < viro> rostedt: my apologies, looks like I had been too optimistic

I have the beginnings of patch along those lines stuck in the local tree, but
the problem had been that ->d_revalidate() is not always called on the way to
some places where ->i_uid/->i_gid is accessed ;-/

I can resurrect the analysis, but that'll take a few hours.