linux-kernel - Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode

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: <20250416191111.GC13877@suse.cz>
Date: Wed, 16 Apr 2025 21:11:11 +0200
From: David Sterba <dsterba@...e.cz>
To: Filipe Manana <fdmanana@...nel.org>
Cc: 李扬韬 <frank.li@...o.com>,
	Sun YangKai <sunk67188@...il.com>, "clm@...com" <clm@...com>,
	"dsterba@...e.com" <dsterba@...e.com>,
	"josef@...icpanda.com" <josef@...icpanda.com>,
	"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"neelx@...e.com" <neelx@...e.com>
Subject: Re: [PATCH 1/3] btrfs: get rid of path allocation in
 btrfs_del_inode_extref()

On Wed, Apr 16, 2025 at 02:37:33PM +0100, Filipe Manana wrote:
> On Wed, Apr 16, 2025 at 2:24 PM 李扬韬 <frank.li@...o.com> wrote:
> >
> >
> >
> > > Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.
> >
> > > Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.
> >
> > IIUC, use btrfs_reset_path instead of btrfs_release_path?
> >
> > noinline void btrfs_reset_path(struct btrfs_path *p)
> > {
> >         int i;
> >
> >         for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
> >                 if (!p->nodes[i])
> >                         continue;
> >                 if (p->locks[i])
> >                         btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
> >                 free_extent_buffer(p->nodes[i]);
> >         }
> >         memset(p, 0, sizeof(struct btrfs_path));
> > }
> >
> > BTW, I have seen released paths being passed across functions in some other paths.
> >
> > Should these also be changed to reset paths, or should these flags be cleared in the release path?
> 
> Please don't complicate things unnecessarily.
> The patch is fine, all that needs to be done is to call
> btrfs_release_path() before passing the path to
> btrfs_del_inode_extref(), which resets nodes, slots and locks.

But this leaves the bits set, btrfs_insert_inode_ref() sets
path->skip_release_on_error, this should be reset. In this case it may
not be significant but I'd rather make the path reusing pattern correct
from the beginning.

My idea was to add only

btrfs_reset_path() {
        memset(p, 0, sizeof(struct btrfs_path));
}

and use it in conection with btrfs_release_path() only in case it's
optimizing the allocation.