[PATCH] fsnotify: fix false positive warning on inode delete

[PATCH] fsnotify: fix false positive warning on inode delete

[Jan Kara]

When inode is getting deleted and someone else holds reference to a mark
attached to the inode, we just detach the connector from the inode. In
that case fsnotify_put_mark() called from fsnotify_destroy_marks() will
decide to recalculate mask for the inode and __fsnotify_recalc_mask()
will WARN about invalid connector type:

WARNING: CPU: 1 PID: 12015 at fs/notify/mark.c:139
__fsnotify_recalc_mask+0x2d7/0x350 fs/notify/mark.c:139

Actually there's no reason to warn about detached connector in
__fsnotify_recalc_mask() so just silently skip updating the mask in such
case.

Reported-by: syzbot+c34692a51b9a6ca93540@syzkaller.appspotmail.com
Fixes: 3ac70bfcde81 ("fsnotify: add helper to get mask from connector")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/mark.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

I plan to merge this fix through my tree for 4.19-rc2.

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 05506d60131c..59cdb27826de 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -132,13 +132,13 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	struct fsnotify_mark *mark;
 
 	assert_spin_locked(&conn->lock);
+	/* We can get detached connector here when inode is getting unlinked. */
+	if (!fsnotify_valid_obj_type(conn->type))
+		return;
 	hlist_for_each_entry(mark, &conn->list, obj_list) {
 		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
 			new_mask |= mark->mask;
 	}
-	if (WARN_ON(!fsnotify_valid_obj_type(conn->type)))
-		return;
-
 	*fsnotify_conn_mask_p(conn) = new_mask;
 }
 
-- 
2.16.4

Re: [PATCH] fsnotify: fix false positive warning on inode delete

[Amir Goldstein]

On Mon, Aug 20, 2018 at 3:32 PM Jan Kara <jack@suse.cz> wrote:
>
> When inode is getting deleted and someone else holds reference to a mark
> attached to the inode, we just detach the connector from the inode. In
> that case fsnotify_put_mark() called from fsnotify_destroy_marks() will
> decide to recalculate mask for the inode and __fsnotify_recalc_mask()
> will WARN about invalid connector type:
>
> WARNING: CPU: 1 PID: 12015 at fs/notify/mark.c:139
> __fsnotify_recalc_mask+0x2d7/0x350 fs/notify/mark.c:139
>
> Actually there's no reason to warn about detached connector in
> __fsnotify_recalc_mask() so just silently skip updating the mask in such
> case.
>
> Reported-by: syzbot+c34692a51b9a6ca93540@syzkaller.appspotmail.com
> Fixes: 3ac70bfcde81 ("fsnotify: add helper to get mask from connector")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/mark.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> I plan to merge this fix through my tree for 4.19-rc2.
>

looks good.
Thanks.

> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 05506d60131c..59cdb27826de 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -132,13 +132,13 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
>         struct fsnotify_mark *mark;
>
>         assert_spin_locked(&conn->lock);
> +       /* We can get detached connector here when inode is getting unlinked. */
> +       if (!fsnotify_valid_obj_type(conn->type))
> +               return;
>         hlist_for_each_entry(mark, &conn->list, obj_list) {
>                 if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
>                         new_mask |= mark->mask;
>         }
> -       if (WARN_ON(!fsnotify_valid_obj_type(conn->type)))
> -               return;
> -
>         *fsnotify_conn_mask_p(conn) = new_mask;
>  }
>
> --
> 2.16.4
>

[PATCH] fsnotify: fix false positive warning on inode delete

[Amir Goldstein]

Reported-and-tested-by: syzbot+c34692a51b9a6ca93540@syzkaller.appspotmail.com
Fixes: 3ac70bfcde81 ("fsnotify: add helper to get mask from connector")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Jan,

syzbot reported (in private email) that the reproducer did not trigger
the warning, so added tested-by.

