Re: possible deadlock in dquot_commit

[Dmitry Vyukov <dvyukov@google.com>]

On Fri, Feb 12, 2021 at 5:10 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
>  >From: Theodore Ts'o <tytso@mit.edu>
>
> On Fri, Feb 12, 2021 at 12:01:51PM +0100, Dmitry Vyukov wrote:
> > > >
> > > > There is a reproducer for 4.19 available on the dashboard. Maybe it will help.
> > > > I don't why it did not pop up on upstream yet, there lots of potential
> > > > reasons for this.
> > >
> > > The 4.19 version of the syzbot report has a very different stack
> > > trace.  Instead of it being related to an apparent write to the quota
> > > file, it is apparently caused by a call to rmdir:
> > >
> >
> > The 4.19 reproducer may reproducer something else, you know better. I
> > just want to answer points re syzkaller reproducers. FTR the 4.19
> > reproducer/reproducer is here:
> > https://syzkaller.appspot.com/bug?id=b6cacc9fa48fea07154b8797236727de981c1e02
>
> Yes, I know.  That was my point.  I don't think it's useful for
> debugging the upstream dquot_commit syzbot report (for which we don't
> have a reproducer yet).
>
> > > there is never any attempt to run rmdir() on the corrupted file system that is mounted.
> >
> > Recursive rmdir happens as part of test cleanup implicitly, you can
> > see rmdir call in remove_dir function in the C reproducer:
> > https://syzkaller.appspot.com/text?tag=ReproC&x=12caea37900000
>
> That rmdir() removes the mountpoint, which is *not* the fuzzed file
> system which has the quota feature enabled.

remove_dir function is recursive, so rmdir should be called for all
subdirectories starting from the deepest ones. At least that was the
intention. Do you see it's not working this way? That would be
something to fix.

> > > procid never gets incremented, so all of the threads only operate on /dev/loop0
> >
> > This is intentional. procid is supposed to "isolate" parallel test
> > processes (if any). This reproducer does not use parallel test
> > processes, thus procid has constant value.
>
> Um... yes it does:

There is waitpid before remove_dir. So these are sequential test
processes, not parallel.

> int main(void)
> {
>   syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
>   syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul);
>   syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
>   use_temporary_dir();
>   loop();
>   return 0;
> }
>
> and what is loop?
>
> static void loop(void)
> {
>   int iter = 0;
>   for (;; iter++) {
>         ...
>     reset_loop();
>     int pid = fork();
>     if (pid < 0)
>       exit(1);
>     if (pid == 0) {
>       if (chdir(cwdbuf))
>         exit(1);
>       setup_test();
>       execute_one();
>       exit(0);
>     }
>     ...
>     remove_dir(cwdbuf);
>   }
> }
>
> > > Am I correct in understanding that when syzbot is running, it uses the syzbot repro, and not the C repro?
> >
> > It tries both. If first tries to interpret "syzkaller program" as it
> > was done when the bug was triggered during fuzzing. But then it tries
> > to convert it to a corresponding stand-alone C program and confirms
> > that it still triggers the bug. If it provides a C reproducer, it
> > means that it did trigger the bug using this exact C program on a
> > freshly booted kernel (and the provided kernel oops is the
> > corresponding oops obtained on this exact program).
> > If it fails to reproduce the bug with a C reproducer, then it provides
> > only the "syzkaller program" to not mislead developers.
>
> Well, looking at the C reproducer, it doesn't reproduce on upstream,
> and the stack trace makes no sense to me.  The rmdir() executes at the
> end of the test, as part of the cleanup, and looking at the syzkaller
> console, the stack trace involving rmdir happens *early* while test
> threads are still trying to mount the file system.

My assumption that the 4.19 reproducer for a somewhat similarly
looking bug may also reproduce this upstream bug is false then.