[PATCH v3 0/2] riscv: Fix issues with module loading

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v3 0/2] riscv: Fix issues with module loading
@ 2023-11-22 23:31 Charlie Jenkins
  2023-11-22 23:31 ` [PATCH v3 1/2] riscv: Safely remove entries from relocation list Charlie Jenkins
  2023-11-22 23:31 ` [PATCH v3 2/2] riscv: Correct type casting in module loading Charlie Jenkins
  0 siblings, 2 replies; 5+ messages in thread
From: Charlie Jenkins @ 2023-11-22 23:31 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Ron Economos,
	Samuel Holland
  Cc: linux-riscv, linux-kernel, Charlie Jenkins

Module loading did not account for multiple threads concurrently loading
modules. This patch fixes that issue. There is also a small patch to fix
the type of a __le16 variable.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Changes in v3:
- Cleanup pointer passing (Samuel)
- Correct indentation (Samuel)
- Check for kmalloc failures (Samuel)
- Link to v2: https://lore.kernel.org/r/20231121-module_linking_freeing-v2-1-974bfcd3664e@rivosinc.com

Changes in v2:
- Support linking modules concurrently across threads.
- Link to v1: https://lore.kernel.org/r/20231120-module_linking_freeing-v1-1-fff81d7289fc@rivosinc.com

---
Charlie Jenkins (2):
      riscv: Safely remove entries from relocation list
      riscv: Correct type casting in module loading

 arch/riscv/kernel/module.c | 97 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 71 insertions(+), 26 deletions(-)
---
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
change-id: 20231120-module_linking_freeing-2b5a3b255b5e
-- 
- Charlie


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/2] riscv: Safely remove entries from relocation list
  2023-11-22 23:31 [PATCH v3 0/2] riscv: Fix issues with module loading Charlie Jenkins
