From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:42556 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728458AbeK2CfR (ORCPT ); Wed, 28 Nov 2018 21:35:17 -0500 Date: Wed, 28 Nov 2018 16:33:12 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v3 05/13] fanotify: classify events that hold a file identifier Message-ID: <20181128153312.GJ15604@quack2.suse.cz> References: <20181125134352.21499-1-amir73il@gmail.com> <20181125134352.21499-6-amir73il@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181125134352.21499-6-amir73il@gmail.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sun 25-11-18 15:43:44, Amir Goldstein wrote: > With group flag FAN_REPORT_FID, we do not store event->path. > Instead, we will store the file handle and fsid in event->info.fid. > > Because event->info and event->path share the same union space, set > bit 0 of event->info.flags, so we know how to free and compare the > event objects. > > Signed-off-by: Amir Goldstein > --- > fs/notify/fanotify/fanotify.c | 35 ++++++++++++++++++++++++------ > fs/notify/fanotify/fanotify.h | 27 +++++++++++++++++++++++ > fs/notify/fanotify/fanotify_user.c | 3 +++ > 3 files changed, 58 insertions(+), 7 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 59d093923c97..8192d4b1db21 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -25,11 +25,16 @@ static bool should_merge(struct fsnotify_event *old_fsn, > old = FANOTIFY_E(old_fsn); > new = FANOTIFY_E(new_fsn); > > - if (old_fsn->inode == new_fsn->inode && old->pid == new->pid && > - old->path.mnt == new->path.mnt && > - old->path.dentry == new->path.dentry) > - return true; > - return false; > + if (old_fsn->inode != new_fsn->inode || old->pid != new->pid) > + return false; > + > + if (FANOTIFY_HAS_FID(new)) { > + return FANOTIFY_HAS_FID(old) && > + fanotify_fid_equal(old->info.fid, new->info.fid); > + } else { > + return old->path.mnt == new->path.mnt && > + old->path.dentry == new->path.dentry; > + } > } > > /* and the list better be locked by something too! */ > @@ -178,7 +183,10 @@ init: __maybe_unused > event->pid = get_pid(task_pid(current)); > else > event->pid = get_pid(task_tgid(current)); > - if (path && !FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > + /* TODO: allocate buffer and encode file handle */ > + fanotify_set_fid(event, NULL); > + } else if (path) { > event->path = *path; > path_get(&event->path); > } else { > @@ -277,8 +285,21 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event) > { > struct fanotify_event *event; > > + /* > + * event->path and event->info are a union. Make sure that > + * event->info.flags overlaps with path.dentry pointer, so it is safe > + * to use bit 0 to classify the event info type (FANOTIFY_HAS_FID). > + */ > + BUILD_BUG_ON(offsetof(struct fanotify_event, path.dentry) != > + offsetof(struct fanotify_event, info.flags)); > + BUILD_BUG_ON(!__builtin_types_compatible_p(typeof(event->info.flags), > + unsigned long)); > + > event = FANOTIFY_E(fsn_event); > - path_put(&event->path); > + if (FANOTIFY_HAS_FID(event)) > + kfree(event->info.fid); > + else > + path_put(&event->path); > put_pid(event->pid); > if (fanotify_is_perm_event(fsn_event->mask)) { > kmem_cache_free(fanotify_perm_event_cachep, > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index 2e4fca30afda..a79dcbd41702 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -13,8 +13,20 @@ extern struct kmem_cache *fanotify_perm_event_cachep; > > struct fanotify_info { > struct fanotify_event_fid *fid; > + unsigned long flags; > }; > > +static inline bool fanotify_fid_equal(const struct fanotify_event_fid *fid1, > + const struct fanotify_event_fid *fid2) > +{ > + if (fid1 == fid2) > + return true; > + > + return fid1 && fid2 && fid1->handle_bytes == fid2->handle_bytes && > + !memcmp((void *)fid1, (void *)fid2, > + FANOTIFY_FID_LEN(fid1->handle_bytes)); > +} > + > /* > * Structure for normal fanotify events. It gets allocated in > * fanotify_handle_event() and freed when the information is retrieved by > @@ -38,6 +50,21 @@ struct fanotify_event { > struct pid *pid; > }; > > +/* > + * Bit 0 of info.flags is set when event has fid information. > + * event->info shares the same union address with event->path, so this helps > + * us tell if event has fid or path. > + */ > +#define __FANOTIFY_FID 1UL > +#define FANOTIFY_HAS_FID(event) ((event)->info.flags & __FANOTIFY_FID) Why isn't this an inline function? And then the name wouldn't have to be all caps... Also > +static inline void fanotify_set_fid(struct fanotify_event *event, > + struct fanotify_event_fid *fid) > +{ > + event->info.fid = fid; > + event->info.flags = fid ? __FANOTIFY_FID : 0; > +} > + > /* > * Structure for permission fanotify events. It gets allocated and freed in > * fanotify_handle_event() since we wait there for user response. When the > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 93e1aa2a389f..47e8bf3bcd28 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -81,6 +81,9 @@ static int create_fd(struct fsnotify_group *group, > > pr_debug("%s: group=%p event=%p\n", __func__, group, event); > > + if (WARN_ON_ONCE(FANOTIFY_HAS_FID(event))) > + return -EBADF; > + > client_fd = get_unused_fd_flags(group->fanotify_data.f_flags); > if (client_fd < 0) > return client_fd; > -- > 2.17.1 > -- Jan Kara SUSE Labs, CR