Re: [PATCH v2 7/7] fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs

[Amir Goldstein]

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.