Re: [PATCH v3 08/13] fanotify: enable FAN_REPORT_FID init flag

[Jan Kara <jack@suse.cz>]

On Sun 25-11-18 15:43:47, Amir Goldstein wrote:
> When setting up an fanotify listener, user may request to get fid
> information in event instead of an open file descriptor.
> 
> The fid obtained with event on a watched object contains the file
> handle returned by name_to_handle_at(2) and fsid returned by statfs(2).
> 
> When setting a mark, we need to make sure that the filesystem
> supports encoding file handles with name_to_handle_at(2) and that
> statfs(2) encodes a non-zero fsid.
...
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index ea8e81a3e80b..d7aa2f392a64 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -857,6 +859,49 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  	return fd;
>  }
>  
> +/* Check if filesystem can encode a unique fid */
> +static int fanotify_test_fid(struct path *path)
> +{
> +	struct kstatfs stat, root_stat;
> +	int err;
> +
> +	/*
> +	 * Make sure path is not in filesystem with zero fsid (e.g. tmpfs).
> +	 * TODO: cache fsid in the mark connector.
> +	 */

TODO in a submitted patch looks strange (looks like the patch isn't done
yet ;)) and in this particular case the code is perfectly justified even
without talking about future functionality...

> +	err = vfs_statfs(path, &stat);
> +	if (err)
> +		return err;
> +
> +	if (!stat.f_fsid.val[0] && !stat.f_fsid.val[1])
> +		return -ENODEV;
> +
> +	/*
> +	 * Make sure path is not inside a filesystem subvolume (e.g. btrfs)
> +	 * which uses a different fsid than sb root.
> +	 */
> +	err = statfs_by_dentry(path->dentry->d_sb->s_root, &root_stat);
> +	if (err)
> +		return err;
> +
> +	if (root_stat.f_fsid.val[0] != stat.f_fsid.val[0] ||
> +	    root_stat.f_fsid.val[1] != stat.f_fsid.val[1])
> +		return -EXDEV;

I think inode watches in subvolumes are actually fine? The same fs object
is going to get different struct inode for different subvolumes if I
remember right. So there won't be any surprises with unexpected fsid being
reported.

Also mount watches are actually fine for subvolume as different subvolumes
appear as different mountpoints, don't they? And I think implementation
that would have different fsid for inodes in the same mountpoint would be
indeed very weird. So again no problem with fsid mismatch.

So we need this check only for superblock watches.

> diff --git a/fs/statfs.c b/fs/statfs.c
> index f0216629621d..6a5d840a2d8d 100644
> --- a/fs/statfs.c
> +++ b/fs/statfs.c
> @@ -50,7 +50,8 @@ static int calculate_f_flags(struct vfsmount *mnt)
>  		flags_by_sb(mnt->mnt_sb->s_flags);
>  }
>  
> -static int statfs_by_dentry(struct dentry *dentry, struct kstatfs *buf)
> +/* Does not set buf->f_flags */
> +int statfs_by_dentry(struct dentry *dentry, struct kstatfs *buf)
>  {
>  	int retval;
>  
> @@ -66,6 +67,7 @@ static int statfs_by_dentry(struct dentry *dentry, struct kstatfs *buf)
>  		buf->f_frsize = buf->f_bsize;
>  	return retval;
>  }
> +EXPORT_SYMBOL(statfs_by_dentry);
>  
>  int vfs_statfs(const struct path *path, struct kstatfs *buf)
>  {

Make this export a separate patch, CC Al Viro on it. Honestly I'm not very
happy about statfs_by_dentry() interface as it may return different result
than vfs_statfs() so it just waits for someone careless to use
statfs_by_dentry() and get burnt. How about implementing vfs_get_fsid()
that will get dentry and return fsid, that will be just internally
implemented using statfs_by_dentry()? We can then use that everywhere in
fsnotify and the interface is going to be much cleaner like that. The
comment regarding CC to Al Viro and separate patch still applies though.

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