linux-kernel - Re: [PATCH] ext4: Use struct_size() to improve ext4_htree_store

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: <2024110510-hardening-turmoil-8032@gregkh>
Date: Tue, 5 Nov 2024 13:11:22 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: Theodore Ts'o <tytso@....edu>,
	Andreas Dilger <adilger.kernel@...ger.ca>, Jan Kara <jack@...e.cz>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ext4: Use struct_size() to improve
 ext4_htree_store_dirent()

On Tue, Nov 05, 2024 at 12:06:42PM +0100, Thorsten Blum wrote:
> On 5. Nov 2024, at 11:39, Greg KH wrote:
> > On Tue, Nov 05, 2024 at 11:33:54AM +0100, Thorsten Blum wrote:
> >> Inline and use struct_size() to calculate the number of bytes to
> >> allocate for new_fn and remove the local variable len.
> >> 
> >> Reviewed-by: Jan Kara <jack@...e.cz>
> >> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> >> ---
> >> This change was originally part of another patch that was split into two
> >> separate patches after feedback from Greg KH
> >> - Link: https://lore.kernel.org/r/20241104234214.8094-2-thorsten.blum@linux.dev/
> >> ---
> >> fs/ext4/dir.c | 5 ++---
> >> 1 file changed, 2 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> >> index 233479647f1b..02d47a64e8d1 100644
> >> --- a/fs/ext4/dir.c
> >> +++ b/fs/ext4/dir.c
> >> @@ -471,14 +471,13 @@ int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
> >> struct rb_node **p, *parent = NULL;
> >> struct fname *fname, *new_fn;
> >> struct dir_private_info *info;
> >> - int len;
> >> 
> >> info = dir_file->private_data;
> >> p = &info->root.rb_node;
> >> 
> >> /* Create and allocate the fname structure */
> >> - len = sizeof(struct fname) + ent_name->len + 1;
> >> - new_fn = kzalloc(len, GFP_KERNEL);
> >> + new_fn = kzalloc(struct_size(new_fn, name, ent_name->len + 1),
> >> + GFP_KERNEL);
> > 
> > Does this actually matter and make the code any more robust or faster?
> > 
> > The original code here is easier to read and understand, why add
> > complexity if it is not required?
> 
> I find struct_size() to be more readable because it explicitly
> communicates the relationship between the flexible array member name and
> ent_name->len that the open-coded version doesn't. Plus, struct_size()
> has some additional compile-time checks (e.g., __must_be_array()).

Ah, missed that, sure, that makes more sense, sorry for the noise.

greg k-h