lkml.org 
[lkml]   [2020]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: memory leak in generic_parse_monolithic [+PATCH]
Date
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?

David

\
 
 \ /
  Last update: 2020-12-08 09:39    [W:0.420 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site