| 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
| ||
|
Message-ID: <CAOQ4uxiv7xSUS7RDK3esa1Crp8reYXewxkr5fFbhG623P20PwA@mail.gmail.com>
Date: Tue, 2 Apr 2024 17:02:12 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Jeff Layton <jlayton@...nel.org>
Cc: Bernd Schubert <bernd.schubert@...tmail.fm>, Miklos Szeredi <miklos@...redi.hu>,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] fuse: allow FUSE drivers to declare themselves free
from outside changes
On Tue, Apr 2, 2024 at 4:29 PM Jeff Layton <jlayton@...nel.org> wrote:
>
> 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@...nel.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.
Don't love the name but don't have any suggestions either.
I am wondering out loud, if we have such a mode for the fs,
if and how should it affect caching configuration?
Thanks,
Amir.
Powered by blists - more mailing lists