Re: [PATCH] fanotify: fix race between fanotify_release() and fanotify_get_response()

[Jan Kara <jack@suse.cz>]

On Wed 24-08-16 11:55:39, Miklos Szeredi wrote:
> List corruption was reported with a fanotify stress test.
> 
> The bug turned out to be due to fsnotify_remove_event() being called on an
> event on the fanotify_data.access_list and protected by
> fanotify_data.access_lock instead of notification_mutex.  This resulted in
> list_del_init() being run concurrently on the same list entry.
> 
> This was introduced by commit 09e5f14e57c7 ("fanotify: on group destroy
> allow all waiters to bypass permission check") which made
> fanotify_get_response() flush out events when bypass_perm was set.  The
> flush doesn't normally happen, since the wake_up() is called after the
> access_list was cleaned in fsnotify_release().  But the two are not
> synchronized, the fanotify_get_response() could still be processing a
> previous wakeup by the time bypass_perm is true.  This was seen in the
> crashdumps in the report.

Thanks for the analysis and the patch! I agree there's a bug you describe,
I just somewhat disagree with the solution.

> This bug can be solved multiple ways, maybe the simplest is moving the
> bypass_perm setting after the list has been processed.
> 
> In theory there's also a memory ordering problem here.  atomic_inc() in
> itself doesn't imply a memory barrier, and spin_unlock() is a semi
> permeable barrier, so we need an explicit memory barrier so that the
> condition is precieved after the list is cleared.

Well, the culprit of the problem seems to be that fanotify_get_response()
does not use proper function to remove the event from the list in all the
cases. The event can be in two states:

1) User didn't yet read the even at the time fanotify_release() is closed -
note that a group can still receive new events queued while
fanotify_release() is running until fsnotify_destroy_group() kills all the
marks and that is the main reason why we have that bypass_perm thing to
avoid blocking new processes in fanotify_get_response() because they could
otherwise miss a wakeup and hang there indefinitely. In this state calling
fsnotify_remove_event() is correct and that's what commit 5838d4442bd5
(fanotify: fix double free of pending permission events) had in mind.

2) User has read the event (thus the permission even was moved to
access_list) but didn't write the response yet. In this state calling
fsnotify_remove_event() from fanotify_get_response() is just wrong - as you
noted it uses the wrong lock to protect the list removal but it also
wrongly updates group->q_len. In this situation we should be calling
dequeue_event().

The problem is that there's no easy way to distinguishing these two cases
in fanotify_get_response(). We could flag this somehow inside the event
structure but I think it's cleaner to remove all permission events in
fanotify_release() (the same way we already handle the permission events on
access_list). I'll send a patch shortly.

> Similarly we need barriers for the case when event->response is set
> (i.e. non zero): fsnotify_destroy_event() might destroy the event while
> it's still on the access_list, since nothing guarantees that the storing
> the response value in event->response will be preceived after the list
> manipulation.  So add the necessary barriers there as well.
> 
> PS not sure why bypass_perm is an atomic_t, it could just as well be a
> boolean flag.

Agreed. I was just never bothered enough to fix this.

> PPS all this subtlety could be removed if the waitq was per-event, which
> would also allow better performance.

I'm not sure how much this problem would be helped by a per-event
waitqueue.  Also it would add a significant memory cost to permission
events and I prefer them small as there can be a lot of them queued...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR