Hi,
On 08.09.2016 16:25, Jan Kara wrote:
> fanotify_get_response() calls fsnotify_remove_event() when it finds that
> group is being released from fanotify_release() (bypass_perm is set).
> However the event it removes need not be only in the group's notification
> queue but it can have already moved to access_list (userspace read the
> event before closing the fanotify instance fd) which is protected by a
> different lock. Thus when fsnotify_remove_event() races with
> fanotify_release() operating on access_list, the list can get corrupted.
>
> Fix the problem by moving all the logic removing permission events from
> the lists to one place - fanotify_release().
>
> Reported-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
I think all in all this patchset implements a sane solution. Nevertheless
some comments/questions concerning the new code in fanotify_release below:
> @@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> spin_unlock(&group->fanotify_data.access_lock);
>
> /*
> - * Since bypass_perm is set, newly queued events will not wait for
> - * access response. Wake up the already sleeping ones now.
> - * synchronize_srcu() in fsnotify_destroy_group() will wait for all
> - * processes sleeping in fanotify_handle_event() waiting for access
> - * response and thus also for all permission events to be freed.
> + * 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.
> */
> + mutex_lock(&group->notification_mutex);
> + while (!fsnotify_notify_queue_is_empty(group)) {
> + fsn_event = fsnotify_remove_first_event(group);
> + if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
> + fsnotify_destroy_event(group, fsn_event);
> + else
> + FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
Why do we have to handle (i.e destroy) non-permission events here? Is this
meant as some kind of optimization?
Also, we now set the response during both iterations. I assume the assignment during
the iteration of the access list is only a leftover and can be skipped.
Why dont we set the shutdown flag directly in release() as soon as we grep
the notification mutex. I dont see any reason to do this seperated from
the removal of events from the notification list (or do I miss something?).
Similar situation in destroy_group(): We can set the flag in flush_notify instead
of destroy_group (we then do not even need the dedicated function stop_queueing() then).
fsnotify_remove_event() is not used any more now, so we should remove this function.
Regards,
Lino
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
