linux-kernel - Re: [GIT PULL] bcachefs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4d3efe17-e114-96c1-dca8-a100cc6f7fc6@kernel.dk>
Date:   Wed, 28 Jun 2023 14:54:06 -0600
From:   Jens Axboe <axboe@...nel.dk>
To:     Kent Overstreet <kent.overstreet@...ux.dev>
Cc:     torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-bcachefs@...r.kernel.org,
        Christoph Hellwig <hch@....de>
Subject: Re: [GIT PULL] bcachefs

On 6/28/23 11:54?AM, Kent Overstreet wrote:
> On Wed, Jun 28, 2023 at 08:58:45AM -0600, Jens Axboe wrote:
>> On 6/27/23 10:01?PM, Kent Overstreet wrote:
>>> On Tue, Jun 27, 2023 at 09:16:31PM -0600, Jens Axboe wrote:
>>>> On 6/27/23 2:15?PM, Kent Overstreet wrote:
>>>>>> to ktest/tests/xfstests/ and run it with -bcachefs, otherwise it kept
>>>>>> failing because it assumed it was XFS.
>>>>>>
>>>>>> I suspected this was just a timing issue, and it looks like that's
>>>>>> exactly what it is. Looking at the test case, it'll randomly kill -9
>>>>>> fsstress, and if that happens while we have io_uring IO pending, then we
>>>>>> process completions inline (for a PF_EXITING current). This means they
>>>>>> get pushed to fallback work, which runs out of line. If we hit that case
>>>>>> AND the timing is such that it hasn't been processed yet, we'll still be
>>>>>> holding a file reference under the mount point and umount will -EBUSY
>>>>>> fail.
>>>>>>
>>>>>> As far as I can tell, this can happen with aio as well, it's just harder
>>>>>> to hit. If the fput happens while the task is exiting, then fput will
>>>>>> end up being delayed through a workqueue as well. The test case assumes
>>>>>> that once it's reaped the exit of the killed task that all files are
>>>>>> released, which isn't necessarily true if they are done out-of-line.
>>>>>
>>>>> Yeah, I traced it through to the delayed fput code as well.
>>>>>
>>>>> I'm not sure delayed fput is responsible here; what I learned when I was
>>>>> tracking this down has mostly fell out of my brain, so take anything I
>>>>> say with a large grain of salt. But I believe I tested with delayed_fput
>>>>> completely disabled, and found another thing in io_uring with the same
>>>>> effect as delayed_fput that wasn't being flushed.
>>>>
>>>> I'm not saying it's delayed_fput(), I'm saying it's the delayed putting
>>>> io_uring can end up doing. But yes, delayed_fput() is another candidate.
>>>
>>> Sorry - was just working through my recollections/initial thought
>>> process out loud
>>
>> No worries, it might actually be a combination and this is why my
>> io_uring side patch didn't fully resolve it. Wrote a simple reproducer
>> and it seems to reliably trigger it, but is fixed with an flush of the
>> delayed fput list on mount -EBUSY return. Still digging...
>>
>>>>>> For io_uring specifically, it may make sense to wait on the fallback
>>>>>> work. The below patch does this, and should fix the issue. But I'm not
>>>>>> fully convinced that this is really needed, as I do think this can
>>>>>> happen without io_uring as well. It just doesn't right now as the test
>>>>>> does buffered IO, and aio will be fully sync with buffered IO. That
>>>>>> means there's either no gap where aio will hit it without O_DIRECT, or
>>>>>> it's just small enough that it hasn't been hit.
>>>>>
>>>>> I just tried your patch and I still have generic/388 failing - it
>>>>> might've taken a bit longer to pop this time.
>>>>
>>>> Yep see the same here. Didn't have time to look into it after sending
>>>> that email today, just took a quick stab at writing a reproducer and
>>>> ended up crashing bcachefs:
>>>
>>> You must have hit an error before we finished initializing the
>>> filesystem, the list head never got initialized. Patch for that will be
>>> in the testing branch momentarily.
>>
>> I'll pull that in. In testing just now, I hit a few more leaks:
>>
>> unreferenced object 0xffff0000e55cf200 (size 128):
>>   comm "mount", pid 723, jiffies 4294899134 (age 85.868s)
>>   hex dump (first 32 bytes):
>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>   backtrace:
>>     [<000000001d69062c>] slab_post_alloc_hook.isra.0+0xb4/0xbc
>>     [<00000000c503def2>] __kmem_cache_alloc_node+0xd0/0x178
>>     [<00000000cde48528>] __kmalloc+0xac/0xd4
>>     [<000000006cb9446a>] kmalloc_array.constprop.0+0x18/0x20
>>     [<000000008341b32c>] bch2_fs_alloc+0x73c/0xbcc
> 
> Can you faddr2line this? I just did a bunch of kmemleak testing and
> didn't see it.

0xffff800008589a20 is in bch2_fs_alloc (fs/bcachefs/super.c:813).
808		    !(c->online_reserved = alloc_percpu(u64)) ||
809		    !(c->btree_paths_bufs = alloc_percpu(struct btree_path_buf)) ||
810		    mempool_init_kvpmalloc_pool(&c->btree_bounce_pool, 1,
811						btree_bytes(c)) ||
812		    mempool_init_kmalloc_pool(&c->large_bkey_pool, 1, 2048) ||
813		    !(c->unused_inode_hints = kcalloc(1U << c->inode_shard_bits,
814						      sizeof(u64), GFP_KERNEL))) {
815			ret = -BCH_ERR_ENOMEM_fs_other_alloc;
816			goto err;
817		}

-- 
Jens Axboe