Thanks,
Amir.

 fs/notify/mark.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 05506d60131c..d559a8ffe7ed 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -236,6 +236,13 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
 	if (hlist_empty(&conn->list)) {
 		inode = fsnotify_detach_connector_from_object(conn);
 		free_conn = true;
+	} else if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) {
+		/*
+		 * fsnotify_destroy_marks() detaches conn from object before
+		 * put on last mark of object list and other marks on the list
+		 * may still have elevated refcounts. We don't need to recalc
+		 * mask nor to free_conn in that case.
+		 */
 	} else {
 		__fsnotify_recalc_mask(conn);
 	}
-- 
2.7.4

Re: [PATCH] fsnotify: fix false positive warning on inode delete

[Jan Kara]

On Sun 19-08-18 15:35:06, Amir Goldstein wrote:
> Reported-and-tested-by: syzbot+c34692a51b9a6ca93540@syzkaller.appspotmail.com
> Fixes: 3ac70bfcde81 ("fsnotify: add helper to get mask from connector")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Jan,
> 
> syzbot reported (in private email) that the reproducer did not trigger
> the warning, so added tested-by.

Thanks for looking into this Amir! I was thinking about this for a while
and I'm not sure that __fsnotify_recalc_mask() call from
fsnotify_put_mark() is the only place calling __fsnotify_recalc_mask() that
can happen on detached connector. unlink(2) can get called pretty much at
any time so places like inotify_update_existing_watch() can easily work on
inode that is getting unlinked and by the time we get to
fsnotify_recalc_mask(), we can pass detached connector to it AFAICT.
conn->lock we hold in __fsnotify_recalc_mask() protecs us from
fsnotify_detach_connector_from_object() so we can reliably check connector
state in __fsnotify_recalc_mask() and just don't do anything when the
connector is already detached without issuing a warning. What do you think?

								Honza

>  fs/notify/mark.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 05506d60131c..d559a8ffe7ed 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -236,6 +236,13 @@ void fsnotify_put_mark(struct fsnotify_mark *mark)
>  	if (hlist_empty(&conn->list)) {
>  		inode = fsnotify_detach_connector_from_object(conn);
>  		free_conn = true;
> +	} else if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) {
> +		/*
> +		 * fsnotify_destroy_marks() detaches conn from object before
> +		 * put on last mark of object list and other marks on the list
> +		 * may still have elevated refcounts. We don't need to recalc
> +		 * mask nor to free_conn in that case.
> +		 */
>  	} else {
>  		__fsnotify_recalc_mask(conn);
>  	}
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fsnotify: fix false positive warning on inode delete

[Amir Goldstein]

On Mon, Aug 20, 2018 at 1:07 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 19-08-18 15:35:06, Amir Goldstein wrote:
> > Reported-and-tested-by: syzbot+c34692a51b9a6ca93540@syzkaller.appspotmail.com
> > Fixes: 3ac70bfcde81 ("fsnotify: add helper to get mask from connector")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Jan,
> >
> > syzbot reported (in private email) that the reproducer did not trigger
> > the warning, so added tested-by.
>
> Thanks for looking into this Amir! I was thinking about this for a while
> and I'm not sure that __fsnotify_recalc_mask() call from
> fsnotify_put_mark() is the only place calling __fsnotify_recalc_mask() that
> can happen on detached connector. unlink(2) can get called pretty much at
> any time so places like inotify_update_existing_watch() can easily work on
> inode that is getting unlinked and by the time we get to
> fsnotify_recalc_mask(), we can pass detached connector to it AFAICT.
> conn->lock we hold in __fsnotify_recalc_mask() protecs us from
> fsnotify_detach_connector_from_object() so we can reliably check connector
> state in __fsnotify_recalc_mask() and just don't do anything when the
> connector is already detached without issuing a warning. What do you think?
>

Makes sense. WARN_ON() is a new addition by cleanup patches
and the fact that disconnected state is valid in __fsnotify_recalc_mask()
was an oversight.

Feel free to apply the simpler fix with reported-by attribution to syzbot ;-)

Thanks,
Amir.