On Fri 19-04-19 12:33:02, Amir Goldstein wrote: > On Thu, Apr 18, 2019 at 8:14 PM syzbot > <syzbot+15927486a4f1bfcbaf91@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote: > > > > syzbot has bisected this bug to: > > > > commit 77115225acc67d9ac4b15f04dd138006b9cd1ef2 > > Author: Amir Goldstein <amir73il@xxxxxxxxx> > > Date: Thu Jan 10 17:04:37 2019 +0000 > > > > fanotify: cache fsid in fsnotify_mark_connector > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1627632d200000 > > start commit: 3f018f4a Add linux-next specific files for 20190418 > > git tree: linux-next > > final crash: https://syzkaller.appspot.com/x/report.txt?x=1527632d200000 > > console output: https://syzkaller.appspot.com/x/log.txt?x=1127632d200000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=faa7bdc352fc157e > > dashboard link: https://syzkaller.appspot.com/bug?extid=15927486a4f1bfcbaf91 > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=155543d3200000 > > > > Reported-by: syzbot+15927486a4f1bfcbaf91@xxxxxxxxxxxxxxxxxxxxxxxxx > > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector") > > Hi Amir! > It looks like lockless access to mark->connector is not safe as there is > nothing preventing a reader from seeing a mark on object list without > seeing the mark->connector assignment. Yes, that seems to be possible. > It made me wonder if (!mark->connector) check in fsnotify_put_mark() is safe. > I couldn't find any call site where that would be a problem, but perhaps > we should be more careful? Why? That check is there really only to catch destruction of a mark that was never attached (i.e., allocated but later never used due to some error). Otherwise we should always have mark->connector set. > Anyway, it seems that fsnotify_put_mark() uses the non NULL mark->connector > as the indication that mark is on object list, so just assigning mark->connector > before adding to object list won't do. > > Since a reference of mark is our guaranty that mark->connector is not > going away, I guess we could do opportunistic test for non NULL > mark->connector from lockless path, if that fails, we check again > with mark->lock held and if that fails something went wrong. > > Another option is to teach fsnotify_first_mark() and fsnotify_next_mark() > to skip over marks with NULL mark->connector. > > What do you think? Did I over complicate this? I'd prefer if we could make only fully initialized marks visible on connector's list. Just to make things simple in the fast path of handling events. So I'd just set mark->connector before adding mark to connector's list and having smp_wmb() there to force ordering in fsnotify_add_mark_list(). And we should use READ_ONCE() as a counter-part to this in fanotify_get_fsid(). It is somewhat unfortunate that there are three different ways how fsnotify_add_mark_list() can add mark to a list so we may need to either repeat this in three different places or just use some macro magic like: #define fanotify_attach_obj_list(where, mark, conn, head) \ do { \ mark->connector = conn; \ smp_wmb(); \ hlist_add_##where##_rcu(&mark->obj_list, head); \ } while (0) And I would not really worry about fsnotify_put_mark() - if it ever sees mark->connector set, mark really is on the connector's list and fsnotify_put_mark() does the right thing (i.e. locks connector->lock if needed). Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
