From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:48636 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726565AbeK2UvZ (ORCPT ); Thu, 29 Nov 2018 15:51:25 -0500 Date: Thu, 29 Nov 2018 10:46:36 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org Subject: Re: [PATCH v3 08/13] fanotify: enable FAN_REPORT_FID init flag Message-ID: <20181129094636.GF31087@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-9-amir73il@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181125134352.21499-9-amir73il@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 SUSE Labs, CR