Message-ID: <Z7T_K8xdJOxN7C0D@dread.disaster.area>
Date: Wed, 19 Feb 2025 08:44:11 +1100
From: Dave Chinner <david@...morbit.com>
To: Miklos Szeredi <miklos@...redi.hu>
Cc: Luis Henriques <luis@...lia.com>, Bernd Schubert <bschubert@....com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Matt Harvey <mharvey@...ptrading.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] fuse: add new function to invalidate cache for
 all inodes

On Tue, Feb 18, 2025 at 10:15:26AM +0100, Miklos Szeredi wrote:
> On Tue, 18 Feb 2025 at 01:55, Dave Chinner <david@...morbit.com> wrote:
> >
> > On Mon, Feb 17, 2025 at 01:32:28PM +0000, Luis Henriques wrote:
> > > Currently userspace is able to notify the kernel to invalidate the cache
> > > for an inode.  This means that, if all the inodes in a filesystem need to
> > > be invalidated, then userspace needs to iterate through all of them and do
> > > this kernel notification separately.
> > >
> > > This patch adds a new option that allows userspace to invalidate all the
> > > inodes with a single notification operation.  In addition to invalidate
> > > all the inodes, it also shrinks the sb dcache.
> >
> > You still haven't justified why we should be exposing this
> > functionality in a low level filesystem ioctl out of sight of the
> > VFS.
> >
> > User driven VFS cache invalidation has long been considered a
> > DOS-in-waiting, hence we don't allow user APIs to invalidate caches
> > like this. This is one of the reasons that /proc/sys/vm/drop_caches
> > requires root access - it's system debug and problem triage
> > functionality, not a production system interface....
> >
> > Every other situation where filesystems invalidate vfs caches is
> > during mount, remount or unmount operations.  Without actually
> > explaining how this functionality is controlled and how user abuse
> > is not possible (e.g. explain the permission model and/or how only
> > root can run this operation), it is not really possible to determine
> > whether we should unconditional allow VFS cache invalidation outside
> > of the existing operation scope....
> 
> I think you are grabbing the wrong end of the stick here.
> 
> This is not about an arbitrary user being able to control caching
> behavior of a fuse filesystem.

Except the filesytsem is under control of some user, yes? Nobody has
explained why such functionality is actually safe to expose to user
controlled filesystems from a security and permissions perspective.
At minimum, this needs to be explained in the commit message so when
it gets abused years down the track we have a record of why we
thought this was a safe thing to do...

> It's about the filesystem itself being
> able to control caching behavior.

I'll give the same response regardless of the filesystem - walking
the list of all VFS cached inodes to invalidate them -does not work-
to invalidate the entire VFS inode cache. That guarantee is only
provided in unmount and mount contexts once we have guaranteed that
there are no remaining active references to any cached inode.

i.e. this is functionality that -does not work- as the filesystem
might want it to work. revoke() is what is needed to do full VFS
cache invaliadtions, but that does not exist....

> I'm not arguing for the validity of this particular patch, just saying
> that something like this could be valid.  And as explained in my other
> reply there's actually a real problem out there waiting for a
> solution.

And I've already pointed out that the solution lies within that
filesystem impementation, not the VFS.

i.e. If a filesystem needs to invalidate all existing inode objects
it has instantiated regardless of whether they are actively
referenced or not by other VFS objects, then the filesystem itself
needs to implement the invalidation mechanism itself to ensure
persistently referenced VFS inodes are correctly handled....

-Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists