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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <4F6CE3AC-0C34-46E5-BF67-95845DFC8449@linux.dev>
Date: Tue, 5 Nov 2024 12:06:42 +0100
From: Thorsten Blum <thorsten.blum@...ux.dev>
To: Greg KH <gregkh@...uxfoundation.org>
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 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()).

Thanks,
Thorsten