Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <t5az5bvpfqd3rrwla43437r5vplmkujdytixcxgm7sc4hojspg@jcc63stk66hz>
 <cover.1687898895.git.nabijaczleweli@nabijaczleweli.xyz> <20230628113853.2b67fic5nvlisx3r@quack3>
In-Reply-To: <20230628113853.2b67fic5nvlisx3r@quack3>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Wed, 28 Jun 2023 16:41:14 +0300
Message-ID: <CAOQ4uxhcZY1XSZ74wUuy=3tqfEW0vbOuwghc4ZoQt=FZ+Lw4-A@mail.gmail.com>
Subject: Re: [PATCH v4 0/3] fanotify accounting for fs/splice.c
To:     Jan Kara <jack@suse.cz>
Cc:     =?UTF-8?Q?Ahelenia_Ziemia=C5=84ska?= 
        <nabijaczleweli@nabijaczleweli.xyz>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Chung-Chiang Cheng <cccheng@synology.com>, ltp@lists.linux.it
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 28, 2023 at 2:38=E2=80=AFPM Jan Kara <jack@suse.cz> wrote:
>
> Hello!
>
> On Tue 27-06-23 22:50:46, Ahelenia Ziemia=C5=84ska 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 hones=
t
> I still have one unresolved question so let me think about it loud here f=
or
> 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 wat=
ch
> for changes or accesses there. And if we say "yes" for FIFOs, then why no=
t
> 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 supporti=
ng
> 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 bei=
ng
> a file is even weaker.
>
> So overall I guess I'm slightly in favor of making fsnotify generate even=
ts
> 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.