* memory leak in generic_parse_monolithic
@ 2020-11-13 17:17 syzbot
2020-12-06 2:45 ` memory leak in generic_parse_monolithic [+PATCH] Randy Dunlap
2020-12-08 8:36 ` David Howells
0 siblings, 2 replies; 9+ messages in thread
From: syzbot @ 2020-11-13 17:17 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, syzkaller-bugs, viro
Hello,
syzbot found the following issue on:
HEAD commit: af5043c8 Merge tag 'acpi-5.10-rc4' of git://git.kernel.org..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13e8c906500000
kernel config: https://syzkaller.appspot.com/x/.config?x=a3f13716fa0212fd
dashboard link: https://syzkaller.appspot.com/bug?extid=86dc6632faaca40133ab
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=102a57dc500000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=129ca3d6500000
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+86dc6632faaca40133ab@syzkaller.appspotmail.com
Warning: Permanently added '10.128.0.84' (ECDSA) to the list of known hosts.
executing program
executing program
BUG: memory leak
unreferenced object 0xffff888111f15a80 (size 32):
comm "syz-executor841", pid 8507, jiffies 4294942125 (age 14.070s)
hex dump (first 32 bytes):
25 5e 5d 24 5b 2b 25 5d 28 24 7b 3a 0f 6b 5b 29 %^]$[+%](${:.k[)
2d 3a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 -:..............
backtrace:
[<000000005c6f565d>] kmemdup_nul+0x2d/0x70 mm/util.c:151
[<0000000054985c27>] vfs_parse_fs_string+0x6e/0xd0 fs/fs_context.c:155
[<0000000077ef66e4>] generic_parse_monolithic+0xe0/0x130 fs/fs_context.c:201
[<00000000d4d4a652>] do_new_mount fs/namespace.c:2871 [inline]
[<00000000d4d4a652>] path_mount+0xbbb/0x1170 fs/namespace.c:3205
[<00000000f43f0071>] do_mount fs/namespace.c:3218 [inline]
[<00000000f43f0071>] __do_sys_mount fs/namespace.c:3426 [inline]
[<00000000f43f0071>] __se_sys_mount fs/namespace.c:3403 [inline]
[<00000000f43f0071>] __x64_sys_mount+0x18e/0x1d0 fs/namespace.c:3403
[<00000000dc5fffd5>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
[<000000004e665669>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-11-13 17:17 memory leak in generic_parse_monolithic syzbot @ 2020-12-06 2:45 ` Randy Dunlap 2020-12-08 8:36 ` David Howells 1 sibling, 0 replies; 9+ messages in thread From: Randy Dunlap @ 2020-12-06 2:45 UTC (permalink / raw) To: syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro, David Howells On 11/13/20 9:17 AM, syzbot wrote: > Hello, > > syzbot found the following issue on: > > HEAD commit: af5043c8 Merge tag 'acpi-5.10-rc4' of git://git.kernel.org.. > git tree: upstream > console output: https://syzkaller.appspot.com/x/log.txt?x=13e8c906500000 > kernel config: https://syzkaller.appspot.com/x/.config?x=a3f13716fa0212fd > dashboard link: https://syzkaller.appspot.com/bug?extid=86dc6632faaca40133ab > compiler: gcc (GCC) 10.1.0-syz 20200507 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=102a57dc500000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=129ca3d6500000 > > IMPORTANT: if you fix the issue, please add the following tag to the commit: > Reported-by: syzbot+86dc6632faaca40133ab@syzkaller.appspotmail.com > > Warning: Permanently added '10.128.0.84' (ECDSA) to the list of known hosts. > executing program > executing program > BUG: memory leak > unreferenced object 0xffff888111f15a80 (size 32): > comm "syz-executor841", pid 8507, jiffies 4294942125 (age 14.070s) > hex dump (first 32 bytes): > 25 5e 5d 24 5b 2b 25 5d 28 24 7b 3a 0f 6b 5b 29 %^]$[+%](${:.k[) > 2d 3a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 -:.............. > backtrace: > [<000000005c6f565d>] kmemdup_nul+0x2d/0x70 mm/util.c:151 > [<0000000054985c27>] vfs_parse_fs_string+0x6e/0xd0 fs/fs_context.c:155 > [<0000000077ef66e4>] generic_parse_monolithic+0xe0/0x130 fs/fs_context.c:201 > [<00000000d4d4a652>] do_new_mount fs/namespace.c:2871 [inline] > [<00000000d4d4a652>] path_mount+0xbbb/0x1170 fs/namespace.c:3205 > [<00000000f43f0071>] do_mount fs/namespace.c:3218 [inline] > [<00000000f43f0071>] __do_sys_mount fs/namespace.c:3426 [inline] > [<00000000f43f0071>] __se_sys_mount fs/namespace.c:3403 [inline] > [<00000000f43f0071>] __x64_sys_mount+0x18e/0x1d0 fs/namespace.c:3403 > [<00000000dc5fffd5>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 > [<000000004e665669>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Hi David, Is this a false positive, maybe having to do with this comment from fs/fsopen.c: ? /* * Check the state and apply the configuration. Note that this function is * allowed to 'steal' the value by setting param->xxx to NULL before returning. */ static int vfs_fsconfig_locked(struct fs_context *fc, int cmd, struct fs_parameter *param) { Otherwise please look at the patch below. Thanks. > --- > This report is generated by a bot. It may contain errors. > See https://goo.gl/tpsmEJ for more information about syzbot. > syzbot engineers can be reached at syzkaller@googlegroups.com. > > syzbot will keep track of this issue. See: > https://goo.gl/tpsmEJ#status for how to communicate with syzbot. > syzbot can test patches for this issue, for details see: > https://goo.gl/tpsmEJ#testing-patches --- From: Randy Dunlap <rdunlap@infradead.org> Callers to vfs_parse_fs_param() should be responsible for freeing param.string. Fixes: ecdab150fddb ("vfs: syscall: Add fsconfig() for configuring and managing a context") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: syzbot+86dc6632faaca40133ab@syzkaller.appspotmail.com Cc: David Howells <dhowells@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> --- This looks promising to me but I haven't fully tested it yet because my build/test machine just started acting flaky, like it is having memory or disk errors. OTOH, it could have ramifications in other places. fs/fs_context.c | 1 - fs/fsopen.c | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) --- linux-next-20201204.orig/fs/fs_context.c +++ linux-next-20201204/fs/fs_context.c @@ -128,7 +128,6 @@ int vfs_parse_fs_param(struct fs_context if (fc->source) return invalf(fc, "VFS: Multiple sources"); fc->source = param->string; - param->string = NULL; return 0; } --- linux-next-20201204.orig/fs/fsopen.c +++ linux-next-20201204/fs/fsopen.c @@ -262,7 +262,9 @@ static int vfs_fsconfig_locked(struct fs fc->phase != FS_CONTEXT_RECONF_PARAMS) return -EBUSY; - return vfs_parse_fs_param(fc, param); + ret = vfs_parse_fs_param(fc, param); + kfree(param->string); + return ret; } fc->phase = FS_CONTEXT_FAILED; return ret; ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 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 2020-12-08 22:54 ` David Howells 1 sibling, 2 replies; 9+ messages in thread From: David Howells @ 2020-12-08 8:36 UTC (permalink / raw) To: Randy Dunlap Cc: dhowells, syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-12-08 8:36 ` David Howells @ 2020-12-08 16:41 ` Randy Dunlap 2020-12-08 22:54 ` David Howells 1 sibling, 0 replies; 9+ messages in thread From: Randy Dunlap @ 2020-12-08 16:41 UTC (permalink / raw) To: David Howells; +Cc: syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-12-08 8:36 ` David Howells 2020-12-08 16:41 ` Randy Dunlap @ 2020-12-08 22:54 ` David Howells 2020-12-08 23:15 ` Randy Dunlap 2020-12-08 23:21 ` David Howells 1 sibling, 2 replies; 9+ messages in thread From: David Howells @ 2020-12-08 22:54 UTC (permalink / raw) To: Randy Dunlap Cc: dhowells, syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro Randy Dunlap <rdunlap@infradead.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? 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; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-12-08 22:54 ` David Howells @ 2020-12-08 23:15 ` Randy Dunlap 2020-12-09 6:03 ` Dmitry Vyukov 2020-12-08 23:21 ` David Howells 1 sibling, 1 reply; 9+ messages in thread From: Randy Dunlap @ 2020-12-08 23:15 UTC (permalink / raw) To: David Howells; +Cc: syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro On 12/8/20 2:54 PM, David Howells wrote: > Randy Dunlap <rdunlap@infradead.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. > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-12-08 23:15 ` Randy Dunlap @ 2020-12-09 6:03 ` Dmitry Vyukov 2020-12-09 6:13 ` Randy Dunlap 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Vyukov @ 2020-12-09 6:03 UTC (permalink / raw) To: Randy Dunlap Cc: David Howells, syzbot, linux-fsdevel, LKML, syzkaller-bugs, Al Viro On Wed, Dec 9, 2020 at 12:15 AM Randy Dunlap <rdunlap@infradead.org> wrote: > > On 12/8/20 2:54 PM, David Howells wrote: > > Randy Dunlap <rdunlap@infradead.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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/e6d9fd7e-ea43-25a6-9f1e-16a605de0f2d%40infradead.org. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-12-09 6:03 ` Dmitry Vyukov @ 2020-12-09 6:13 ` Randy Dunlap 0 siblings, 0 replies; 9+ messages in thread From: Randy Dunlap @ 2020-12-09 6:13 UTC (permalink / raw) To: Dmitry Vyukov Cc: David Howells, syzbot, linux-fsdevel, LKML, syzkaller-bugs, Al Viro On 12/8/20 10:03 PM, Dmitry Vyukov wrote: > On Wed, Dec 9, 2020 at 12:15 AM Randy Dunlap <rdunlap@infradead.org> wrote: >> >> On 12/8/20 2:54 PM, David Howells wrote: >>> Randy Dunlap <rdunlap@infradead.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 > OK, great, thanks for the info. > > >>> 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@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/e6d9fd7e-ea43-25a6-9f1e-16a605de0f2d%40infradead.org. -- ~Randy ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: memory leak in generic_parse_monolithic [+PATCH] 2020-12-08 22:54 ` David Howells 2020-12-08 23:15 ` Randy Dunlap @ 2020-12-08 23:21 ` David Howells 1 sibling, 0 replies; 9+ messages in thread From: David Howells @ 2020-12-08 23:21 UTC (permalink / raw) To: Randy Dunlap Cc: dhowells, syzbot, linux-fsdevel, linux-kernel, syzkaller-bugs, viro Randy Dunlap <rdunlap@infradead.org> wrote: > 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. Bah. Multiple source= parameters. I don't reject the second one, but just overwrite fc->source. David ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-12-09 6:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).