Message-ID: <CACT4Y+bbh=5SLG_ruq1QKd3xKaC-NzJo842KP7cmXFcRRrmOig@mail.gmail.com>
Date:   Wed, 9 Dec 2020 07:03:58 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Randy Dunlap <rdunlap@...radead.org>
Cc:     David Howells <dhowells@...hat.com>,
        syzbot <syzbot+86dc6632faaca40133ab@...kaller.appspotmail.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: memory leak in generic_parse_monolithic [+PATCH]

On Wed, Dec 9, 2020 at 12:15 AM Randy Dunlap <rdunlap@...radead.org> wrote:
>
> On 12/8/20 2:54 PM, David Howells wrote:
> > Randy Dunlap <rdunlap@...radead.org> wrote:
> >
> >>> Now the backtrace only shows what the state was when the string was allocated;
> >>> it doesn't show what happened to it after that, so another possibility is that
> >>> the filesystem being mounted nicked what vfs_parse_fs_param() had rightfully
> >>> stolen, transferring fc->source somewhere else and then failed to release it -
> >>> most likely on mount failure (ie. it's an error handling bug in the
> >>> filesystem).
> >>>
> >>> Do we know what filesystem it was?
> >>
> >> Yes, it's call AFS (or kAFS).
> >
> > Hmmm...  afs parses the string in afs_parse_source() without modifying it,
> > then moves the pointer to fc->source (parallelling vfs_parse_fs_param()) and
> > doesn't touch it again.  fc->source should be cleaned up by do_new_mount()
> > calling put_fs_context() at the end of the function.
> >
> > As far as I can tell with the attached print-insertion patch, it works, called
> > by the following commands, some of which are correct and some which aren't:
> >
> > # mount -t afs none /xfstest.test/ -o dyn
> > # umount /xfstest.test
> > # mount -t afs "" /xfstest.test/ -o foo
> > mount: /xfstest.test: bad option; for several filesystems (e.g. nfs, cifs) you might need a /sbin/mount.<type> helper program.
> > # umount /xfstest.test
> > umount: /xfstest.test: not mounted.
> > # mount -t afs %xfstest.test20 /xfstest.test/ -o foo
> > mount: /xfstest.test: bad option; for several filesystems (e.g. nfs, cifs) you might need a /sbin/mount.<type> helper program.
> > # umount /xfstest.test
> > umount: /xfstest.test: not mounted.
> > # mount -t afs %xfstest.test20 /xfstest.test/
> > # umount /xfstest.test
> >
> > Do you know if the mount was successful and what the mount parameters were?
>
> Here's the syzbot reproducer:
> https://syzkaller.appspot.com/x/repro.c?x=129ca3d6500000
>
> The "interesting" mount params are:
>         source=%^]$[+%](${:\017k[)-:,source=%^]$[+.](%{:\017\200[)-:,\000
>
> There is no other AFS activity: nothing mounted, no cells known (or
> whatever that is), etc.
>
> I don't recall if the mount was successful and I can't test it just now.
> My laptop is mucked up.
>
>
> Be aware that this report could just be a false positive: it waits
> for 5 seconds then looks for a memleak. AFAIK, it's possible that the "leaked"
> memory is still in valid use and will be freed some day.

FWIW KMEMLEAK scans memory for pointers. If it claims a memory leak,
it means the heap object is not referenced anywhere anymore. There are
no live pointers to it to call kfree or anything else.
Some false positives are theoretically possible, but so I don't
remember any, all reported ones were true leaks:
https://syzkaller.appspot.com/upstream/fixed?manager=ci-upstream-gce-leak



> > David
> > ---
> > diff --git a/fs/afs/super.c b/fs/afs/super.c
> > index 6c5900df6aa5..4c44ec0196c9 100644
> > --- a/fs/afs/super.c
> > +++ b/fs/afs/super.c
> > @@ -299,7 +299,7 @@ static int afs_parse_source(struct fs_context *fc, struct fs_parameter *param)
> >               ctx->cell = cell;
> >       }
> >
> > -     _debug("CELL:%s [%p] VOLUME:%*.*s SUFFIX:%s TYPE:%d%s",
> > +     kdebug("CELL:%s [%p] VOLUME:%*.*s SUFFIX:%s TYPE:%d%s",
> >              ctx->cell->name, ctx->cell,
> >              ctx->volnamesz, ctx->volnamesz, ctx->volname,
> >              suffix ?: "-", ctx->type, ctx->force ? " FORCE" : "");
> > @@ -318,6 +318,8 @@ static int afs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >       struct afs_fs_context *ctx = fc->fs_private;
> >       int opt;
> >
> > +     kenter("%s,%p '%s'", param->key, param->string, param->string);
> > +
> >       opt = fs_parse(fc, afs_fs_parameters, param, &result);
> >       if (opt < 0)
> >               return opt;
> > diff --git a/fs/fs_context.c b/fs/fs_context.c
> > index 2834d1afa6e8..f530a33876ce 100644
> > --- a/fs/fs_context.c
> > +++ b/fs/fs_context.c
> > @@ -450,6 +450,8 @@ void put_fs_context(struct fs_context *fc)
> >       put_user_ns(fc->user_ns);
> >       put_cred(fc->cred);
> >       put_fc_log(fc);
> > +     if (strcmp(fc->fs_type->name, "afs") == 0)
> > +             printk("PUT %p '%s'\n", fc->source, fc->source);
> >       put_filesystem(fc->fs_type);
> >       kfree(fc->source);
> >       kfree(fc);
> > @@ -671,6 +673,8 @@ void vfs_clean_context(struct fs_context *fc)
> >       fc->s_fs_info = NULL;
> >       fc->sb_flags = 0;
> >       security_free_mnt_opts(&fc->security);
> > +     if (strcmp(fc->fs_type->name, "afs") == 0)
> > +             printk("CLEAN %p '%s'\n", fc->source, fc->source);
> >       kfree(fc->source);
> >       fc->source = NULL;
> >
> >
>
> I'll check more after my test machine is working again.
>
> thanks.
> --
> ~Randy
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/e6d9fd7e-ea43-25a6-9f1e-16a605de0f2d%40infradead.org.

Powered by blists - more mailing lists