Re: WARNING in fanotify_handle_event

[Amir Goldstein]

On Tue, Jun 18, 2019 at 11:27 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Tue, Jun 18, 2019 at 8:07 PM syzbot
> <syzbot+c277e8e2f46414645508@xxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit:    963172d9 Merge branch 'x86-urgent-for-linus' of git://git...
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=17c090eaa00000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=fa9f7e1b6a8bb586
> > dashboard link: https://syzkaller.appspot.com/bug?extid=c277e8e2f46414645508
> > compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=15a32f46a00000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=13a7dc9ea00000
> >
> > The bug was bisected 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=12bfcb66a00000
> > final crash:    https://syzkaller.appspot.com/x/report.txt?x=11bfcb66a00000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=16bfcb66a00000
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+c277e8e2f46414645508@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> >
> > WARNING: CPU: 0 PID: 8994 at fs/notify/fanotify/fanotify.c:359
> > fanotify_get_fsid fs/notify/fanotify/fanotify.c:359 [inline]
>
> Oops, we forgot to update conn->fsid when the first mark added
> for inode has no fsid (e.g. inotify) and the second mark has fid,
> which is more or less the only thing the repro does.
> And if we are going to update conn->fsid, we do no have the
> cmpxchg to guaranty setting fsid atomically.
>
> I am thinking a set-once flag on connector FSNOTIFY_CONN_HAS_FSID
> checked before smp_rmb() in fanotify_get_fsid().
> If the flag is not set then call vfs_get_fsid() instead of using fsid cache.

Actually, we don't need to call vfs_get_fsid() in race we just drop the event.

> conn->fsid can be updated in fsnotify_add_mark_list() under conn->lock,
> and flag set after smp_wmb().
>
> Does that sound correct?
>

Something like this:

#syz test: https://github.com/amir73il/linux.git fsnotify-fix-fsid-cache

It passed my modified ltp test:
https://github.com/amir73il/ltp/commits/fanotify_dirent

Thanks,
Amir.