Hi, On 23.06.2015 12:25, Jan Kara wrote: > On Mon 22-06-15 16:23:16, Ashish Sangwan wrote: >> For deleting the fsnotify_mark related with an inode, there are 2 paths in the >> kernel. When the inotify fd is closed, all the marks belonging to a group are >> removed one by one in fsnotify_clear_marks_by_group_flags. Other path is when >> the inode is removed from user space by unlink, fsnotify_destroy_mark is >> called to delete a single mark. >> There is a race between these 2 paths which is caused due to the temporary >> release of the mark_mutex inside fsnotify_destroy_mark_locked. >> The race happen when the inotify app monitoring the file(s) exits, triggering >> fsnotify_clear_marks_by_group_flags to delete the marks. >> This function use lmark pointer to move to the next node after a safe removal >> of the node. In parallel, if there is rm call for a file and such that the >> lmark is pointing to the mark which is removed by this rm call, lmark ends up >> pointing to a freed memory. Now, when we try to move to the next node using >> lmark, it triggers an invalid virtual address crash. >> Although fsnotify_clear_marks_by_group_flags and fsnotify_destroy_mark are >> synchronized by mark_mutex, but both of these functions call >> fsnotify_destroy_mark_locked which release the mark_mutex and acquire it again >> creating a subtle race window. There seems to be no reason for releasing >> mark_mutex, so this patch remove the mutex_unlock call. > > Thanks for report and the analysis. I agree with your problem analysis. > Indeed the loop in fsnotify_clear_marks_by_group_flags() isn't safe against > us dropping the mark_mutex inside fsnotify_destroy_mark_locked(). However > mark_mutex is dropped in fsnotify_destroy_mark_locked() for a purpose. We > call ->freeing_mark() callback from there and that should be called without > mark_mutex. In particular inotify uses this callback to send the IN_IGNORE > event and that code certainly isn't prepared to be called under mark_mutex > and you likely introduce interesting deadlock possibilities there. > Why dont we call freeing_mark() from the "fsnotify_mark"-thread instead of fsnotify_destroy_mark_locked()? So there would not be a reason for this temporary unlock any longer and we could close that race as Ashish suggested. 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
