linux-kernel - Re: [PATCH] btrfs: keep `priv` struct on stack for sync reads in `btrfs_encoded_read_regular_fill

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: <c51c6cc1-4bc5-48d0-adce-a8d8d63227ce@wdc.com>
Date: Wed, 8 Jan 2025 18:29:24 +0000
From: Johannes Thumshirn <Johannes.Thumshirn@....com>
To: Daniel Vacek <neelx@...e.com>
CC: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>, David Sterba
	<dsterba@...e.com>, "linux-btrfs@...r.kernel.org"
	<linux-btrfs@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] btrfs: keep `priv` struct on stack for sync reads in
 `btrfs_encoded_read_regular_fill_pages()`

On 08.01.25 16:25, Daniel Vacek wrote:
> On Wed, 8 Jan 2025 at 13:42, Johannes Thumshirn
> <Johannes.Thumshirn@....com> wrote:
>>
>> On 08.01.25 12:44, Daniel Vacek wrote:
>>> Only allocate the `priv` struct from slab for asynchronous mode.
>>>
>>> There's no need to allocate an object from slab in the synchronous mode. In
>>> such a case stack can be happily used as it used to be before 68d3b27e05c7
>>> ("btrfs: move priv off stack in btrfs_encoded_read_regular_fill_pages()")
>>> which was a preparation for the async mode.
>>>
>>> While at it, fix the comment to reflect the atomic => refcount change in
>>> d29662695ed7 ("btrfs: fix use-after-free waiting for encoded read endios").
>>
>>
>> Generally I'm not a huge fan of conditional allocation/freeing. It just
>> complicates matters. I get it in case of the bio's bi_inline_vecs where
>> it's a optimization, but I fail to see why it would make a difference in
>> this case.
>>
>> If we're really going down that route, there should at least be a
>> justification other than "no need" to.
> 
> Well the main motivation was not to needlessly exercise the slab
> allocator when IO uring is not used. It is a bit of an overhead,
> though the object is not really big so I guess it's not a big deal
> after all (the slab should manage just fine even under low memory
> conditions).
> 
> 68d3b27e05c7 added the allocation for the async mode but also changed
> the original behavior of the sync mode which was using stack before.
> The async mode indeed requires the allocation as the object's lifetime
> extends over the function's one. The sync mode is perfectly contained
> within as it always was.
> 
> Simply, I tend not to do any allocations which are not strictly
> needed. If you prefer to simply allocate the object unconditionally,
> we can just drop this patch.

At the end of the day it's David's call, he's the maintainer. I'm just 
not sure if skipping the allocator for a small short lived object is 
worth the special casing. Especially as I got bitten by this in the past 
when hunting down kmemleak reports. Conditional allocation is like 
conditional locking, sometimes OK but it raises suspicion.