linux-kernel - Re: [PATCH v2] riscv: module: use a plain variable for list

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: <ba0f252c-345e-4988-8d4f-36116708f19e@rivosinc.com>
Date: Thu, 28 Nov 2024 08:36:04 +0100
From: Clément Léger <cleger@...osinc.com>
To: laokz <zhangkai@...as.ac.cn>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc: Charlie Jenkins <charlie@...osinc.com>,
 Andrew Jones <ajones@...tanamicro.com>
Subject: Re: [PATCH v2] riscv: module: use a plain variable for list_head
 instead of a pointer



On 28/11/2024 02:01, laokz wrote:
> On Wed, 2024-11-27 at 15:25 +0100, Clément Léger wrote:
>> rel_head's list_head member, rel_entry, doesn't need to be allocated,
>> its storage can just be part of the allocated rel_head. Remove the
> 
> Oh my poor English. OK, it's more better than just add the lost kfree.
> 
>> pointer which allows to get rid of the allocation as well as an
>> existing
>> memory leak found by Kai Zang using kmemleak.
>                             ^
>                            Zhang
> 
> BTW. Doesn't it need a fixes tag like what you suggested? (The bug
> might come from 6.7)

Hey Kai,

Apologies for misspelling your name, I'll fix that !

That's a good question, I guess I could have added it indeed but since
it was rather a rework, I left it out. But it's probably better to add
it anyway. I'll send a V3 with the Fixes tag as well as fixing your name
spelling.

Thanks,

Clément

> 
>>
>> Reported-by: Kai Zhang <zhangkai@...as.ac.cn>
>> Signed-off-by: Clément Léger <cleger@...osinc.com>
>> Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
>> ---
>>
>> V2:
>>  - Add Kai Reported-by
>>  - Reword the commit description (Andrew)
>>
>> ---
>>  arch/riscv/kernel/module.c | 18 ++++--------------
>>  1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
>> index 1cd461f3d872..47d0ebeec93c 100644
>> --- a/arch/riscv/kernel/module.c
>> +++ b/arch/riscv/kernel/module.c
>> @@ -23,7 +23,7 @@ struct used_bucket {
>>  
>>  struct relocation_head {
>>         struct hlist_node node;
>> -       struct list_head *rel_entry;
>> +       struct list_head rel_entry;
>>         void *location;
>>  };
>>  
>> @@ -634,7 +634,7 @@ process_accumulated_relocations(struct module
>> *me,
>>                         location = rel_head_iter->location;
>>                         list_for_each_entry_safe(rel_entry_iter,
>>                                                  rel_entry_iter_tmp,
>> -                                                rel_head_iter-
>>> rel_entry,
>> +                                                &rel_head_iter-
>>> rel_entry,
>>                                                  head) {
>>                                 curr_type = rel_entry_iter->type;
>>                                 reloc_handlers[curr_type].reloc_handl
>> er(
>> @@ -704,16 +704,7 @@ static int add_relocation_to_accumulate(struct
>> module *me, int type,
>>                         return -ENOMEM;
>>                 }
>>  
>> -               rel_head->rel_entry =
>> -                       kmalloc(sizeof(struct list_head),
>> GFP_KERNEL);
>> -
>> -               if (!rel_head->rel_entry) {
>> -                       kfree(entry);
>> -                       kfree(rel_head);
>> -                       return -ENOMEM;
>> -               }
>> -
>> -               INIT_LIST_HEAD(rel_head->rel_entry);
>> +               INIT_LIST_HEAD(&rel_head->rel_entry);
>>                 rel_head->location = location;
>>                 INIT_HLIST_NODE(&rel_head->node);
>>                 if (!current_head->first) {
>> @@ -722,7 +713,6 @@ static int add_relocation_to_accumulate(struct
>> module *me, int type,
>>  
>>                         if (!bucket) {
>>                                 kfree(entry);
>> -                               kfree(rel_head->rel_entry);
>>                                 kfree(rel_head);
>>                                 return -ENOMEM;
>>                         }
>> @@ -735,7 +725,7 @@ static int add_relocation_to_accumulate(struct
>> module *me, int type,
>>         }
>>  
>>         /* Add relocation to head of discovered rel_head */
>> -       list_add_tail(&entry->head, rel_head->rel_entry);
>> +       list_add_tail(&entry->head, &rel_head->rel_entry);
>>  
>>         return 0;
>>  }
>