Received-SPF: pass (google.com: domain of linux-kernel+bounces-16060-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 3 Jan 2024 22:14:44 +0000
From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Christian Brauner <brauner@kernel.org>,
	linux-fsdevel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] eventfs: Stop using dcache_readdir() for getdents()
Message-ID: <20240103221444.GM1674809@ZenIV>
References: <20240103102553.17a19cea@gandalf.local.home>
 <CAHk-=whrRobm82kcjwj625bZrdK+vvEo0B5PBzP+hVaBcHUkJA@mail.gmail.com>
 <CAHk-=wjVdGkjDXBbvLn2wbZnqP4UsH46E3gqJ9m7UG6DpX2+WA@mail.gmail.com>
 <20240103145306.51f8a4cd@gandalf.local.home>
 <CAHk-=wg=tnnsTjnzTs8xRQOBLvw4ceKe7=yxfzNtx4Z9gb-xJw@mail.gmail.com>
 <CAHk-=wh5kkk2+JAv_D1fm8t1SOpTQyb4n7zuMuVSBG094HH7gA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wh5kkk2+JAv_D1fm8t1SOpTQyb4n7zuMuVSBG094HH7gA@mail.gmail.com>
Sender: Al Viro <viro@ftp.linux.org.uk>

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.