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

[Jan Kara <jack@suse.cz>]

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().

> 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 :)

> 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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR