On Wed, Oct 25, 2017 at 11:41 AM, Miklos Szeredi <mszeredi@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
