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

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Feb 8, 2024 at 8:31 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 08-02-24 16:04:29, Amir Goldstein wrote:
> > > > 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.
> > > > > > 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?
> > >
> >
> > Jan,
> >
> > I wanted to run an idea by you.
> >
> > I like your idea to start a clean slate with
> > FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID
> > and it would be nice if we could restrict this HSM to use
> > pre-content events, which is why I was not happy about allowing
> > FAN_DENY_ERRNO() for the legacy FAN_OPEN*_PERM events,
> > especially with the known deadlocks.
> >
> > Since we already know that we need to generate
> > FAN_PRE_ACCESS(offset,length) for read-only mmap() and
> > FAN_PRE_MODIFY(offset,length) for writable mmap(),
> > we could treat uselib() and execve() the same way and generate
> > FAN_PRE_ACCESS(0,i_size) as if the file was mmaped.
>
> BTW uselib() is deprecated and there is a patch queued to not generate
> OPEN_EXEC events for it because it was causing problems (not the generation
> of events itself but the FMODE_EXEC bit being set in uselib). So I don't
> think we need to instrument uselib().
>

Great. The fewer the better :)

BTW, for mmap, I was thinking of adding fsnotify_file_perm() next to
call sites of security_mmap_file(), but I see that:
1. shmat() has security_mmap_file() - is it relevant?
2. remap_file_pages() calls do_mmap() without security_mmap_file() -
    do we need to cover it?

> > My idea is to generate an event FAN_PRE_MODIFY(0,0)
> > for an open for write *after* file was truncated and
> > FAN_PRE_ACCESS(0,0) for open O_RDONLY.
>
> What I find somewhat strange about this is that if we return error from the
> fsnotify_file_perm() hook, open(2) will fail with error but the file is
> already truncated. But I guess it should be rare and it's bearable.
>
> > Possibly also FAN_PRE_*(offset,0) events for llseek().
>
> That seem overdoing it a bit IMO :)

Heh! forget I said it ;)

>
> > I've already pushed a POC to fan_pre_content branch [1].
> > Only sanity tested that nothing else is broken.
> > I still need to add the mmap hooks and test the new events.
> >
> > With this, HSM will have appropriate hooks to fill executable
> > and library on first access and also fill arbitrary files on open
> > including the knowledge if the file was opened for write.
> >
> > Thoughts?
>
> Yeah, I guess this looks sensible.
>

Cool, so let's see, what is left to do for the plan of
FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID?

1. event->fd is O_PATH mount_fd for open_by_handle_at()
2. open_by_handle_at() inherits FMODE_NONOTIFY from mount_fd
3. either implement the FAN_CLOSE_FD response flag (easy?) and/or
    implement FAN_REPORT_EVENT_ID and new header format

Anything else?
Are you ok with 1 and 2?
Do you have a preference for 3?

Thanks,
Amir.