| Message ID | 1534682106-26538-1-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify: fix false positive warning on inode delete | expand |
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 >
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.
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); }
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(+)