Re: [PATCH v2] fuse: allow FUSE drivers to declare themselves free from outside changes

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2024-04-02 at 15:23 +0200, Bernd Schubert wrote:
> 
> On 4/2/24 15:10, Jeff Layton wrote:
> > Traditionally, we've allowed people to set leases on FUSE inodes.  Some
> > FUSE drivers are effectively local filesystems and should be fine with
> > kernel-internal lease support. Others are backed by a network server
> > that may have multiple clients, or may be backed by something non-file
> > like entirely. On those, we don't want to allow leases.
> > 
> > Have the filesytem driver to set a fuse_conn flag to indicate whether
> > the inodes are subject to outside changes, not done via kernel APIs.  If
> > the flag is unset (the default), then setlease attempts will fail with
> > -EINVAL, indicating that leases aren't supported on that inode.
> > 
> > Local-ish filesystems may want to start setting this value to true to
> > preserve the ability to set leases.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This is only tested for compilation, but it's fairly straightforward.
> > 
> > I've left the default the "safe" value of false, so that we assume that
> > outside changes are possible unless told otherwise.
> > ---
> > Changes in v2:
> > - renamed flag to FUSE_NO_OUTSIDE_CHANGES
> > - flesh out comment describing the new flag
> > ---
> >  fs/fuse/file.c            | 11 +++++++++++
> >  fs/fuse/fuse_i.h          |  5 +++++
> >  fs/fuse/inode.c           |  4 +++-
> >  include/uapi/linux/fuse.h |  1 +
> >  4 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index a56e7bffd000..79c7152c0d12 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -3298,6 +3298,16 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off,
> >  	return ret;
> >  }
> >  
> > +static int fuse_setlease(struct file *file, int arg,
> > +			 struct file_lease **flp, void **priv)
> > +{
> > +	struct fuse_conn *fc = get_fuse_conn(file_inode(file));
> > +
> > +	if (fc->no_outside_changes)
> > +		return generic_setlease(file, arg, flp, priv);
> > +	return -EINVAL;
> > +}
> > +
> >  static const struct file_operations fuse_file_operations = {
> >  	.llseek		= fuse_file_llseek,
> >  	.read_iter	= fuse_file_read_iter,
> > @@ -3317,6 +3327,7 @@ static const struct file_operations fuse_file_operations = {
> >  	.poll		= fuse_file_poll,
> >  	.fallocate	= fuse_file_fallocate,
> >  	.copy_file_range = fuse_copy_file_range,
> > +	.setlease	= fuse_setlease,
> >  };
> >  
> >  static const struct address_space_operations fuse_file_aops  = {
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index b24084b60864..49d44a07b0db 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -860,6 +860,11 @@ struct fuse_conn {
> >  	/** Passthrough support for read/write IO */
> >  	unsigned int passthrough:1;
> >  
> > +	/** Can we assume that the only changes will be done via the local
> > +	 *  kernel? If the driver represents a network filesystem or is a front
> > +	 *  for data that can change on its own, set this to false. */
> > +	unsigned int no_outside_changes:1;
> > +
> >  	/** Maximum stack depth for passthrough backing files */
> >  	int max_stack_depth;
> >  
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 3a5d88878335..f33aedccdb26 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1330,6 +1330,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >  			}
> >  			if (flags & FUSE_NO_EXPORT_SUPPORT)
> >  				fm->sb->s_export_op = &fuse_export_fid_operations;
> > +			if (flags & FUSE_NO_OUTSIDE_CHANGES)
> > +				fc->no_outside_changes = 1;
> >  		} else {
> >  			ra_pages = fc->max_read / PAGE_SIZE;
> >  			fc->no_lock = 1;
> > @@ -1377,7 +1379,7 @@ void fuse_send_init(struct fuse_mount *fm)
> >  		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
> >  		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
> >  		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
> > -		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND;
> > +		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_NO_OUTSIDE_CHANGES;
> >  #ifdef CONFIG_FUSE_DAX
> >  	if (fm->fc->dax)
> >  		flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index d08b99d60f6f..703d149d45ff 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -463,6 +463,7 @@ struct fuse_file_lock {
> >  #define FUSE_PASSTHROUGH	(1ULL << 37)
> >  #define FUSE_NO_EXPORT_SUPPORT	(1ULL << 38)
> >  #define FUSE_HAS_RESEND		(1ULL << 39)
> > +#define FUSE_NO_OUTSIDE_CHANGES	(1ULL << 40)
> 
> Above all of these flags are comments explaining the flags, so that one
> doesn't need to look up in kernel sources what the exact meaning is.
> 
> Could you please add something like below?
> 
> FUSE_NO_OUTSIDE_CHANGES: No file changes through other mounts / clients
> 

Definitely. I've added that in my local branch. I can either resend
later, or maybe Miklos can just add that if he's otherwise OK with this
patch.

-- 
Jeff Layton <jlayton@kernel.org>