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

[Jan Kara <jack@suse.cz>]

  Hello,

On Wed 24-06-15 10:13:50, Ashish Sangwan wrote:
> > On Tue 23-06-15 12:05:51, Ashish Sangwan wrote:
> > > > 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?
> > 
> > So we can use a temporary list_head for entries selected for destruction as
> > well. *But* in either case you have to be careful because even the
> > temporary free_list can be modified by concurrent mark destruction e.g. from
> > fsnotify_free_marks_by_inode() call as each mark is in two lists - one
> > hanging from inode / mount and one hanging from the notification group.
> True.
> > 
> > Actually, looking into it even fsnotify_destroy_marks() doesn't seem to be
> > properly protected from the race. Sigh.
> True.
> 
> So anything which is calling fsnotify_destroy_mark is not protected against
> concurrent deletion from fsnotify_clear_marks_by_group_flags() and vice versa.
> Plus, as you rightly pointed, we have both the inode mark and vfsmount sharing the same list.
> So even if fsnotify_clear_marks_by_group_flags is for removing inode mark, a
> parallel fsnotify_destroy_mark for vfsmount can cause crash as they are sharing
> same list.

So I was looking more into the issue today and fsnotify_destroy_marks() is
fine in the end since it holds a reference to each mark (so it cannot be
freed) and it also uses a special free_list list_head within mark for its
temporary list so nobody removes any mark from that list.

> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index d90deaa..d83ec7d 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -124,14 +124,6 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark,
> 
>         spin_lock(&mark->lock);
> 
> -       /* something else already called this function on this mark */
> -       if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE)) {
> -               spin_unlock(&mark->lock);
> -               return;
> -       }
> -
> -       mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
> -
>         if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
>                 inode = mark->i.inode;
>                 fsnotify_destroy_inode_mark(mark);
> @@ -188,7 +180,10 @@ void fsnotify_destroy_mark(struct fsnotify_mark *mark,
>                            struct fsnotify_group *group)
>  {
>         mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
> -       fsnotify_destroy_mark_locked(mark, group);
> +       if (mark->flags & FSNOTIFY_MARK_FLAG_ALIVE) {
> +               mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
> +               fsnotify_destroy_mark_locked(mark, group);
> +       }
>         mutex_unlock(&group->mark_mutex);
>  }
> 
> @@ -293,14 +288,27 @@ void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group,
>                                          unsigned int flags)
>  {
>         struct fsnotify_mark *lmark, *mark;
> -
> +       LIST_HEAD(free_list);
> +
>         mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
> -       list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
> +       /* Pass 1 : clear the alive flag and move to free list */
> +       list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list)
>                 if (mark->flags & flags) {
> +                       /*
> +                        * If the mark is present on group's mark list
> +                        * it has to be alive.
> +                        */
> +                       WARN_ON(!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE));
> +                       list_del_init(&mark->g_list);
> +                       list_add(&mark->g_list, &free_list);
> +                       mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
> +               }
> +
> +       /* Pass 2: remove mark */
> +       list_for_each_entry_safe(mark, lmark, &free_list, g_list) {
>                         fsnotify_get_mark(mark);
>                         fsnotify_destroy_mark_locked(mark, group);
>                         fsnotify_put_mark(mark);
> -               }
>         }
>         mutex_unlock(&group->mark_mutex);
>  }

So this should fix the problem as well. However the lifetime rules are so
convoluted that I'm thinking how to make things simpler (especially to
unify how clearing via inode/mount and via group works). When investigating
the code I also found out there are possible issues with mark->inode
getting cleared while we are delivering some event. So I'm looking into
fixing all the problems.

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