From: Randy Dunlap <rdunlap@infradead.org>
To: David Howells <dhowells@redhat.com>
Cc: syzbot <syzbot+86dc6632faaca40133ab@syzkaller.appspotmail.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
syzkaller-bugs@googlegroups.com, viro@zeniv.linux.org.uk
Subject: Re: memory leak in generic_parse_monolithic [+PATCH]
Date: Tue, 8 Dec 2020 08:41:50 -0800 [thread overview]
Message-ID: <1c752ffe-8118-f9ea-e928-d92783a5c516@infradead.org> (raw)
In-Reply-To: <928043.1607416561@warthog.procyon.org.uk>
On 12/8/20 12:36 AM, David Howells wrote:
> Randy Dunlap <rdunlap@infradead.org> wrote:
>
>> Otherwise please look at the patch below.
>
> The patch won't help, since it's not going through sys_fsconfig() - worse, it
> introduces two new errors.
>
>> fc->source = param->string;
>> - param->string = NULL;
>
> This will cause the string now attached to fc->source to be freed by the
> caller. No, the original is doing the correct thing here. The point is to
> steal the string.
>
>> @@ -262,7 +262,9 @@ static int vfs_fsconfig_locked(struct fs
>>
>> - return vfs_parse_fs_param(fc, param);
>> + ret = vfs_parse_fs_param(fc, param);
>> + kfree(param->string);
>> + return ret;
>
> But your stack trace shows you aren't going through sys_fsconfig(), so this
> function isn't involved. Further, this introduces a double free, since
> sys_fsconfig() frees param.string after it drops uapi_mutex.
>
> Looking at the backtrace:
>
>> kmemdup_nul+0x2d/0x70 mm/util.c:151
>> vfs_parse_fs_string+0x6e/0xd0 fs/fs_context.c:155
>> generic_parse_monolithic+0xe0/0x130 fs/fs_context.c:201
>> do_new_mount fs/namespace.c:2871 [inline]
>> path_mount+0xbbb/0x1170 fs/namespace.c:3205
>> do_mount fs/namespace.c:3218 [inline]
>> __do_sys_mount fs/namespace.c:3426 [inline]
>> __se_sys_mount fs/namespace.c:3403 [inline]
>> __x64_sys_mount+0x18e/0x1d0 fs/namespace.c:3403
>
> A couple of possibilities spring to mind from that: maybe
> vfs_parse_fs_string() is not releasing the param.string - but that's not the
> problem since we stole the string and the free is definitely there at the
> bottom of the function:
>
> int vfs_parse_fs_string(struct fs_context *fc, const char *key,
> const char *value, size_t v_size)
> {
> ...
> kfree(param.string);
> return ret;
> }
>
> or fc->source is not being cleaned up in vfs_clean_context() - but that's
> there as well:
>
> void vfs_clean_context(struct fs_context *fc)
> {
> ...
> kfree(fc->source);
> fc->source = NULL;
>
> In either of these cases, I would expect this to have already become evident
> from other filesystem mounts as there would be a lot of leaking going on,
> particularly with the first.
>
> 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).
Thanks for your comments & help.
--
~Randy
next prev parent reply other threads:[~2020-12-08 16:42 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-13 17:17 memory leak in generic_parse_monolithic syzbot
2020-12-06 2:45 ` memory leak in generic_parse_monolithic [+PATCH] Randy Dunlap
2020-12-08 8:36 ` David Howells
2020-12-08 16:41 ` Randy Dunlap [this message]
2020-12-08 22:54 ` David Howells
2020-12-08 23:15 ` Randy Dunlap
2020-12-09 6:03 ` Dmitry Vyukov
2020-12-09 6:13 ` Randy Dunlap
2020-12-08 23:21 ` David Howells
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c752ffe-8118-f9ea-e928-d92783a5c516@infradead.org \
--to=rdunlap@infradead.org \
--cc=dhowells@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=syzbot+86dc6632faaca40133ab@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).