Re: [RFC][PATCH] fanotify: allow to set errno in FAN_DENY permission response

From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Josef Bacik <josef@toxicpanda.com>,
	Christian Brauner <brauner@kernel.org>,
	 linux-fsdevel@vger.kernel.org, Sargun Dhillon <sargun@sargun.me>,
	 Sweet Tea Dorminy <thesweettea@meta.com>
Subject: Re: [RFC][PATCH] fanotify: allow to set errno in FAN_DENY permission response
Date: Tue, 6 Feb 2024 18:35:34 +0200	[thread overview]
Message-ID: <CAOQ4uxgMKjEMjPP5HBk0kiZTfkqGU-ezkVpeS22wxL=JmUqhuQ@mail.gmail.com> (raw)
In-Reply-To: <20240205182718.lvtgfsxcd6htbqyy@quack3>

On Mon, Feb 5, 2024 at 8:27 PM Jan Kara <jack@suse.cz> wrote:
>
> I'm sorry for the delay. The last week was busy and this fell through the
> cracks.
>

No worries, I was busy as well.
I did already rebase fan_pre_content & fan_errno [1] (over v6.8-rc2)
last week, made the small changes I mention here and ran some
basic tests, but did not complete writing tests.
Hoping to switch back to it this week.

[1] https://github.com/amir73il/linux/commits/fan_errno

> On Mon 29-01-24 20:30:34, Amir Goldstein wrote:
> > On Mon, Dec 18, 2023 at 5:53 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > In the HttpDirFS HSM demo, I used FAN_OPEN_PERM on a mount mark
> > > to deny open of file during the short time that it's content is being
> > > punched out [1].
> > > It is quite complicated to explain, but I only used it for denying access,
> > > not to fill content and not to write anything to filesystem.
> > > It's worth noting that returning EBUSY in that case would be more meaningful
> > > to users.
> > >
> > > That's one case in favor of allowing FAN_DENY_ERRNO for FAN_OPEN_PERM,
> > > but mainly I do not have a proof that people will not need it.
> > >
> > > OTOH, I am a bit concerned that this will encourage developer to use
> > > FAN_OPEN_PERM as a trigger to filling file content and then we are back to
> > > deadlock risk zone.
> > >
> > > Not sure which way to go.
> > >
> > > Anyway, I think we agree that there is no reason to merge FAN_DENY_ERRNO
> > > before FAN_PRE_* events, so we can continue this discussion later when
> > > I post FAN_PRE_* patches - not for this cycle.
> >
> > I started to prepare the pre-content events patches for posting and got back
> > to this one as well.
> >
> > Since we had this discussion I have learned of another use case that
> > requires filling file content in FAN_OPEN_PERM hook, FAN_OPEN_EXEC_PERM
> > to be exact.
> >
> > The reason is that unless an executable content is filled at execve() time,
> > there is no other opportunity to fill its content without getting -ETXTBSY.
>
> Yes, I've been scratching my head over this usecase for a few days. I was
> thinking whether we could somehow fill in executable (and executed) files on
> access but it all seemed too hacky so I agree that we probably have to fill
> them in on open.
>

Normally, I think there will not be a really huge executable(?)
If there were huge executables, they would have likely been broken down
into smaller loadable libraries which should allow more granular
content filling,
but I guess there will always be worst case exceptions.

> > So to keep things more flexible, I decided to add -ETXTBSY to the
> > allowed errors with FAN_DENY_ERRNO() and to decided to allow
> > FAN_DENY_ERRNO() with all permission events.
> >
> > To keep FAN_DENY_ERRNO() a bit more focused on HSM, I have
> > added a limitation that FAN_DENY_ERRNO() is allowed only for
> > FAN_CLASS_PRE_CONTENT groups.
>
> I have no problem with adding -ETXTBSY to the set of allowed errors. That
> makes sense. Adding FAN_DENY_ERRNO() to all permission events in
> FAN_CLASS_PRE_CONTENT groups - OK,

done that.

I am still not very happy about FAN_OPEN_PERM being part of HSM
event family when I know that O_TRUCT and O_CREAT call this hook
with sb writers held.

The irony, is that there is no chance that O_TRUNC will require filling
content, same if the file is actually being created by O_CREAT, so the
cases where sb writers is actually needed and the case where content
filling is needed do not overlap, but I cannot figure out how to get those
cases out of the HSM risk zone. Ideas?

> if we don't find anything better - I
> wanted to hash out another possibility here: Currently all permission
> events (and thus also the events we plan to use for HSM AFAIU) are using
> 'fd' to identify file where the event happened. This is used as identifier
> for response, can be used to fill in file contents for HSM but it also
> causes issues such as the problem with exec(2) occasionally failing if this
> fd happens to get closed only after exec(2) gets to checking
> deny_write_access(). So what if we implemented events needed for HSM as FID
> events (we'd have think how to match replies to events)? Then the app would
> open the file for filling in using FID as well as it would naturally close
> the handle before replying so problems with exec(2) would not arise. These

The two things are independent IMO.
We can use an event->key instead of event->fd, which I like,
but we may still leave event->fd as a way to get an FMODE_NONOTIFY
fd as long as the user closes event->fd before responding or we can
implement Sargun's suggestion of the FAN_CLOSE_FD response flag.

If a user needs to open an FMODE_NONOTIFY fd from fid, we will
need to provide a way to do that.
My WIP pre-lookup event patches [2] implements inheritance of
FMODE_NONOTIFY from dirfd used for openat().
Perhaps we can do the same for open_by_handle_at() and inherit
FMODE_NONOTIFY from mount_fd to implement your suggestion?

[2] https://github.com/amir73il/linux/commits/fan_lookup_perm

> would be essentially new events (so far we didn't allow permission events
> in FID groups) so allowing FAN_DENY_ERRNO() replies for them would be
> natural. Overall it would seem like a cleaner "clean room implementation"
> API?

I like the idea of a clean slate.

Looking a head, for the PRE_PATH events (e.g. lookup,create)
I was planning to use FAN_EVENT_INFO_TYPE_DFID_NAME
to carry the last component lookup name, but then also have
event->fd as the dirfd of lookup/create.

That's a bit ugly duplicity and also it does not cover rename(), because
if we use FAN_EVENT_INFO_TYPE_{OLD,NEW}_DFID_NAME
to report names, where would newdirfd, olddirfd be reported?

Your suggestion solves both these questions elegantly and
if you agree to adapting open_by_handle_at() to cater fanotify needs,
then we have a plan to propose.

The bad side of clean slate is that it reduces the chances of me
being able to get pre-content events ready in time for 6.9, which
is a shame, but we got to do what we got to do ;)

Thanks,
Amir.