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

[Jan Kara <jack@suse.cz>]

  Hi,

On Wed 24-06-15 00:30:16, Lino Sanfilippo wrote:
> 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.

We could do that as well. But I'd prefer to keep sending the IN_IGNORED
event from the context of the process destroying the mark (not that I would
be aware of any strong reason why that must happen but it just seems more
natural). Also the event from destruction thread will be sent with a delay
caused by synchronize_srcu(). Finally one long critical section for
destruction of all marks belonging to a group doesn't seem ideal either.

Anyway, I'll have this possibility in mind when implementing some solution.
Maybe it will be the most elegant way...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR