| 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
| ||
|
Message-ID: <CAPjX3Fcd2MgzhRtDEGRsNNZ1qARiaT0edfnqPA-5HAchwf1gAg@mail.gmail.com>
Date: Wed, 22 Oct 2025 10:06:17 +0200
From: Daniel Vacek <neelx@...e.com>
To: dsterba@...e.cz
Cc: Miquel Sabaté Solà <mssola@...ola.com>,
linux-btrfs@...r.kernel.org, clm@...com, dsterba@...e.com,
johannes.thumshirn@....com, fdmanana@...e.com, boris@....io, wqu@...e.com,
neal@...pa.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] btrfs: declare free_ipath() via DEFINE_FREE() instead
On Wed, 22 Oct 2025 at 09:32, David Sterba <dsterba@...e.cz> wrote:
>
> On Tue, Oct 21, 2025 at 04:27:46PM +0200, Miquel Sabaté Solà wrote:
> > This transforms the signature to __free_ipath() instead of the original
> > free_ipath(), but this function was only being used as a cleanup
> > function anyways. Hence, define it as a helper and use it via the
> > __free() attribute on all uses. This change also means that
> > __free_ipath() will be inlined whereas that wasn't the case for the
> > original one, but this shouldn't be a problem.
> >
> > A follow up macro like we do with BTRFS_PATH_AUTO_FREE() has been
> > discarded as the usage of this struct is not as widespread as that.
>
> The point of adding the macros or at least the freeing wrappers is to
> force the NULL initialization and to make it visible in the declarations
> (typed all in capitals). The number of use should not be the main factor
> and in this case it's 4 files.
>
> > Signed-off-by: Miquel Sabaté Solà <mssola@...ola.com>
> > ---
> > fs/btrfs/backref.c | 10 +---------
> > fs/btrfs/backref.h | 7 ++++++-
> > fs/btrfs/inode.c | 4 +---
> > fs/btrfs/ioctl.c | 3 +--
> > fs/btrfs/scrub.c | 4 +---
> > 5 files changed, 10 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index e050d0938dc4..a1456402752a 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
> > @@ -2785,7 +2785,7 @@ struct btrfs_data_container *init_data_container(u32 total_bytes)
> > * allocates space to return multiple file system paths for an inode.
> > * total_bytes to allocate are passed, note that space usable for actual path
> > * information will be total_bytes - sizeof(struct inode_fs_paths).
> > - * the returned pointer must be freed with free_ipath() in the end.
> > + * the returned pointer must be freed with __free_ipath() in the end.
> > */
> > struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
> > struct btrfs_path *path)
> > @@ -2810,14 +2810,6 @@ struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
> > return ifp;
> > }
> >
> > -void free_ipath(struct inode_fs_paths *ipath)
> > -{
> > - if (!ipath)
> > - return;
> > - kvfree(ipath->fspath);
> > - kfree(ipath);
> > -}
> > -
> > struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_info)
> > {
> > struct btrfs_backref_iter *ret;
> > diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
> > index 25d51c246070..d3b1ad281793 100644
> > --- a/fs/btrfs/backref.h
> > +++ b/fs/btrfs/backref.h
> > @@ -241,7 +241,12 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
> > struct btrfs_data_container *init_data_container(u32 total_bytes);
> > struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
> > struct btrfs_path *path);
> > -void free_ipath(struct inode_fs_paths *ipath);
> > +
> > +DEFINE_FREE(ipath, struct inode_fs_paths *,
> > + if (_T) {
>
> You can drop the if() as kvfree/kfree handles NULL pointers and we don't
> do that in the struct btrfs_path either.
But it's being dereferenced right on the next line. I think the check
is still needed due to that.
> > + kvfree(_T->fspath);
> > + kfree(_T);
> > + })
> >
> > int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
> > u64 start_off, struct btrfs_path *path,
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 79732756b87f..4d154209d70b 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -130,7 +130,7 @@ static int data_reloc_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
> > struct btrfs_fs_info *fs_info = warn->fs_info;
> > struct extent_buffer *eb;
> > struct btrfs_inode_item *inode_item;
> > - struct inode_fs_paths *ipath = NULL;
> > + struct inode_fs_paths *ipath __free(ipath) = NULL;
>
> I'd spell the name in full, like __free(free_ipath) or
> __free(inode_fs_paths) so it matches the type not the variable name.
>
> > struct btrfs_root *local_root;
> > struct btrfs_key key;
> > unsigned int nofs_flag;
> > @@ -193,7 +193,6 @@ static int data_reloc_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
> > }
> >
> > btrfs_put_root(local_root);
> > - free_ipath(ipath);
> > return 0;
> >
> > err:
> > @@ -201,7 +200,6 @@ static int data_reloc_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
> > "checksum error at logical %llu mirror %u root %llu inode %llu offset %llu, path resolving failed with ret=%d",
> > warn->logical, warn->mirror_num, root, inum, offset, ret);
> >
> > - free_ipath(ipath);
> > return ret;
> > }
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 692016b2b600..453832ded917 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -3298,7 +3298,7 @@ static long btrfs_ioctl_ino_to_path(struct btrfs_root *root, void __user *arg)
> > u64 rel_ptr;
> > int size;
> > struct btrfs_ioctl_ino_path_args *ipa = NULL;
> > - struct inode_fs_paths *ipath = NULL;
> > + struct inode_fs_paths *ipath __free(ipath) = NULL;
> > struct btrfs_path *path;
> >
> > if (!capable(CAP_DAC_READ_SEARCH))
> > @@ -3346,7 +3346,6 @@ static long btrfs_ioctl_ino_to_path(struct btrfs_root *root, void __user *arg)
> >
> > out:
> > btrfs_free_path(path);
> > - free_ipath(ipath);
> > kfree(ipa);
> >
> > return ret;
> > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> > index fe266785804e..74d8af1ff02d 100644
> > --- a/fs/btrfs/scrub.c
> > +++ b/fs/btrfs/scrub.c
> > @@ -505,7 +505,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
> > struct btrfs_inode_item *inode_item;
> > struct scrub_warning *swarn = warn_ctx;
> > struct btrfs_fs_info *fs_info = swarn->dev->fs_info;
> > - struct inode_fs_paths *ipath = NULL;
> > + struct inode_fs_paths *ipath __free(ipath) = NULL;
> > struct btrfs_root *local_root;
> > struct btrfs_key key;
> >
> > @@ -569,7 +569,6 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
> > (char *)(unsigned long)ipath->fspath->val[i]);
> >
> > btrfs_put_root(local_root);
> > - free_ipath(ipath);
> > return 0;
> >
> > err:
> > @@ -580,7 +579,6 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
> > swarn->physical,
> > root, inum, offset, ret);
> >
> > - free_ipath(ipath);
> > return 0;
> > }
> >
> > --
> > 2.51.1
> >
>
Powered by blists - more mailing lists