Re: [PATCH] fsnotify: fix a crash due to invalid virtual address

[Lino Sanfilippo <LinoSanfilippo@gmx.de>]

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