@ 2023-11-22 23:31 ` Charlie Jenkins
  2023-11-27 10:42   ` Andreas Schwab
  2023-11-22 23:31 ` [PATCH v3 2/2] riscv: Correct type casting in module loading Charlie Jenkins
  1 sibling, 1 reply; 5+ messages in thread
From: Charlie Jenkins @ 2023-11-22 23:31 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Ron Economos,
	Samuel Holland
  Cc: linux-riscv, linux-kernel, Charlie Jenkins

Use the safe versions of list and hlist iteration to safely remove
entries from the module relocation lists. To allow mutliple threads to
load modules concurrently, move relocation list pointers onto the stack
rather than using global variables.

Fixes: 8fd6c5142395 ("riscv: Add remaining module relocations")
Reported-by: Ron Economos <re@w6rz.net>
Closes: https://lore.kernel.org/linux-riscv/444de86a-7e7c-4de7-5d1d-c1c40eefa4ba@w6rz.net
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
 arch/riscv/kernel/module.c | 93 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 69 insertions(+), 24 deletions(-)

diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index 56a8c78e9e21..fd9a5533518c 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -40,14 +40,16 @@ struct relocation_handlers {
 				  long buffer);
 };
 
-unsigned int initialize_relocation_hashtable(unsigned int num_relocations);
-void process_accumulated_relocations(struct module *me);
+unsigned int
+initialize_relocation_hashtable(unsigned int num_relocations,
+				struct hlist_head **relocation_hashtable);
+void process_accumulated_relocations(struct module *me,
+				     struct hlist_head **relocation_hashtable,
+				     struct list_head *used_buckets_list);
 int add_relocation_to_accumulate(struct module *me, int type, void *location,
-				 unsigned int hashtable_bits, Elf_Addr v);
-
-struct hlist_head *relocation_hashtable;
-
-struct list_head used_buckets_list;
+				 unsigned int hashtable_bits, Elf_Addr v,
+				 struct hlist_head *relocation_hashtable,
+				 struct list_head *used_buckets_list);
 
 /*
  * The auipc+jalr instruction pair can reach any PC-relative offset
@@ -604,7 +606,9 @@ static const struct relocation_handlers reloc_handlers[] = {
 	/* 192-255 nonstandard ABI extensions  */
 };
 
-void process_accumulated_relocations(struct module *me)
+void process_accumulated_relocations(struct module *me,
+				     struct hlist_head **relocation_hashtable,
+				     struct list_head *used_buckets_list)
 {
 	/*
 	 * Only ADD/SUB/SET/ULEB128 should end up here.
@@ -624,18 +628,25 @@ void process_accumulated_relocations(struct module *me)
 	 *	- Each relocation entry for a location address
 	 */
 	struct used_bucket *bucket_iter;
+	struct used_bucket *bucket_iter_tmp;
 	struct relocation_head *rel_head_iter;
+	struct hlist_node *rel_head_iter_tmp;
 	struct relocation_entry *rel_entry_iter;
+	struct relocation_entry *rel_entry_iter_tmp;
 	int curr_type;
 	void *location;
 	long buffer;
 
-	list_for_each_entry(bucket_iter, &used_buckets_list, head) {
-		hlist_for_each_entry(rel_head_iter, bucket_iter->bucket, node) {
+	list_for_each_entry_safe(bucket_iter, bucket_iter_tmp,
+				 used_buckets_list, head) {
+		hlist_for_each_entry_safe(rel_head_iter, rel_head_iter_tmp,
+					  bucket_iter->bucket, node) {
 			buffer = 0;
 			location = rel_head_iter->location;
-			list_for_each_entry(rel_entry_iter,
-					    rel_head_iter->rel_entry, head) {
+			list_for_each_entry_safe(rel_entry_iter,
+						 rel_entry_iter_tmp,
+						 rel_head_iter->rel_entry,
+						 head) {
 				curr_type = rel_entry_iter->type;
 				reloc_handlers[curr_type].reloc_handler(
 					me, &buffer, rel_entry_iter->value);
@@ -648,11 +659,13 @@ void process_accumulated_relocations(struct module *me)
 		kfree(bucket_iter);
 	}
 
-	kfree(relocation_hashtable);
+	kfree(*relocation_hashtable);
 }
 
 int add_relocation_to_accumulate(struct module *me, int type, void *location,
-				 unsigned int hashtable_bits, Elf_Addr v)
+				 unsigned int hashtable_bits, Elf_Addr v,
+				 struct hlist_head *relocation_hashtable,
+				 struct list_head *used_buckets_list)
 {
 	struct relocation_entry *entry;
 	struct relocation_head *rel_head;
@@ -661,6 +674,10 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
 	unsigned long hash;
 
 	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+
+	if (!entry)
+		return -ENOMEM;
+
 	INIT_LIST_HEAD(&entry->head);
 	entry->type = type;
 	entry->value = v;
@@ -683,17 +700,29 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
 
 	if (!found) {
 		rel_head = kmalloc(sizeof(*rel_head), GFP_KERNEL);
+
+		if (!rel_head)
+			return -ENOMEM;
+
 		rel_head->rel_entry =
 			kmalloc(sizeof(struct list_head), GFP_KERNEL);
+
+		if (!rel_head->rel_entry)
+			return -ENOMEM;
+
 		INIT_LIST_HEAD(rel_head->rel_entry);
 		rel_head->location = location;
 		INIT_HLIST_NODE(&rel_head->node);
 		if (!current_head->first) {
 			bucket =
 				kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
+
+			if (!bucket)
+				return -ENOMEM;
+
 			INIT_LIST_HEAD(&bucket->head);
 			bucket->bucket = current_head;
-			list_add(&bucket->head, &used_buckets_list);
+			list_add(&bucket->head, used_buckets_list);
 		}
 		hlist_add_head(&rel_head->node, current_head);
 	}
@@ -704,7 +733,9 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
 	return 0;
 }
 
-unsigned int initialize_relocation_hashtable(unsigned int num_relocations)
+unsigned int
+initialize_relocation_hashtable(unsigned int num_relocations,
+				struct hlist_head **relocation_hashtable)
 {
 	/* Can safely assume that bits is not greater than sizeof(long) */
 	unsigned long hashtable_size = roundup_pow_of_two(num_relocations);
@@ -720,12 +751,13 @@ unsigned int initialize_relocation_hashtable(unsigned int num_relocations)
 
 	hashtable_size <<= should_double_size;
 
-	relocation_hashtable = kmalloc_array(hashtable_size,
-					     sizeof(*relocation_hashtable),
-					     GFP_KERNEL);
-	__hash_init(relocation_hashtable, hashtable_size);
+	*relocation_hashtable = kmalloc_array(hashtable_size,
+					      sizeof(*relocation_hashtable),
+					      GFP_KERNEL);
+	if (!*relocation_hashtable)
+		return -ENOMEM;
 
-	INIT_LIST_HEAD(&used_buckets_list);
+	__hash_init(*relocation_hashtable, hashtable_size);
 
 	return hashtable_bits;
 }
@@ -742,7 +774,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 	Elf_Addr v;
 	int res;
 	unsigned int num_relocations = sechdrs[relsec].sh_size / sizeof(*rel);
-	unsigned int hashtable_bits = initialize_relocation_hashtable(num_relocations);
+	struct hlist_head *relocation_hashtable;
+	struct list_head used_buckets_list;
+	unsigned int hashtable_bits;
+
+	hashtable_bits = initialize_relocation_hashtable(num_relocations,
+							 &relocation_hashtable);
+
+	if (hashtable_bits < 0)
+		return hashtable_bits;
+
+	INIT_LIST_HEAD(&used_buckets_list);
 
 	pr_debug("Applying relocate section %u to %u\n", relsec,
 	       sechdrs[relsec].sh_info);
@@ -823,14 +865,17 @@ int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
 		}
 
 		if (reloc_handlers[type].accumulate_handler)
-			res = add_relocation_to_accumulate(me, type, location, hashtable_bits, v);
+			res = add_relocation_to_accumulate(
+				me, type, location, hashtable_bits, v,
+				relocation_hashtable, &used_buckets_list);
 		else
 			res = handler(me, location, v);
 		if (res)
 			return res;
 	}
 
-	process_accumulated_relocations(me);
+	process_accumulated_relocations(me, &relocation_hashtable,
+					&used_buckets_list);
 
 	return 0;
 }

-- 
2.42.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/2] riscv: Safely remove entries from relocation list
  2023-11-22 23:31 ` [PATCH v3 1/2] riscv: Safely remove entries from relocation list Charlie Jenkins
