| Message ID | 1508920899-8115-8-git-send-email-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@redhat.com> wrote: > The only negative from this patch should be an addition of 32bytes to > 'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not > defined. > Thanks for this cleanup! See one question and coding style nits below. > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/notify/fanotify/fanotify.c | 30 +++------ > fs/notify/fanotify/fanotify.h | 8 ++- > fs/notify/fanotify/fanotify_user.c | 123 ++++++++++++++++++------------------- > include/linux/fsnotify_backend.h | 2 - > 4 files changed, 72 insertions(+), 91 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index df3f484e458a..63f56b007280 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -35,15 +35,13 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event) > > pr_debug("%s: list=%p event=%p\n", __func__, list, event); > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > /* > * Don't merge a permission event with any other event so that we know > * the event structure we have created in fanotify_handle_event() is the > * one we should check for permission response. > */ > - if (event->mask & FAN_ALL_PERM_EVENTS) > + if (fanotify_is_perm_event(event->mask)) > return 0; > -#endif > > list_for_each_entry_reverse(test_event, list, list) { > if (should_merge(test_event, event)) { > @@ -55,7 +53,6 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event) > return 0; > } > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > static int fanotify_get_response(struct fsnotify_group *group, > struct fanotify_perm_event_info *event, > struct fsnotify_iter_info *iter_info) > @@ -82,7 +79,6 @@ static int fanotify_get_response(struct fsnotify_group *group, > > return ret; > } > -#endif > > static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark, > struct fsnotify_mark *vfsmnt_mark, > @@ -141,8 +137,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask, > { > struct fanotify_event_info *event; > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - if (mask & FAN_ALL_PERM_EVENTS) { > + if (fanotify_is_perm_event(mask)) { > struct fanotify_perm_event_info *pevent; > > pevent = kmem_cache_alloc(fanotify_perm_event_cachep, > @@ -153,7 +148,6 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask, > pevent->response = 0; > goto init; > } > -#endif > event = kmem_cache_alloc(fanotify_event_cachep, GFP_KERNEL); > if (!event) > return NULL; > @@ -200,8 +194,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, > pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode, > mask); > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - if (mask & FAN_ALL_PERM_EVENTS) { > + if (fanotify_is_perm_event(mask)) { > /* > * fsnotify_prepare_user_wait() fails if we race with mark > * deletion. Just let the operation pass in that case. > @@ -209,7 +202,6 @@ static int fanotify_handle_event(struct fsnotify_group *group, > if (!fsnotify_prepare_user_wait(iter_info)) > return 0; > } > -#endif > > event = fanotify_alloc_event(inode, mask, data); > ret = -ENOMEM; > @@ -225,21 +217,15 @@ static int fanotify_handle_event(struct fsnotify_group *group, > fsnotify_destroy_event(group, fsn_event); > > ret = 0; > - goto finish; > - } > - > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - if (mask & FAN_ALL_PERM_EVENTS) { > + } else if (fanotify_is_perm_event(mask)) { > ret = fanotify_get_response(group, FANOTIFY_PE(fsn_event), > iter_info); > fsnotify_destroy_event(group, fsn_event); > } > finish: > - if (mask & FAN_ALL_PERM_EVENTS) > + if (fanotify_is_perm_event(mask)) > fsnotify_finish_user_wait(iter_info); > -#else > -finish: > -#endif Could have reduced messiness of path 5/7 if cleanup was done before - oh well > + > return ret; > } > > @@ -259,13 +245,11 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event) > event = FANOTIFY_E(fsn_event); > path_put(&event->path); > put_pid(event->tgid); > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - if (fsn_event->mask & FAN_ALL_PERM_EVENTS) { > + if (fanotify_is_perm_event(fsn_event->mask)) { > kmem_cache_free(fanotify_perm_event_cachep, > FANOTIFY_PE(fsn_event)); > return; > } > -#endif > kmem_cache_free(fanotify_event_cachep, event); > } > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index 4eb6f5efa282..dc219cf07a6a 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -21,7 +21,6 @@ struct fanotify_event_info { > struct pid *tgid; > }; > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > /* > * Structure for permission fanotify events. It gets allocated and freed in > * fanotify_handle_event() since we wait there for user response. When the > @@ -40,7 +39,12 @@ FANOTIFY_PE(struct fsnotify_event *fse) > { > return container_of(fse, struct fanotify_perm_event_info, fae.fse); > } > -#endif > + > +static inline bool fanotify_is_perm_event(u32 mask) > +{ > + return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) && > + mask & FAN_ALL_PERM_EVENTS; I know this is good w.r.t operation precedence, but it gets me eerie to see bit masking without (). maybe its just me. > +} > > static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse) > { > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 907a481ac781..44fd12aa17ff 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -142,7 +142,6 @@ static int fill_event_metadata(struct fsnotify_group *group, > return ret; > } > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > static struct fanotify_perm_event_info *dequeue_event( > struct fsnotify_group *group, int fd) > { > @@ -199,7 +198,6 @@ static int process_access_response(struct fsnotify_group *group, > > return 0; > } > -#endif > > static ssize_t copy_event_to_user(struct fsnotify_group *group, > struct fsnotify_event *event, > @@ -221,10 +219,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, > fanotify_event_metadata.event_len)) > goto out_close_fd; > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - if (event->mask & FAN_ALL_PERM_EVENTS) > + if (fanotify_is_perm_event(event->mask)) > FANOTIFY_PE(event)->fd = fd; > -#endif > > if (fd != FAN_NOFD) > fd_install(fd, f); > @@ -309,10 +305,9 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > * Permission events get queued to wait for response. Other > * events can be destroyed now. > */ > - if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) { > + if (!fanotify_is_perm_event(kevent->mask)) { > fsnotify_destroy_event(group, kevent); > } else { > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > if (ret <= 0) { > FANOTIFY_PE(kevent)->response = FAN_DENY; > wake_up(&group->fanotify_data.access_waitq); > @@ -322,7 +317,6 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > &group->fanotify_data.access_list); > spin_unlock(&group->notification_lock); > } > -#endif > } > if (ret < 0) > break; > @@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > > static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) > { > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > struct fanotify_response response = { .fd = -1, .response = -1 }; > struct fsnotify_group *group; > int ret; > > + if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) > + return -EINVAL; > + > + > group = file->private_data; > > if (count > sizeof(response)) > @@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t > count = ret; > > return count; > -#else > - return -EINVAL; > -#endif > } > > static int fanotify_release(struct inode *ignored, struct file *file) > { > struct fsnotify_group *group = file->private_data; > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - struct fanotify_perm_event_info *event, *next; > - struct fsnotify_event *fsn_event; > + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { > + struct fanotify_perm_event_info *event, *next; > + struct fsnotify_event *fsn_event; > > - /* > - * Stop new events from arriving in the notification queue. since > - * userspace cannot use fanotify fd anymore, no event can enter or > - * leave access_list by now either. > - */ > - fsnotify_group_stop_queueing(group); > + /* > + * Stop new events from arriving in the notification > + * queue. since userspace cannot use fanotify fd anymore, no > + * event can enter or leave access_list by now either. > + */ > + fsnotify_group_stop_queueing(group); > > - /* > - * Process all permission events on access_list and notification queue > - * and simulate reply from userspace. > - */ > - spin_lock(&group->notification_lock); > - list_for_each_entry_safe(event, next, &group->fanotify_data.access_list, > - fae.fse.list) { > - pr_debug("%s: found group=%p event=%p\n", __func__, group, > - event); > + /* > + * Process all permission events on access_list and notification > + * queue and simulate reply from userspace. > + */ > + spin_lock(&group->notification_lock); > + list_for_each_entry_safe(event, next, > + &group->fanotify_data.access_list, > + fae.fse.list) { > + pr_debug("%s: found group=%p event=%p\n", __func__, > + group, event); > + > + list_del_init(&event->fae.fse.list); > + event->response = FAN_ALLOW; > + } > > - list_del_init(&event->fae.fse.list); > - event->response = FAN_ALLOW; > - } > + /* > + * Destroy all non-permission events. For permission events just > + * dequeue them and set the response. They will be freed once > + * the response is consumed and fanotify_get_response() returns. > + */ > + while (!fsnotify_notify_queue_is_empty(group)) { > + fsn_event = fsnotify_remove_first_event(group); > + if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) { > + spin_unlock(&group->notification_lock); > + fsnotify_destroy_event(group, fsn_event); > + spin_lock(&group->notification_lock); > + } else > + FANOTIFY_PE(fsn_event)->response = FAN_ALLOW; Not your change, but if you post another version please add {} after else > + } > + spin_unlock(&group->notification_lock); > > - /* > - * Destroy all non-permission events. For permission events just > - * dequeue them and set the response. They will be freed once the > - * response is consumed and fanotify_get_response() returns. > - */ > - while (!fsnotify_notify_queue_is_empty(group)) { > - fsn_event = fsnotify_remove_first_event(group); > - if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) { > - spin_unlock(&group->notification_lock); > - fsnotify_destroy_event(group, fsn_event); > - spin_lock(&group->notification_lock); > - } else > - FANOTIFY_PE(fsn_event)->response = FAN_ALLOW; > + /* Response for all permission events it set, wakeup waiters */ > + wake_up(&group->fanotify_data.access_waitq); > } > - spin_unlock(&group->notification_lock); > - > - /* Response for all permission events it set, wakeup waiters */ > - wake_up(&group->fanotify_data.access_waitq); So I might as well learn something from this review - why did you move wake_up inside spinlock? Does it matter at all? Amir.
On Wed, Oct 25, 2017 at 1:44 PM, Amir Goldstein <amir73il@gmail.com> wrote: >> +static inline bool fanotify_is_perm_event(u32 mask) >> +{ >> + return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) && >> + mask & FAN_ALL_PERM_EVENTS; > > I know this is good w.r.t operation precedence, but it gets me eerie to see > bit masking without (). maybe its just me. Yeah, not easy to get used to, but here I don't think it hurts readability. >> + } else >> + FANOTIFY_PE(fsn_event)->response = FAN_ALLOW; > > Not your change, but if you post another version please add {} after else Okay. > >> + } >> + spin_unlock(&group->notification_lock); >> >> - /* >> - * Destroy all non-permission events. For permission events just >> - * dequeue them and set the response. They will be freed once the >> - * response is consumed and fanotify_get_response() returns. >> - */ >> - while (!fsnotify_notify_queue_is_empty(group)) { >> - fsn_event = fsnotify_remove_first_event(group); >> - if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) { >> - spin_unlock(&group->notification_lock); >> - fsnotify_destroy_event(group, fsn_event); >> - spin_lock(&group->notification_lock); >> - } else >> - FANOTIFY_PE(fsn_event)->response = FAN_ALLOW; >> + /* Response for all permission events it set, wakeup waiters */ >> + wake_up(&group->fanotify_data.access_waitq); >> } >> - spin_unlock(&group->notification_lock); >> - >> - /* Response for all permission events it set, wakeup waiters */ >> - wake_up(&group->fanotify_data.access_waitq); > > So I might as well learn something from this review - > why did you move wake_up inside spinlock? Does it matter at all? It would be strange if I did that. But I didn't, it's just that diff sometimes doesn't make it easy to read the (non) change. Thanks, Miklos
On Wed 25-10-17 10:41:39, Miklos Szeredi wrote: > The only negative from this patch should be an addition of 32bytes to > 'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not > defined. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> I like this but some comments below. > @@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > > static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) > { > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > struct fanotify_response response = { .fd = -1, .response = -1 }; > struct fsnotify_group *group; > int ret; > > + if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) > + return -EINVAL; > + > + Two empty lines here look superfluous. > @@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t > count = ret; > > return count; > -#else > - return -EINVAL; > -#endif > } > > static int fanotify_release(struct inode *ignored, struct file *file) > { > struct fsnotify_group *group = file->private_data; > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - struct fanotify_perm_event_info *event, *next; > - struct fsnotify_event *fsn_event; > + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { > + struct fanotify_perm_event_info *event, *next; > + struct fsnotify_event *fsn_event; Rather than doing this, I'd just let fanotify_release() go through the same path for both CONFIG_FANOTIFY_ACCESS_PERMISSIONS enabled and disabled. Enabled path won't be much more expensive since access_list will be empty and we have to walk & destroy events anyway. That way you also don't have to reindent everything. > @@ -768,10 +763,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) > if (force_o_largefile()) > event_f_flags |= O_LARGEFILE; > group->fanotify_data.f_flags = event_f_flags; > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - init_waitqueue_head(&group->fanotify_data.access_waitq); > - INIT_LIST_HEAD(&group->fanotify_data.access_list); > -#endif > + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { > + init_waitqueue_head(&group->fanotify_data.access_waitq); > + INIT_LIST_HEAD(&group->fanotify_data.access_list); > + } When having space for these allocated, just initialize them properly. Otherwise it's asking for trouble. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index df3f484e458a..63f56b007280 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -35,15 +35,13 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event) pr_debug("%s: list=%p event=%p\n", __func__, list, event); -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS /* * Don't merge a permission event with any other event so that we know * the event structure we have created in fanotify_handle_event() is the * one we should check for permission response. */ - if (event->mask & FAN_ALL_PERM_EVENTS) + if (fanotify_is_perm_event(event->mask)) return 0; -#endif list_for_each_entry_reverse(test_event, list, list) { if (should_merge(test_event, event)) { @@ -55,7 +53,6 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event) return 0; } -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS static int fanotify_get_response(struct fsnotify_group *group, struct fanotify_perm_event_info *event, struct fsnotify_iter_info *iter_info) @@ -82,7 +79,6 @@ static int fanotify_get_response(struct fsnotify_group *group, return ret; } -#endif static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark, struct fsnotify_mark *vfsmnt_mark, @@ -141,8 +137,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask, { struct fanotify_event_info *event; -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (mask & FAN_ALL_PERM_EVENTS) { + if (fanotify_is_perm_event(mask)) { struct fanotify_perm_event_info *pevent; pevent = kmem_cache_alloc(fanotify_perm_event_cachep, @@ -153,7 +148,6 @@ struct fanotify_event_info *fanotify_alloc_event(struct inode *inode, u32 mask, pevent->response = 0; goto init; } -#endif event = kmem_cache_alloc(fanotify_event_cachep, GFP_KERNEL); if (!event) return NULL; @@ -200,8 +194,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, pr_debug("%s: group=%p inode=%p mask=%x\n", __func__, group, inode, mask); -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (mask & FAN_ALL_PERM_EVENTS) { + if (fanotify_is_perm_event(mask)) { /* * fsnotify_prepare_user_wait() fails if we race with mark * deletion. Just let the operation pass in that case. @@ -209,7 +202,6 @@ static int fanotify_handle_event(struct fsnotify_group *group, if (!fsnotify_prepare_user_wait(iter_info)) return 0; } -#endif event = fanotify_alloc_event(inode, mask, data); ret = -ENOMEM; @@ -225,21 +217,15 @@ static int fanotify_handle_event(struct fsnotify_group *group, fsnotify_destroy_event(group, fsn_event); ret = 0; - goto finish; - } - -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (mask & FAN_ALL_PERM_EVENTS) { + } else if (fanotify_is_perm_event(mask)) { ret = fanotify_get_response(group, FANOTIFY_PE(fsn_event), iter_info); fsnotify_destroy_event(group, fsn_event); } finish: - if (mask & FAN_ALL_PERM_EVENTS) + if (fanotify_is_perm_event(mask)) fsnotify_finish_user_wait(iter_info); -#else -finish: -#endif + return ret; } @@ -259,13 +245,11 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event) event = FANOTIFY_E(fsn_event); path_put(&event->path); put_pid(event->tgid); -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (fsn_event->mask & FAN_ALL_PERM_EVENTS) { + if (fanotify_is_perm_event(fsn_event->mask)) { kmem_cache_free(fanotify_perm_event_cachep, FANOTIFY_PE(fsn_event)); return; } -#endif kmem_cache_free(fanotify_event_cachep, event); } diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 4eb6f5efa282..dc219cf07a6a 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -21,7 +21,6 @@ struct fanotify_event_info { struct pid *tgid; }; -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS /* * Structure for permission fanotify events. It gets allocated and freed in * fanotify_handle_event() since we wait there for user response. When the @@ -40,7 +39,12 @@ FANOTIFY_PE(struct fsnotify_event *fse) { return container_of(fse, struct fanotify_perm_event_info, fae.fse); } -#endif + +static inline bool fanotify_is_perm_event(u32 mask) +{ + return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) && + mask & FAN_ALL_PERM_EVENTS; +} static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse) { diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 907a481ac781..44fd12aa17ff 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -142,7 +142,6 @@ static int fill_event_metadata(struct fsnotify_group *group, return ret; } -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS static struct fanotify_perm_event_info *dequeue_event( struct fsnotify_group *group, int fd) { @@ -199,7 +198,6 @@ static int process_access_response(struct fsnotify_group *group, return 0; } -#endif static ssize_t copy_event_to_user(struct fsnotify_group *group, struct fsnotify_event *event, @@ -221,10 +219,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, fanotify_event_metadata.event_len)) goto out_close_fd; -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (event->mask & FAN_ALL_PERM_EVENTS) + if (fanotify_is_perm_event(event->mask)) FANOTIFY_PE(event)->fd = fd; -#endif if (fd != FAN_NOFD) fd_install(fd, f); @@ -309,10 +305,9 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, * Permission events get queued to wait for response. Other * events can be destroyed now. */ - if (!(kevent->mask & FAN_ALL_PERM_EVENTS)) { + if (!fanotify_is_perm_event(kevent->mask)) { fsnotify_destroy_event(group, kevent); } else { -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS if (ret <= 0) { FANOTIFY_PE(kevent)->response = FAN_DENY; wake_up(&group->fanotify_data.access_waitq); @@ -322,7 +317,6 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, &group->fanotify_data.access_list); spin_unlock(&group->notification_lock); } -#endif } if (ret < 0) break; @@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) { -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS struct fanotify_response response = { .fd = -1, .response = -1 }; struct fsnotify_group *group; int ret; + if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) + return -EINVAL; + + group = file->private_data; if (count > sizeof(response)) @@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count = ret; return count; -#else - return -EINVAL; -#endif } static int fanotify_release(struct inode *ignored, struct file *file) { struct fsnotify_group *group = file->private_data; -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - struct fanotify_perm_event_info *event, *next; - struct fsnotify_event *fsn_event; + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { + struct fanotify_perm_event_info *event, *next; + struct fsnotify_event *fsn_event; - /* - * Stop new events from arriving in the notification queue. since - * userspace cannot use fanotify fd anymore, no event can enter or - * leave access_list by now either. - */ - fsnotify_group_stop_queueing(group); + /* + * Stop new events from arriving in the notification + * queue. since userspace cannot use fanotify fd anymore, no + * event can enter or leave access_list by now either. + */ + fsnotify_group_stop_queueing(group); - /* - * Process all permission events on access_list and notification queue - * and simulate reply from userspace. - */ - spin_lock(&group->notification_lock); - list_for_each_entry_safe(event, next, &group->fanotify_data.access_list, - fae.fse.list) { - pr_debug("%s: found group=%p event=%p\n", __func__, group, - event); + /* + * Process all permission events on access_list and notification + * queue and simulate reply from userspace. + */ + spin_lock(&group->notification_lock); + list_for_each_entry_safe(event, next, + &group->fanotify_data.access_list, + fae.fse.list) { + pr_debug("%s: found group=%p event=%p\n", __func__, + group, event); + + list_del_init(&event->fae.fse.list); + event->response = FAN_ALLOW; + } - list_del_init(&event->fae.fse.list); - event->response = FAN_ALLOW; - } + /* + * Destroy all non-permission events. For permission events just + * dequeue them and set the response. They will be freed once + * the response is consumed and fanotify_get_response() returns. + */ + while (!fsnotify_notify_queue_is_empty(group)) { + fsn_event = fsnotify_remove_first_event(group); + if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) { + spin_unlock(&group->notification_lock); + fsnotify_destroy_event(group, fsn_event); + spin_lock(&group->notification_lock); + } else + FANOTIFY_PE(fsn_event)->response = FAN_ALLOW; + } + spin_unlock(&group->notification_lock); - /* - * Destroy all non-permission events. For permission events just - * dequeue them and set the response. They will be freed once the - * response is consumed and fanotify_get_response() returns. - */ - while (!fsnotify_notify_queue_is_empty(group)) { - fsn_event = fsnotify_remove_first_event(group); - if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) { - spin_unlock(&group->notification_lock); - fsnotify_destroy_event(group, fsn_event); - spin_lock(&group->notification_lock); - } else - FANOTIFY_PE(fsn_event)->response = FAN_ALLOW; + /* Response for all permission events it set, wakeup waiters */ + wake_up(&group->fanotify_data.access_waitq); } - spin_unlock(&group->notification_lock); - - /* Response for all permission events it set, wakeup waiters */ - wake_up(&group->fanotify_data.access_waitq); -#endif /* matches the fanotify_init->fsnotify_alloc_group */ fsnotify_destroy_group(group); @@ -768,10 +763,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) if (force_o_largefile()) event_f_flags |= O_LARGEFILE; group->fanotify_data.f_flags = event_f_flags; -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - init_waitqueue_head(&group->fanotify_data.access_waitq); - INIT_LIST_HEAD(&group->fanotify_data.access_list); -#endif + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { + init_waitqueue_head(&group->fanotify_data.access_waitq); + INIT_LIST_HEAD(&group->fanotify_data.access_list); + } switch (flags & FAN_ALL_CLASS_BITS) { case FAN_CLASS_NOTIF: group->priority = FS_PRIO_0; @@ -825,6 +820,7 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, struct fsnotify_group *group; struct fd f; struct path path; + u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD; int ret; pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n", @@ -855,11 +851,10 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags, mask &= ~FAN_ONDIR; } -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - if (mask & ~(FAN_ALL_EVENTS | FAN_ALL_PERM_EVENTS | FAN_EVENT_ON_CHILD)) -#else - if (mask & ~(FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD)) -#endif + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) + valid_mask |= FAN_ALL_PERM_EVENTS; + + if (mask & ~valid_mask) return -EINVAL; f = fdget(fanotify_fd); @@ -949,10 +944,10 @@ static int __init fanotify_user_setup(void) { fanotify_mark_cache = KMEM_CACHE(fsnotify_mark, SLAB_PANIC); fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC); -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS - fanotify_perm_event_cachep = KMEM_CACHE(fanotify_perm_event_info, - SLAB_PANIC); -#endif + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { + fanotify_perm_event_cachep = + KMEM_CACHE(fanotify_perm_event_info, SLAB_PANIC); + } return 0; } diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index c6c69318752b..eaea494683ab 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -182,11 +182,9 @@ struct fsnotify_group { #endif #ifdef CONFIG_FANOTIFY struct fanotify_group_private_data { -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS /* allows a group to block waiting for a userspace response */ struct list_head access_list; wait_queue_head_t access_waitq; -#endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */ int f_flags; unsigned int max_marks; struct user_struct *user;
The only negative from this patch should be an addition of 32bytes to 'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not defined. Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/notify/fanotify/fanotify.c | 30 +++------ fs/notify/fanotify/fanotify.h | 8 ++- fs/notify/fanotify/fanotify_user.c | 123 ++++++++++++++++++------------------- include/linux/fsnotify_backend.h | 2 - 4 files changed, 72 insertions(+), 91 deletions(-)