On Thu, Jul 23, 2015 at 7:24 PM, Jan Kara <jack@xxxxxxxx> wrote:
> fsnotify_clear_marks_by_group_flags() can race with
> fsnotify_destroy_marks() so when fsnotify_destroy_mark_locked() drops
> mark_mutex, a mark from the list iterated by
> fsnotify_clear_marks_by_group_flags() can be freed and thus the next
> entry pointer we have cached may become stale and we dereference
> free memory.
>
> Fix the problem by first moving marks to free to a special private list
> and then always free the first entry in the special list. This method is
> safe even when entries from the list can disappear once we drop the lock.
>
> CC: stable@xxxxxxxxxxxxxxx
> Reported-by: Ashish Sangwan <a.sangwan@xxxxxxxxxxx>
> Signed-off-by: Jan Kara <jack@xxxxxxxx>
> ---
> fs/notify/mark.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> Andrew, this is the new version of the fsnotify oops fix. It has survived
> LTP tests and also a reproducer I wrote for triggering the oops. I'll work
> on integrating the reproducer in LTP inotify tests.
>
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 92e48c70f0f0..39ddcaf0918f 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -412,16 +412,36 @@ void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group,
> unsigned int flags)
> {
> struct fsnotify_mark *lmark, *mark;
> + LIST_HEAD(to_free);
>
> + /*
> + * We have to be really careful here. Anytime we drop mark_mutex, e.g.
> + * fsnotify_clear_marks_by_inode() can come and free marks. Even in our
> + * to_free list so we have to use mark_mutex even when accessing that
> + * list. And freeing mark requires us to drop mark_mutex. So we can
> + * reliably free only the first mark in the list. That's why we first
> + * move marks to free to to_free list in one go and then free marks in
> + * to_free list one by one.
> + */
> mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
> list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
> - if (mark->flags & flags) {
> - fsnotify_get_mark(mark);
> - fsnotify_destroy_mark_locked(mark, group);
> - fsnotify_put_mark(mark);
> - }
> + if (mark->flags & flags)
> + list_move(&mark->g_list, &to_free);
> }
> mutex_unlock(&group->mark_mutex);
> +
> + while (1) {
> + mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
Just a nitpick. Instead of locking/unlocking mutex multiple times in
the while loop,
can't we just keep the entire while loop inside the mutex_lock?
Overall, the patch seems ok to me.
Reviewed-by: Ashish Sangwan <a.sangwan@xxxxxxxxxxx>
> + if (list_empty(&to_free)) {
> + mutex_unlock(&group->mark_mutex);
> + break;
> + }
> + mark = list_first_entry(&to_free, struct fsnotify_mark, g_list);
> + fsnotify_get_mark(mark);
> + fsnotify_destroy_mark_locked(mark, group);
> + mutex_unlock(&group->mark_mutex);
> + fsnotify_put_mark(mark);
> + }
> }
>
> /*
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html