Re: [PATCH v4 0/3] fanotify accounting for fs/splice.c

[Amir Goldstein]

On Wed, Jun 28, 2023 at 2:38 PM Jan Kara <jack@xxxxxxx> wrote:
>
> Hello!
>
> On Tue 27-06-23 22:50:46, Ahelenia Ziemiańska wrote:
> > Always generate modify out, access in for splice;
> > this gets automatically merged with no ugly special cases.
> >
> > No changes to 2/3 or 3/3.
>
> Thanks for the patches Ahelena! The code looks fine to me but to be honest
> I still have one unresolved question so let me think about it loud here for
> documentation purposes :). Do we want fsnotify (any filesystem
> notification framework like inotify or fanotify) to actually generate
> events on FIFOs? FIFOs are virtual objects and are not part of the
> filesystem as such (well, the inode itself and the name is), hence
> *filesystem* notification framework does not seem like a great fit to watch
> for changes or accesses there. And if we say "yes" for FIFOs, then why not
> AF_UNIX sockets? Where do we draw the line? And is it all worth the
> trouble?
>
> I understand the convenience of inotify working on FIFOs for the "tail -f"
> usecase but then wouldn't this better be fixed in tail(1) itself by using
> epoll(7) for FIFOs which, as I've noted in my other reply, does not have
> the problem that poll(2) has when there are no writers?
>
> Another issue with FIFOs is that they do not have a concept of file
> position. For hierarchical storage usecase we are introducing events that
> will report file ranges being modified / accessed and officially supporting
> FIFOs is one more special case to deal with.
>
> What is supporting your changes is that fsnotify mostly works for FIFOs
> already now (normal reads & writes generate notification) so splice not
> working could be viewed as an inconsistency. Sockets (although they are
> visible in the filesystem) cannot be open so for them the illusion of being
> a file is even weaker.
>
> So overall I guess I'm slightly in favor of making fsnotify generate events
> on FIFOs even with splice, provided Amir does not see a big trouble in
> supporting this with his upcoming HSM changes.
>

I've also thought about this.

The thing about the HSM events is that they are permission events
and just like FAN_ACCESS_PERM, they originate from the common
access control helpers {rw,remap}_verify_area(), which also happen
to have the file range info (with ppos NULL for pipes).

Ahelenia's patches do not add any new rw_verify_area() to pipes
so no new FAN_ACCESS_PERM events were added.

If we could go back to the design of fanotify we would have probably
made it explicit that permission events are only allowed on regular
files and dirs. For the new HSM events we can (and will) do that.

In any case, the new events are supposed to be delivered with
file access range records, so delivering HSM events on pipes
wouldn't make any sense.

So I do not see any problem with these patches wrt upcomping
HSM events.

However, note that these patches create more inconsistencies
between IN_ACCESS and FAN_ACCESS_PERM on pipes.

We can leave it at that if we want, but fixing the inconsistencies
by adding more FAN_ACCESS_PERM events on pipes - this
is not something that I wouldn't be comfortable with.

If anything, we can remove FAN_ACCESS_PERM events from
special files and see if anybody complains.

I don't know of any users of FAN_ACCESS_PERM and even for
FAN_OPEN_PERM, I don't think that AV-vendors have anything
useful to do with open permission events on special files.

Thanks,
Amir.