fanotify: on group destroy allow all waiters to bypass permission check
When fanotify_release() is called, there may still be processes waiting for access permission. Currently only processes for which an event has already been queued into the groups access list will be woken up. Processes for which no event has been queued will continue to sleep and thus cause a deadlock when fsnotify_put_group() is called. Furthermore there is a race allowing further processes to be waiting on the access wait queue after wake_up (if they arrive before clear_marks_by_group() is called). This patch corrects this by setting a flag to inform processes that the group is about to be destroyed and thus not to wait for access permission. [additional changelog from eparis] Lets think about the 4 relevant code paths from the PoV of the 'operator' 'listener' 'responder' and 'closer'. Where operator is the process doing an action (like open/read) which could require permission. Listener is the task (or in this case thread) slated with reading from the fanotify file descriptor. The 'responder' is the thread responsible for responding to access requests. 'Closer' is the thread attempting to close the fanotify file descriptor. The 'operator' is going to end up in: fanotify_handle_event() get_response_from_access() (THIS BLOCKS WAITING ON USERSPACE) The 'listener' interesting code path fanotify_read() copy_event_to_user() prepare_for_access_response() (THIS CREATES AN fanotify_response_event) The 'responder' code path: fanotify_write() process_access_response() (REMOVE A fanotify_response_event, SET RESPONSE, WAKE UP 'operator') The 'closer': fanotify_release() (SUPPOSED TO CLEAN UP THE REST OF THIS MESS) What we have today is that in the closer we remove all of the fanotify_response_events and set a bit so no more response events are ever created in prepare_for_access_response(). The bug is that we never wake all of the operators up and tell them to move along. You fix that in fanotify_get_response_from_access(). You also fix other operators which haven't gotten there yet. So I agree that's a good fix. [/additional changelog from eparis] [remove additional changes to minimize patch size] [move initialization so it was inside CONFIG_FANOTIFY_PERMISSION] Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> Signed-off-by: Eric Paris <eparis@redhat.com>
This commit is contained in:
parent
1734dee4e3
commit
09e5f14e57
|
@ -92,7 +92,11 @@ static int fanotify_get_response_from_access(struct fsnotify_group *group,
|
|||
|
||||
pr_debug("%s: group=%p event=%p\n", __func__, group, event);
|
||||
|
||||
wait_event(group->fanotify_data.access_waitq, event->response);
|
||||
wait_event(group->fanotify_data.access_waitq, event->response ||
|
||||
atomic_read(&group->fanotify_data.bypass_perm));
|
||||
|
||||
if (!event->response) /* bypass_perm set */
|
||||
return 0;
|
||||
|
||||
/* userspace responded, convert to something usable */
|
||||
spin_lock(&event->lock);
|
||||
|
|
|
@ -200,7 +200,7 @@ static int prepare_for_access_response(struct fsnotify_group *group,
|
|||
|
||||
mutex_lock(&group->fanotify_data.access_mutex);
|
||||
|
||||
if (group->fanotify_data.bypass_perm) {
|
||||
if (atomic_read(&group->fanotify_data.bypass_perm)) {
|
||||
mutex_unlock(&group->fanotify_data.access_mutex);
|
||||
kmem_cache_free(fanotify_response_event_cache, re);
|
||||
event->response = FAN_ALLOW;
|
||||
|
@ -390,7 +390,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
|
|||
|
||||
mutex_lock(&group->fanotify_data.access_mutex);
|
||||
|
||||
group->fanotify_data.bypass_perm = true;
|
||||
atomic_inc(&group->fanotify_data.bypass_perm);
|
||||
|
||||
list_for_each_entry_safe(re, lre, &group->fanotify_data.access_list, list) {
|
||||
pr_debug("%s: found group=%p re=%p event=%p\n", __func__, group,
|
||||
|
@ -703,6 +703,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
|
|||
mutex_init(&group->fanotify_data.access_mutex);
|
||||
init_waitqueue_head(&group->fanotify_data.access_waitq);
|
||||
INIT_LIST_HEAD(&group->fanotify_data.access_list);
|
||||
atomic_set(&group->fanotify_data.bypass_perm, 0);
|
||||
#endif
|
||||
switch (flags & FAN_ALL_CLASS_BITS) {
|
||||
case FAN_CLASS_NOTIF:
|
||||
|
|
|
@ -166,7 +166,7 @@ struct fsnotify_group {
|
|||
struct mutex access_mutex;
|
||||
struct list_head access_list;
|
||||
wait_queue_head_t access_waitq;
|
||||
bool bypass_perm; /* protected by access_mutex */
|
||||
atomic_t bypass_perm;
|
||||
#endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
|
||||
int f_flags;
|
||||
unsigned int max_marks;
|
||||
|
|
Loading…
Reference in New Issue