[PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[Clément Léger]

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
pointer which allows to get rid of the allocation as well as an existing
memory leak found by Kai Zang using kmemleak.

Reported-by: Kai Zhang <zhangkai@iscas.ac.cn>
Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.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_handler(
@@ -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;
 }
-- 
2.45.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[Charlie Jenkins]

On Wed, Nov 27, 2024 at 03:25:17PM +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
> pointer which allows to get rid of the allocation as well as an existing
> memory leak found by Kai Zang using kmemleak.
> 
> Reported-by: Kai Zhang <zhangkai@iscas.ac.cn>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks for looking into this!

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.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_handler(
> @@ -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;
>  }
> -- 
> 2.45.2
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[laokz]

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)

> 
> Reported-by: Kai Zhang <zhangkai@iscas.ac.cn>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.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;
>  }

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[Clément Léger]

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@iscas.ac.cn>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.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;
>>  }
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[Andrew Jones]

On Thu, Nov 28, 2024 at 09:01:52AM +0800, 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.
>

It wasn't the English I was correcting. That was fine, but just saying
the object could be "a plain variable" wasn't giving any justification
for the change and, to me, even implied that rel_entry was locally
scoped. So, when I first skimmed the patch and saw that it was getting
appended to a list, I almost stated the patch was wrong. It was clear
after looking closer, but it could have been clear the first time
through if the commit message had better guided me.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[laokz]

On Thu, 2024-11-28 at 09:32 +0100, Andrew Jones wrote:
> On Thu, Nov 28, 2024 at 09:01:52AM +0800, 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.
> > 
> 
> It wasn't the English I was correcting. That was fine, but just
> saying
> the object could be "a plain variable" wasn't giving any
> justification
> for the change and, to me, even implied that rel_entry was locally
> scoped. So, when I first skimmed the patch and saw that it was
> getting
> appended to a list, I almost stated the patch was wrong. It was clear
> after looking closer, but it could have been clear the first time
> through if the commit message had better guided me.

Thanks for the explanation.
laokz

> Thanks,
> drew


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: module: use a plain variable for list_head instead of a pointer

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed, 27 Nov 2024 15:25:17 +0100 you 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
> pointer which allows to get rid of the allocation as well as an existing
> memory leak found by Kai Zang using kmemleak.
> 
> Reported-by: Kai Zhang <zhangkai@iscas.ac.cn>
> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
> [...]

Here is the summary with links:
  - [v2] riscv: module: use a plain variable for list_head instead of a pointer
    https://git.kernel.org/riscv/c/03f0b548537f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv