From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:34628 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727040AbeK2Wc5 (ORCPT ); Thu, 29 Nov 2018 17:32:57 -0500 Date: Thu, 29 Nov 2018 11:52:06 +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: <20181129105206.GK31087@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-9-amir73il@gmail.com> <20181129094636.GF31087@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181129094636.GF31087@quack2.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 29-11-18 10:46:36, Jan Kara wrote: > 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. See my reply to the next patch for more on this. Also I was thinking about the "no zero fsid" restriction. I understand where you are coming from but IMO FAN_REPORT_FID can be useful even if the filesystem doesn't support fsid - e.g. if you create a notification group and put there marks only for one filesystem, then you don't need the fsid part at all. I don't have a strong opinion on this (we can always enable this functionality later if we want) but wanted to run this by you. Honza -- Jan Kara SUSE Labs, CR