| Message ID | 20181125134352.21499-6-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: add support for more event types | expand |
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 <amir73il@gmail.com> > --- > 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 >
Sorry, sent previous reply too early. On Wed 28-11-18 16:33:12, Jan Kara wrote: > @@ -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 I'd prefer if we just enlarged struct fanotify_event by 8 bytes to allow simple 'flags' field and info about possible embedded fid, rather than playing these bit tricks. They save space but make code more subtle and I don't file struct fanotify_event size so critical. Honza
On Wed, Nov 28, 2018 at 5:44 PM Jan Kara <jack@suse.cz> wrote: > > Sorry, sent previous reply too early. > > On Wed 28-11-18 16:33:12, Jan Kara wrote: > > @@ -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... Sure. > > Also I'd prefer if we just enlarged struct fanotify_event by 8 bytes to > allow simple 'flags' field and info about possible embedded fid, rather > than playing these bit tricks. They save space but make code more subtle > and I don't file struct fanotify_event size so critical. > Ok. Freeing the space of path.dentry and path.mnt will give us 16 bytes of space for embedded fid, which should be sufficient for many file handles (ext4, xfs). and flags could indicate if fid is embedded or allocated. Thanks, Amir.
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) + +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;
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 <amir73il@gmail.com> --- 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(-)