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

* Re: Re: [PATCH] fsnotify: fix a crash due to invalid virtual address
@ 2015-06-23 12:05 Ashish Sangwan
  2015-06-23 14:16 ` Jan Kara
  0 siblings, 1 reply; 2+ messages in thread
From: Ashish Sangwan @ 2015-06-23 12:05 UTC (permalink / raw)
  To: Jan Kara
  Cc: linux-fsdevel, Andrew Morton, Eric Paris, AMIT SAHRAWAT,
	Namjae Jeon, PANKAJ MISHRA, Lino Sanfilippo

> 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.
Right. inotify_handle_event() can recursively call fsnotify_destroy_mark()
when IN_ONESHOT is set. I missed it, thanks for pointing out.
> 
> Looking into this in more detail, it might be worthwhile to revisit how
> mark_mutex is used since at least fanotify and dnotify use it for more than
> just a protection of list of group marks and untangling this would simplify
> things. But that's a longer term goal.
> 
> A relatively simple fix for your issue is to split group list of marks into
> a list of inode marks and a list of mount marks. Then destroying becomes
> much simpler because we always discard the whole list (or both of them) and
> we can easily avoid problems with list corruption when dropping the
> mark_mutex. I can write the patch later or you can do that if you are
Sorry I could not understand why the group's list of marks needs to be split.
I was browsing through the old code, from the days mark_mutex was not present
and it looked like below:
void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group,
					 unsigned int flags)
{
	struct fsnotify_mark *lmark, *mark;
	LIST_HEAD(free_list);

	spin_lock(&group->mark_lock);
	list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
		if (mark->flags & flags) {
			list_add(&mark->free_g_list, &free_list);
			list_del_init(&mark->g_list);
			fsnotify_get_mark(mark);
		}
	}
	spin_unlock(&group->mark_lock);

	list_for_each_entry_safe(mark, lmark, &free_list, free_g_list) {
		fsnotify_destroy_mark(mark);
		fsnotify_put_mark(mark);
	}
}
How about using a temporary onstack list_head like above?

> interested.
> 
> 					Honza
> 
> > 
> > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
> > Reviewed-by: Amit Sahrawat <a.sahrawat@samsung.com>
> > ---
> >  fs/notify/mark.c |    4 ----
> >  1 files changed, 0 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> > index 92e48c7..4ee419f 100755
> > --- a/fs/notify/mark.c
> > +++ b/fs/notify/mark.c
> > @@ -157,8 +157,6 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark,
> >  
> >  	if (inode && (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED))
> >  		iput(inode);
> > -	/* release lock temporarily */
> > -	mutex_unlock(&group->mark_mutex);
> >  
> >  	spin_lock(&destroy_lock);
> >  	list_add(&mark->g_list, &destroy_list);
> > @@ -191,8 +189,6 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark,
> >  	 */
> >  
> >  	atomic_dec(&group->num_marks);
> > -
> > -	mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
> >  }
> >  
> >  void fsnotify_destroy_mark(struct fsnotify_mark *mark,
> > -- 
> > 1.7.7
> > 
> > -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR

^ permalink raw reply	[flat|nested] 2+ messages in thread