Messages in this thread | | | Date | Wed, 28 Oct 2015 00:39:07 +0200 | Subject | Re: 4.3-rc7: kmemleak BUG: Redzone overwritten | From | Andy Shevchenko <> |
| |
On Wed, Oct 28, 2015 at 12:16 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Oct 28, 2015 at 12:46 AM, Aaro Koskinen <aaro.koskinen@nokia.com> wrote: >> >> With 4.3-rc7 and slub_debug=FZUP, I get the below when reading >> /sys/kernel/debug/kmemleak with a large number of reported entries. >> It's pretty repeatable. HW is MIPS64. >> >> With the SLUB debugging disabled, box crashes randomly in kmem_cache_free >> or kmem_cache_alloc when the kmemleak file is read on a running system. >> >> Seems to start with 6fc37c490076 ("kmemleak: use seq_hex_dump() to >> dump buffers"). > > Well, so that commit itself looks fine - it just uses the seq accessor > functions to print things out, instead of doing it by hand. > > So if that commit causes problems, then I suspect that the real issue > is that seq_hex_dump() itself is buggered, and that the commit just > exposed it by adding new use-cases. It looks like the hexdump wrote > one byte (the terminating NUL) past the end of the buffer: > >> [ 77.706871] BUG kmalloc-4096 (Not tainted): Redzone overwritten >> [ 77.706877] >> [ 77.706894] INFO: 0x800000002e939000-0x800000002e939000. First byte 0x0 instead of 0xcc >> [ 77.706914] INFO: Allocated in seq_buf_alloc+0x24/0x58 age=452 cpu=2 pid=587 >> [ 77.706928] __slab_alloc.isra.72.constprop.75+0x4a4/0x508 >> [ 77.706938] __kmalloc+0x30c/0x3f0 >> [ 77.706947] seq_buf_alloc+0x24/0x58 >> [ 77.706956] seq_read+0x304/0x4a0 >> [ 77.706968] __vfs_read+0x3c/0x100 >> [ 77.706977] vfs_read+0x8c/0x138 >> [ 77.706987] SyS_read+0x64/0xe8 >> [ 77.707000] syscall_common+0x34/0x58 >> [ 77.707012] INFO: Freed in seq_release+0x24/0x40 age=3450 cpu=3 pid=584 >> [ 77.707023] __slab_free+0x340/0x4f0 >> [ 77.707032] seq_release+0x24/0x40 >> [ 77.707044] kernfs_fop_release+0x50/0x80 >> [ 77.707055] __fput+0xa4/0x218 >> [ 77.707066] task_work_run+0xb0/0x108 >> [ 77.707078] work_notifysig+0x10/0x18 >> [ 77.707087] INFO: Slab 0x8000000003ec4440 objects=7 used=1 fp=0x800000002e93e7b0 flags=0x200000004081 >> [ 77.707095] INFO: Object 0x800000002e938000 @offset=0 fp=0x800000002e939148 >> [ 77.707095] >> [ 77.707108] Object 800000002e938000: 75 6e 72 65 66 65 72 65 6e 63 65 64 20 6f 62 6a unreferenced obj >> [ 77.707118] Object 800000002e938010: 65 63 74 20 30 78 38 30 30 30 30 30 30 30 32 66 ect 0x800000002f > ... >> [ 77.709583] Object 800000002e938f90: 6d 6d 20 22 73 77 61 70 70 65 72 2f 30 22 2c 20 mm "swapper/0", >> [ 77.709593] Object 800000002e938fa0: 70 69 64 20 31 2c 20 6a 69 66 66 69 65 73 20 34 pid 1, jiffies 4 >> [ 77.709603] Object 800000002e938fb0: 32 39 34 39 33 38 30 35 31 20 28 61 67 65 20 34 294938051 (age 4 >> [ 77.709613] Object 800000002e938fc0: 31 2e 35 37 30 73 29 0a 20 20 68 65 78 20 64 75 1.570s). hex du >> [ 77.709623] Object 800000002e938fd0: 6d 70 20 28 66 69 72 73 74 20 33 32 20 62 79 74 mp (first 32 byt >> [ 77.709633] Object 800000002e938fe0: 65 73 29 3a 0a 20 20 20 20 36 62 20 36 62 20 36 es):. 6b 6b 6 >> [ 77.709643] Object 800000002e938ff0: 62 20 36 62 20 36 62 20 36 62 20 36 62 20 00 20 b 6b 6b 6b 6b . >> [ 77.709653] Redzone 800000002e939000: 00 cc cc cc cc cc cc cc ........ > > So I suspect that some seq function ends up adding a terminating NUL > character too much when the buffer overflows. > > The obvious suspect would be the "hex_dump_to_buffer()" call in > seq_hex_dump(). It's the only thing that doesn't use really common > core helpers, though. > > Looking at "hex_dump_to_buffer()", code like this strikes me as > particularly dangerous: > > if (linebuflen < lx + 3) > goto overflow2;
Just send couple of minutes before a message (pity it was in html, due to was sent from phone). Similar suspicion.
> ... > overflow2: > linebuf[lx++] = '\0'; > overflow1: > return ascii ? ascii_column + len : (groupsize * 2 + 1) * > ngroups - 1; > > because what if lx == linebuflen in the overflow condition. > > But the non-overflow condition looks a bit scary too: the > "non-overflow" case checks that there is room for three characters, > and then adds those three characters (and possible removes the last > one). Fine - but what if the three characters *exactly* filled the > buffer, and we think we haven't overflowed, and now we just do > > nil: > linebuf[lx] = '\0'; > return lx; > > there as the "success" case. > > So without trying to really analyze this, I do suspect that the > problem is in either of those cases. > > I would suggest the "nil:" case do > > nil: > if (lx < linebuflen) > linebuf[lx] = 0; > return lx; > > and add something similar to overflow2 too.
I don't think it should be fixed like this. All other cases are checking for room correctly (if I didn't miss anything).
Here I would like to repeat the snprintf() behaviour, i.e. print each symbol separately
if (linwbuflen < lx + 2) goto overflow; linebuf[lx++] = hi_byte; ...
> > Hmm? Does that fix your test-case? Added Al Viro as seq_file > maintainer to the cc.
-- With Best Regards, Andy Shevchenko
| |