Re: [PATCH v2 0/2] New fanotify API for ignoring events

[Jan Kara <jack@suse.cz>]

Hi Amir!

On Sun 26-06-22 18:57:01, Amir Goldstein wrote:
> On Fri, Jun 24, 2022 at 5:35 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > As we discussed [1], here is the implementation of the new
> > FAN_MARK_IGNORE API, to try and sort the historic mess of
> > FAN_MARK_IGNORED_MASK.
> 
> When we started talking about adding FAN_MARK_IGNORE
> it was to address one specific flaw of FAN_MARK_IGNORED_MASK,
> but after staring at the API for some time, I realized there are other
> wrinkles with FAN_MARK_IGNORED_MASK that could be addressed
> by a fresh new API.
> 
> I added more input validations following the EEXIST that you requested.
> The new errors can be seen in the ERRORS section of the man page [3].
> The new restrictions will reduce the size of the test matrix, but I did not
> update the LTP tests [2] to check the new restrictions yet.
> 
> I do not plan to post v3 patches before improving the LTP tests,
> but I wanted to send this heads up as an API proposal review.
> The kernel commit that adds FAN_MARK_IGNORE [1] summarize the
> new API restrictions as follows:
> 
>     The new behavior is non-downgradable.  After calling fanotify_mark() with
>     FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
>     on the same object will return EEXIST error.
> 
>     Setting the event flags with FAN_MARK_IGNORE on a non-dir inode mark
>     has no meaning and will return ENOTDIR error.
> 
>     The meaning of FAN_MARK_IGNORED_SURV_MODIFY is preserved with the new
>     FAN_MARK_IGNORE flag, but with a few semantic differences:
> 
>     1. FAN_MARK_IGNORED_SURV_MODIFY is required for filesystem and mount
>        marks and on an inode mark on a directory. Omitting this flag
>        will return EINVAL or EISDIR error.
> 
>     2. An ignore mask on a non-directory inode that survives modify could
>        never be downgraded to an ignore mask that does not survive modify.
>        With new FAN_MARK_IGNORE semantics we make that rule explicit -
>        trying to update a surviving ignore mask without the flag
>        FAN_MARK_IGNORED_SURV_MODIFY will return EEXIST error.
> 
>     The conveniene macro FAN_MARK_IGNORE_SURV is added for
>     (FAN_MARK_IGNORE | FAN_MARK_IGNORED_SURV_MODIFY), because the
>     common case should use short constant names.

This looks good to me. Thanks for working on this. The only additional
thing that occurred to me is that we might want to restrict events usable
with FAN_MARK_IGNORE similarly as we did restrict non-sensical events with
FAN_REPORT_TARGET_FID but after putting more thought into it I'm not sure
it is such a great idea because it is not so obvious which events may
be valid to ignore for a particular object and how is it with backward
compatibility when a new type of event becomes possible for an object.

								Honza
 
> [1] https://github.com/amir73il/linux/commits/fan_mark_ignore
> [2] https://github.com/amir73il/ltp/commits/fan_mark_ignore
> [3] https://github.com/amir73il/man-pages/commits/fan_mark_ignore
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR