On Tue, Jan 8, 2019 at 6:46 PM Jan Kara <jack@xxxxxxx> wrote:
>
> When waiting for response to fanotify permission events, we currently
> use uninterruptible waits. That makes code simple however it can cause
> lots of processes to end up in uninterruptible sleep with hard reboot
> being the only alternative in case fanotify listener process stops
> responding (e.g. due to a bug in its implementation). Uninterruptible
> sleep also makes system hibernation fail if the listener gets frozen
> before the process generating fanotify permission event.
>
> Fix these problems by using interruptible sleep for waiting for response
> to fanotify event. This is slightly tricky though - we have to
> detect when the event got already reported to userspace as in that
> case we must not free the event.
> Instead we push the responsibility for
> freeing the event to the process that will write response to the
> event.
It a bit hard to follow this patch. Can we make the patch that
shifts responsibility to free the event a separate patch?
fsnotify_remove_queued_event() helper can tag along with it
or separate patch as you wish.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
> ---
I think it would be good to add reported-by tand the links you provided
in cover letter in this patch as well.
...
>
> @@ -87,8 +116,8 @@ static int fanotify_get_response(struct fsnotify_group *group,
> if (response & FAN_AUDIT)
> audit_fanotify(response & ~FAN_AUDIT);
>
> - pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
> - group, event, ret);
Looks useful. Why remove?
...
>
> +static void set_event_response(struct fsnotify_group *group,
> + struct fanotify_perm_event_info *event,
> + unsigned int response)
> +{
> + spin_lock(&group->notification_lock);
> + /* Waiter got aborted by a signal? Free the event. */
> + if (unlikely(event->response == FAN_EVENT_CANCELED)) {
> + spin_unlock(&group->notification_lock);
> + fsnotify_destroy_event(group, &event->fae.fse);
> + return;
> + }
> + event->response = response | FAN_EVENT_ANSWERED;
> + spin_unlock(&group->notification_lock);
> +}
> +
I don't understand. AFAICS, all callers of set_event_response()
call immediately after spin_unlock(&group->notification_lock),
without any user wait involved.
I think it makes more sense for set_event_response() to assert the
lock than it is to take it.
Am I missing anything?
Thanks,
Amir.
