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

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 [off-list ref]
SUSE Labs, CR