Re: [PATCH 0/7] Report more information in fanotify dirent events

On Sat 13-11-21 21:31:25, Amir Goldstein wrote:

On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein [off-list ref] wrote:

quoted

On Fri, Nov 12, 2021 at 6:39 PM Jan Kara [off-list ref] wrote:

quoted

Hi Amir!

On Sat 06-11-21 18:29:39, Amir Goldstein wrote:

quoted

On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein [off-list ref] wrote:

quoted

This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
from 3 months ago.

With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
I figured we could get an early (re)start of the discussion on
FAN_REPORT_TARGET_FID towards 5.17.

The added information in dirent events solves problems for my use case -
It helps getting the following information in a race free manner:
1. fid of a created directory on mkdir
2. from/to path information on rename of non-dir

I realize those are two different API traits, but they are close enough
so I preferred not to clutter the REPORT flags space any further than it
already is. The single added flag FAN_REPORT_TARGET_FID adds:
1. child fid info to CREATE/DELETE/MOVED_* events
2. new parent+name info to MOVED_FROM event

Instead of going the "inotify way" and trying to join the MOVED_FROM/
MOVED_TO events using a cookie, I chose to incorporate the new
parent+name intomation only in the MOVED_FROM event.
I made this choice for several reasons:
1. Availability of the moved dentry in the hook and event data
2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
   DFID_NAME info to statat(2) the object as we suggested

I chose to reduce testing complexity and require all other FID
flags with FAN_REPORT_TARGET_FID and there is a convenience
macro FAN_REPORT_ALL_FIDS that application can use.

Self comment - Don't use ALL_ for macro names in uapi...
There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...

Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
another FID flag later ;)

quoted

BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
not because I object to FAN_RENAME, just because it was simpler to implement
the MOVED_FROM alternative, so I thought I'll start with this proposal
and see how
it goes.

I've read through all the patches and I didn't find anything wrong.
Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
fsnotify_name() once more with FS_RENAME event and we'd gate addition of
second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
lean a bit more towards that.

I grew to like FAN_RENAME better myself as well.
To make sure we are talking about the same thing:
1. FAN_RENAME always reports 2*(dirfid+name)
2. FAN_REPORT_TARGET_FID adds optional child fid record to
    CREATE/DELETE/RENAME/MOVED_TO/FROM

Correct, that's what I meant.

Err, I tried the FAN_RENAME approach and hit a semantic issue:
Users can watch a directory inode and get only MOVED_FROM
when entries are moved from this directory. Same for MOVED_TO.
How would FAN_RENAME behave when setting FAN_RENAME on a directory inode?
Should listeners get events on files renamed in and out of that
directory?

I see several options:
1. Go back to FAN_MOVED_FROM as in this patch set, where semantics are clear

Well, semantics are clear but in principle user does not have access to
target dir either so the permission problems are the same as with option 2,
aren't they?

2. Report FAN_RENAME if either old or new dir is watched (or mount/sb)
3. Report FAN_RENAME only if both old and new dirs are watched (or mount/sb)

For option 2, we may need to hide information records, For example,
because an unprivileged listener may not have access to old or new
directory.

Good spotting. That can indeed be a problem.

A variant of option 3, is that FAN_RENAME will be an event mask flag
that can be added to FAN_MOVE events, to request that if both FROM/TO events
are going to be reported, then a single joint event will be reported
instead, e.g:

#define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO)
#define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN)

Instead of generating an extra FS_RENAME event in fsnotify_move(),
fsnotify() will search for matching marks on the moved->d_parent->d_inode
of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT
mark iterator type and then fanotify_group_event_mask() will be able
to tell if the
event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint
FAN_RENAME.

If a group has the FAN_RENAME mask on the new parent dir, then
FS_MOVED_TO events can be dropped, because the event was already
reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM
event.

Am I over complicating this?
Do you have a better and clearer semantics to propose?

So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO &
FAN_MOVED_FROM. It would be generated whenever source or target is tagged
with FAN_RENAME, source info is provided if source is tagged, target info
is provided when target is tagged (both are provides when both are tagged).
So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed
merging. This looks like a clean enough and simple to explain API. Sure it
duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity
of the API outweights the duplication. Basically FAN_MOVED_FROM &
FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although
I don't think we want to do it for compatibility reasons.

Implementation-wise we have couple of options. Currently the simplest I can
see is that fsnotify() would iterate marks on both source & target dirs
(like we already do for inode & parent) when it handles FS_RENAME event. In
fanotify_handle_event() we will decide which info to report with FAN_RENAME
event based on which marks in iter_info have FS_RENAME set (luckily mount
marks are out of question for rename events so it will be relatively
simple). What do you think?

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR