Message-ID: <CAOQ4uxiY=sZVsJF670T73bk2zq+LpG9B5VQA-5rOOpaSrvhdXA@mail.gmail.com>
Date:   Thu, 28 Oct 2021 07:13:10 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Vivek Goyal <vgoyal@...hat.com>
Cc:     Ioannis Angelakopoulos <iangelak@...hat.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        virtio-fs-list <virtio-fs@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>, Al Viro <viro@...iv.linux.org.uk>,
        Miklos Szeredi <miklos@...redi.hu>
Subject: Re: [RFC PATCH 1/7] FUSE: Add the fsnotify opcode and in/out structs
 to FUSE

> > you need to either include the generation in object identifier
> > or much better use the object's nfs file handle, the same way
> > that fanotify stores object identifiers.
>
> I think nfs file handle is much more complicated and its a separate
> project altogether. I am assuming we are talking about persistent
> nfs file handle as generated by host. I think biggest issue we faced
> with that is that guest is untrusted and we don't want to resolve
> file handle provided by guest on host otherwise guest can craft
> file handles and possibly be able to open other files on same filesystem
> outside shared dir.
>

Right now, virtiofsd keeps all inodes and dentries of live client inodes
pinned in cache on the server.
If you switch to file handles design, virtiofsd only need to keep a map
of all the file handles that server handed out to client to address
this concern.

For directories, this practice is not even needed for security, because
a decoded directory file handle can be verified to be within the shared dir.
It is only needed to prevent DoS, because a crafted directory file handle
(outside of shared dir) can be used to generate extra IO and thrash the
inode/dentry cache on the server.

> >
> > > +       uint64_t mask;
> > > +       uint32_t namelen;
> > > +       uint32_t cookie;
> >
> > I object to persisting with the two-events-joined-by-cookie design.
> > Any new design should include a single event for rename
> > with information about src and dst.
> >
> > I know this is inconvenient, but we are NOT going to create a "remote inotify"
> > interface, we need to create a "remote fsnotify" interface and if server wants
> > to use inotify, it will need to join the disjoined MOVE_FROM/TO event into
> > a single "remote event", that FUSE will use to call fsnotify_move().
>
> man inotify says following.
>
> "       Matching up the IN_MOVED_FROM and IN_MOVED_TO event pair  generated  by
>        rename(2)  is thus inherently racy.  (Don't forget that if an object is
>        renamed outside of a monitored directory, there  may  not  even  be  an
>        IN_MOVED_TO  event.)"
>
> So if guest is no monitoring target dir of renamed file, then we will not
> even get IN_MOVED_TO. In that case we can't merge two events into one.
>
> And this sounds like inotify/fanotify interface needs to come up with
> an merged event and in that case remote filesystem will simply propagate
> that event. (Instead of coming up with a new event only for remote
> filesystems. Sounds like this is not a problem limited to remote
> filesystems only).
>

I don't see it that way.
I see the "internal protocol" for filesystems/vfs to report rename is
fsnotify_move() which carries information about both src and target.
I would like the remote protocol  to carry the same information.
It is then up to the userspace API whether to report the rename
as two events or a unified event.

For example, debugfs, calls fsnotify_move() when an object is renamed
from underneath the vfs. It does not call fsnotify() twice with a cookie,
because we would not want to change local filesystem nor remote protocols
when we want to add new userspace APIs for reporting fsevents.

That comes down to my feeling that this claims to be a proposal
for "remote fsnotify", but it looks and sounds like a proposal for
"remote inotify" and this is the core of my objection to passing
the rename cookie in the protocol.

Regarding the issue that the src/dst path may not be known
to the server, as I wrote, it is fine if either the src/dst path information
is omitted from an event, but the protocol should provide the
placeholder to report them.

After sufficient arguing, I might be convinced that the cookie may be included
as an optional field in addition to the fields that I requested.

I understand why you write that this sounds like an fanotify interface
that needs to be resolved and you are correct, but the reason that the
fanotify interface issue was not yet resolved is that we are trying to not
repeat the mistakes of the past and for that same reason, I am insisting
on the protocol.

Thanks,
Amir.

Powered by blists - more mailing lists