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

[Ashish Sangwan <a.sangwan@samsung.com>]

> 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.

Thanks for your help Jan. Looking forward to your patch(es).

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