Re: [PATCH 2/2] fsnotify: Let userspace know about lost events due to ENOMEM

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
> Currently if notification event is lost due to event allocation failing
> we ENOMEM, we just silently continue (except for fanotify permission
> events where we deny the access). This is undesirable as userspace has
> no way of knowing whether the notifications it got are complete or not.
> Treat lost events due to ENOMEM the same way as lost events due to queue
> overflow so that userspace knows something bad happened and it likely
> needs to rescan the filesystem.
>
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>


> ---
>  fs/notify/fanotify/fanotify.c        | 9 ++++++++-
>  fs/notify/inotify/inotify_fsnotify.c | 8 +++++++-
>  fs/notify/notification.c             | 3 ++-
>  include/linux/fsnotify_backend.h     | 6 ++++++
>  4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 928f2a5eedb7..d51e1bb781cf 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -221,8 +221,15 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>
>         event = fanotify_alloc_event(group, inode, mask, data);
>         ret = -ENOMEM;
> -       if (unlikely(!event))
> +       if (unlikely(!event)) {
> +               /*
> +                * We don't queue overflow events for permission events as
> +                * there the access is denied and so no event is in fact lost.
> +                */
> +               if (!fanotify_is_perm_event(mask))
> +                       fsnotify_queue_overflow(group);
>                 goto finish;
> +       }
>
>         fsn_event = &event->fse;
>         ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index 8b73332735ba..6440df8fb29a 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -99,8 +99,14 @@ int inotify_handle_event(struct fsnotify_group *group,
>                               fsn_mark);
>
>         event = kmalloc(alloc_len, GFP_KERNEL);
> -       if (unlikely(!event))
> +       if (unlikely(!event)) {
> +               /*
> +                * Treat lost event due to ENOMEM the same way as queue
> +                * overflow to let userspace know event was lost.
> +                */
> +               fsnotify_queue_overflow(group);
>                 return -ENOMEM;
> +       }
>
>         fsn_event = &event->fse;
>         fsnotify_init_event(fsn_event, inode, mask);
> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
> index 66f85c651c52..3c3e36745f59 100644
> --- a/fs/notify/notification.c
> +++ b/fs/notify/notification.c
> @@ -111,7 +111,8 @@ int fsnotify_add_event(struct fsnotify_group *group,
>                 return 2;
>         }
>
> -       if (group->q_len >= group->max_events) {
> +       if (event == group->overflow_event ||
> +           group->q_len >= group->max_events) {
>                 ret = 2;
>                 /* Queue overflow event only if it isn't already queued */
>                 if (!list_empty(&group->overflow_event->list)) {
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 067d52e95f02..9f1edb92c97e 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -331,6 +331,12 @@ extern int fsnotify_add_event(struct fsnotify_group *group,
>                               struct fsnotify_event *event,
>                               int (*merge)(struct list_head *,
>                                            struct fsnotify_event *));
> +/* Queue overflow event to a notification group */
> +static inline void fsnotify_queue_overflow(struct fsnotify_group *group)
> +{
> +       fsnotify_add_event(group, group->overflow_event, NULL);
> +}
> +
>  /* true if the group notification queue is empty */
>  extern bool fsnotify_notify_queue_is_empty(struct fsnotify_group *group);
>  /* return, but do not dequeue the first event on the notification queue */
> --
> 2.13.6
>