@ 2023-11-27 10:42   ` Andreas Schwab
  2023-12-04 18:42     ` Charlie Jenkins
  0 siblings, 1 reply; 5+ messages in thread
From: Andreas Schwab @ 2023-11-27 10:42 UTC (permalink / raw)
  To: Charlie Jenkins
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Ron Economos,
	Samuel Holland, linux-riscv, linux-kernel

On Nov 22 2023, Charlie Jenkins wrote:

> @@ -683,17 +700,29 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
>  
>  	if (!found) {
>  		rel_head = kmalloc(sizeof(*rel_head), GFP_KERNEL);
> +
> +		if (!rel_head)
> +			return -ENOMEM;
> +
>  		rel_head->rel_entry =
>  			kmalloc(sizeof(struct list_head), GFP_KERNEL);
> +
> +		if (!rel_head->rel_entry)
> +			return -ENOMEM;

This leaks rel_head on error.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/2] riscv: Safely remove entries from relocation list
  2023-11-27 10:42   ` Andreas Schwab
@ 2023-12-04 18:42     ` Charlie Jenkins
  0 siblings, 0 replies; 5+ messages in thread
From: Charlie Jenkins @ 2023-12-04 18:42 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Ron Economos,
	Samuel Holland, linux-riscv, linux-kernel

On Mon, Nov 27, 2023 at 11:42:08AM +0100, Andreas Schwab wrote:
> On Nov 22 2023, Charlie Jenkins wrote:
> 
> > @@ -683,17 +700,29 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
> >  
> >  	if (!found) {
> >  		rel_head = kmalloc(sizeof(*rel_head), GFP_KERNEL);
> > +
> > +		if (!rel_head)
> > +			return -ENOMEM;
> > +
> >  		rel_head->rel_entry =
> >  			kmalloc(sizeof(struct list_head), GFP_KERNEL);
> > +
> > +		if (!rel_head->rel_entry)
> > +			return -ENOMEM;
> 
> This leaks rel_head on error.
> 
> -- 
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

Thank you for pointing this out, I fixed this issue in the next version
but forgot to respond to this thread.

https://lore.kernel.org/linux-riscv/20231127-module_linking_freeing-v4-1-a2ca1d7027d0@rivosinc.com/

- Charlie


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 2/2] riscv: Correct type casting in module loading
  2023-11-22 23:31 [PATCH v3 0/2] riscv: Fix issues with module loading Charlie Jenkins
  2023-11-22 23:31 ` [PATCH v3 1/2] riscv: Safely remove entries from relocation list Charlie Jenkins
@ 2023-11-22 23:31 ` Charlie Jenkins
  1 sibling, 0 replies; 5+ messages in thread
From: Charlie Jenkins @ 2023-11-22 23:31 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Ron Economos,
	Samuel Holland
  Cc: linux-riscv, linux-kernel, Charlie Jenkins

Use __le16 with le16_to_cpu.

Fixes: 8fd6c5142395 ("riscv: Add remaining module relocations")
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
Tested-by: Samuel Holland <samuel.holland@sifive.com>
---
 arch/riscv/kernel/module.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index fd9a5533518c..b570988e7d43 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -66,7 +66,7 @@ static bool riscv_insn_valid_32bit_offset(ptrdiff_t val)
 
 static int riscv_insn_rmw(void *location, u32 keep, u32 set)
 {
-	u16 *parcel = location;
+	__le16 *parcel = location;
 	u32 insn = (u32)le16_to_cpu(parcel[0]) | (u32)le16_to_cpu(parcel[1]) << 16;
 
 	insn &= keep;
@@ -79,7 +79,7 @@ static int riscv_insn_rmw(void *location, u32 keep, u32 set)
 
 static int riscv_insn_rvc_rmw(void *location, u16 keep, u16 set)
 {
-	u16 *parcel = location;
+	__le16 *parcel = location;
 	u16 insn = le16_to_cpu(*parcel);
 
 	insn &= keep;

-- 
2.42.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-12-04 18:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-22 23:31 [PATCH v3 0/2] riscv: Fix issues with module loading Charlie Jenkins
2023-11-22 23:31 ` [PATCH v3 1/2] riscv: Safely remove entries from relocation list Charlie Jenkins
2023-11-27 10:42   ` Andreas Schwab
2023-12-04 18:42     ` Charlie Jenkins
2023-11-22 23:31 ` [PATCH v3 2/2] riscv: Correct type casting in module loading Charlie Jenkins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox