Re: Fanotify API - Tracking File Movement

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat 07-05-22 19:03:13, Amir Goldstein wrote:
> > So I've slept on it and agree that allowing FAN_RENAME on a file with the
> > semantics Matthew wants is consistent with the current design and probably
> > the only sensible meaning we can give to it. I also agree that updating
> > permission checks for reporting parent dirs isn't that big of a headache
> > and maintenance burden going further.
> >
> > I'm still somewhat concerned about how the propagation of two parent
> > directories and then formatting into the event is going to work out (i.e.,
> > how many special cases that's going to need) but I'm willing to have a look
> > at the patch. Maybe it won't be as bad as I was afraid :).
> 
> Here is a patch. I don't think it is so bad.(?)
> In fact, I think the code is made a bit more clear when we stop overloading
> ITER_TYPE_INODE as the new_dir in FS_RENAME case.
> But yes, it does add yet another special (moved non-dir vs. moved dir).

Yeah, it looks fine.

> > > > Ah, I really wanted to stay away from watching the super block for all
> > > > FAN_RENAME events. I feel like userspace wearing the pain for such
> > > > cases is suboptimal, as this is something that can effectively be done
> > > > in-kernel.
> >
> > I agree that kernel can do this more efficiently than userspace but the
> > question is how much in terms of code (and thus maintenance overhead) are
> > we willing to spend for this IMO rather specialized feature. The code to
> > build necessary information to pass with the event, dealing with all
> > different types of watches and backends and then formatting it to the event
> > for userspace is complex as hell. Whenever I have to do or review some
> > non-trivial changes to it, my head hurts ;) And the amount of subtle
> > cornercase bugs we were fixing in that code over the years is just a
> > testimony of this. So that's why I'm reluctant to add even small
> > complications to it for something I find relatively specialized use (think
> > for how many userspace programs this feature is going to be useful, I don't
> > think many).
> >
> 
> My 0.02$ - while FAN_RENAME is a snowflake, this is not because
> of our design, this is because rename(2) is a snowflake vfs operation.
> The event information simply reflects the operation complexity and when
> looking at non-Linux filesystem event APIs, the event information for rename
> looks very similar to FAN_RENAME. In some cases (lustre IIRC) the protocol
> was enhanced at some point exactly as we did with FAN_RENAME to
> have all the info in one event vs. having to join two events.

I agree that rename is a special operation and that reflects to some extent
in fsnotify as well.


> But... (there is always a but when it comes to UAPI),
> When looking at my patch, one cannot help wondering -
> what about FAN_CREATE/FAN_DELETE/FAN_MOVE?
> If those can report child fid, why should they be treated differently
> than FAN_RENAME w.r.t marking the child inode?
> 
> For example, when watching a non-dir for FAN_CREATE, it could
> be VERY helpful to get the dirfid+name of where the inode was
> hard linked. In fact, if an application is watching FAN_RENAME to track
> the movement of a non-dir file and does not watch hardlink+unlink, then
> the file could escape under the application's nose.

I agree that being able to track FAN_CREATE (or FAN_DELETE for that matter
if you'd like to maintain in which dirs a file is visible) for a file like
this would be useful in a similar way as what we now enable for rename.

> We should definitely not extend the UAPI to a non-dir file before we provide
> an answer to this question. For that reason I also sent v2 of Fix patch to
> deny setting all dirent events on non-dir with FAN_REPORT_TARGET_FID.

Agreed.

> From d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6 Mon Sep 17 00:00:00 2001
> From: Amir Goldstein <amir73il@xxxxxxxxx>
> Date: Sat, 7 May 2022 10:04:08 +0300
> Subject: [PATCH] fsnotify: send FS_RENAME to groups watching the moved inode
> 
> Send FS_RENAME to groups watching the moved inode itself if the
> moved inode is a non-dir to allow tracking the movement of a watched
> inode.
> 
> Sending FS_RENAME to a moved watched directory would be confusing
> and FAN_MOVE_SELF provided enough information to track the movements
> of a watched directory.
> 
> At the moment, no backend allows watching FS_RENAME on a non-dir inode.
> When backends (i.e. fanotify) will allow watching FS_RENAME on a non-dir
> inode, old/new dir+name should be reported only if the group proves to
> have read permission on old/new dir.
> 
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>  fs/notify/fanotify/fanotify.c    |  4 +-
>  fs/notify/fsnotify.c             | 72 ++++++++++++++++++++------------
>  include/linux/fsnotify.h         | 19 +++++----
>  include/linux/fsnotify_backend.h |  6 ++-
>  4 files changed, 63 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 985e995d2a39..fc498fc090da 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -780,9 +780,9 @@ static struct fanotify_event *fanotify_alloc_event(
>  			report_old = report_new =
>  				match_mask & (1U << FSNOTIFY_ITER_TYPE_SB);
>  			report_old |=
> -				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE);
> +				match_mask & (1U << FSNOTIFY_ITER_TYPE_OLD_DIR);
>  			report_new |=
> -				match_mask & (1U << FSNOTIFY_ITER_TYPE_INODE2);
> +				match_mask & (1U << FSNOTIFY_ITER_TYPE_NEW_DIR);
>  
>  			if (!report_old) {
>  				/* Do not report old parent+name */
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 6eee19d15e8c..ce1ae725ec8c 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -277,19 +277,19 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
>  	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
>  		return 0;
>  
> -	/*
> -	 * For FS_RENAME, 'dir' is old dir and 'data' is new dentry.
> -	 * The only ->handle_inode_event() backend that supports FS_RENAME is
> -	 * dnotify, where it means file was renamed within same parent.
> -	 */
>  	if (mask & FS_RENAME) {
> -		struct dentry *moved = fsnotify_data_dentry(data, data_type);
> +		inode_mark = fsnotify_iter_old_dir_mark(iter_info);
> +		parent_mark = fsnotify_iter_new_dir_mark(iter_info);

AFAIU you use parent_mark here as a temporary variable. Maybe it would be
less confusing to just declare new 'new_dir_mark' variable to use for
comparison here? Also we would not have to change the "if (parent_mark)"
condition below.
  
> -		if (dir != moved->d_parent->d_inode)
> +		/*
> +		 * The only ->handle_inode_event() backend that supports
> +		 * FS_RENAME is dnotify, where DN_RENAME means that file
> +		 * was renamed within the same parent.
> +		 */
> +		if (WARN_ON_ONCE(!inode_mark) ||
> +		    inode_mark != parent_mark)
>  			return 0;
> -	}
> -
> -	if (parent_mark) {
> +	} else if (parent_mark) {
>  		/*
>  		 * parent_mark indicates that the parent inode is watching
>  		 * children and interested in this event, which is an event

...

> @@ -491,19 +491,31 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
>  	if (!inode) {
>  		/* Dirent event - report on TYPE_INODE to dir */
>  		inode = dir;
> -		/* For FS_RENAME, inode is old_dir and inode2 is new_dir */
> -		if (mask & FS_RENAME) {
> -			moved = fsnotify_data_dentry(data, data_type);
> -			inode2 = moved->d_parent->d_inode;
> -			inode2_type = FSNOTIFY_ITER_TYPE_INODE2;
> -		}
> +	} else if (mask & FS_RENAME) {
> +		/* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
> +		moved = fsnotify_data_dentry(data, data_type);
> +		dir1 = moved->d_parent->d_inode;
> +		dir2 = dir;
> +		if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
> +			dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
> +		/*
> +		 * Send FS_RENAME to groups watching the moved inode itself
> +		 * only if the moved inode is a non-dir.
> +		 * Sending FS_RENAME to a moved watched directory would be
> +		 * confusing and FS_MOVE_SELF provided enough information to
> +		 * track the movements of a watched directory.
> +		 */
> +		if (mask & FS_ISDIR)
> +			inode = NULL;

So I agree that sending FS_RENAME to a directory when the directory itself
moves is confusing. But then it makes me wonder whether it would not be
more logical if we extended FS_MOVE_SELF rather than FS_RENAME. Currently
FS_MOVE_SELF is an inode event but we could expand it to provide new parent
directory to priviledged listeners (and even old directory if we wanted).
But I guess the concern is that we'd have to introduce a group flag for
this to make sure things are backward compatible, whereas with FAN_RENAME
we could mostly get away without a feature flag?

> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index bb8467cd11ae..75f1048443a5 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -28,18 +28,19 @@
>   */
>  static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
>  				struct inode *dir, const struct qstr *name,
> -				u32 cookie)
> +				struct inode *inode, u32 cookie)

So you add 'inode' argument to fsnotify_name() but never actually use it?
The only place where inode would be non-NULL is changed to use fsnotify()
directly...

Also I'm not sure why you pass the inode for rename event now when the same
inode is passed as 'dentry' in data? I feel like I'm missing something
here.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux