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

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

[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 v4:
- Make functions only used internally static
- Free data structures on kmalloc failure (Andreas)
- Link to v3: https://lore.kernel.org/r/20231122-module_linking_freeing-v3-0-8e9e412a3305@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 | 114 +++++++++++++++++++++++++++++++++------------
 1 file changed, 84 insertions(+), 30 deletions(-)
---
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
change-id: 20231120-module_linking_freeing-2b5a3b255b5e
-- 
- Charlie


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

[PATCH v4 1/2] riscv: Safely remove entries from relocation list

[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 | 110 +++++++++++++++++++++++++++++++++------------
 1 file changed, 82 insertions(+), 28 deletions(-)

diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index 56a8c78e9e21..53593fe58cd8 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -40,15 +40,6 @@ struct relocation_handlers {
 				  long buffer);
 };
 
-unsigned int initialize_relocation_hashtable(unsigned int num_relocations);
-void process_accumulated_relocations(struct module *me);
-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;
-
 /*
  * The auipc+jalr instruction pair can reach any PC-relative offset
  * in the range [-2^31 - 2^11, 2^31 - 2^11)
@@ -604,7 +595,10 @@ static const struct relocation_handlers reloc_handlers[] = {
 	/* 192-255 nonstandard ABI extensions  */
 };
 
-void process_accumulated_relocations(struct module *me)
+static 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 +618,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 +649,14 @@ 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)
+static 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)
 {
 	struct relocation_entry *entry;
 	struct relocation_head *rel_head;
@@ -661,6 +665,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;
@@ -669,7 +677,10 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
 
 	current_head = &relocation_hashtable[hash];
 
-	/* Find matching location (if any) */
+	/*
+	 * Search for the relocation_head for the relocations that happen at the
+	 * provided location
+	 */
 	bool found = false;
 	struct relocation_head *rel_head_iter;
 
@@ -681,19 +692,45 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
 		}
 	}
 
+	/*
+	 * If there has not yet been any relocations at the provided location,
+	 * create a relocation_head for that location and populate it with this
+	 * relocation_entry.
+	 */
 	if (!found) {
 		rel_head = kmalloc(sizeof(*rel_head), GFP_KERNEL);
+
+		if (!rel_head) {
+			kfree(entry);
+			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);
 		rel_head->location = location;
 		INIT_HLIST_NODE(&rel_head->node);
 		if (!current_head->first) {
 			bucket =
 				kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
+
+			if (!bucket) {
+				kfree(entry);
+				kfree(rel_head);
+				kfree(rel_head->rel_entry);
+				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 +741,9 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
 	return 0;
 }
 
-unsigned int initialize_relocation_hashtable(unsigned int num_relocations)
+static 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 +759,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 +782,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 +873,18 @@ 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


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

Re: [PATCH v4 1/2] riscv: Safely remove entries from relocation list

[Björn Töpel]

Charlie Jenkins <charlie@rivosinc.com> writes:

> 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 | 110 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 82 insertions(+), 28 deletions(-)
>
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 56a8c78e9e21..53593fe58cd8 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -40,15 +40,6 @@ struct relocation_handlers {
>  				  long buffer);
>  };
>  
> -unsigned int initialize_relocation_hashtable(unsigned int num_relocations);
> -void process_accumulated_relocations(struct module *me);
> -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;
> -
>  /*
>   * The auipc+jalr instruction pair can reach any PC-relative offset
>   * in the range [-2^31 - 2^11, 2^31 - 2^11)
> @@ -604,7 +595,10 @@ static const struct relocation_handlers reloc_handlers[] = {
>  	/* 192-255 nonstandard ABI extensions  */
>  };
>  
> -void process_accumulated_relocations(struct module *me)
> +static void
> +process_accumulated_relocations(struct module *me,

Nit/breaks my workflow ;-): Don't bother if you're not respinning for
other reasons. The linebreak after return type makes it harder to grep
the code (and also is not in line with the layout with rest of this).


Björn

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

Re: [PATCH v4 1/2] riscv: Safely remove entries from relocation list

[Lad, Prabhakar]

On Mon, Nov 27, 2023 at 10:08 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> 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 | 110 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 82 insertions(+), 28 deletions(-)
>
This fixes issues seen on RZ/Five SMARC EVK.

Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> #On
RZ/Five SMARC EVK

Cheers,
Prabhakar

> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 56a8c78e9e21..53593fe58cd8 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -40,15 +40,6 @@ struct relocation_handlers {
>                                   long buffer);
>  };
>
> -unsigned int initialize_relocation_hashtable(unsigned int num_relocations);
> -void process_accumulated_relocations(struct module *me);
> -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;
> -
>  /*
>   * The auipc+jalr instruction pair can reach any PC-relative offset
>   * in the range [-2^31 - 2^11, 2^31 - 2^11)
> @@ -604,7 +595,10 @@ static const struct relocation_handlers reloc_handlers[] = {
>         /* 192-255 nonstandard ABI extensions  */
>  };
>
> -void process_accumulated_relocations(struct module *me)
> +static 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 +618,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 +649,14 @@ 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)
> +static 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)
>  {
>         struct relocation_entry *entry;
>         struct relocation_head *rel_head;
> @@ -661,6 +665,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;
> @@ -669,7 +677,10 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
>
>         current_head = &relocation_hashtable[hash];
>
> -       /* Find matching location (if any) */
> +       /*
> +        * Search for the relocation_head for the relocations that happen at the
> +        * provided location
> +        */
>         bool found = false;
>         struct relocation_head *rel_head_iter;
>
> @@ -681,19 +692,45 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
>                 }
>         }
>
> +       /*
> +        * If there has not yet been any relocations at the provided location,
> +        * create a relocation_head for that location and populate it with this
> +        * relocation_entry.
> +        */
>         if (!found) {
>                 rel_head = kmalloc(sizeof(*rel_head), GFP_KERNEL);
> +
> +               if (!rel_head) {
> +                       kfree(entry);
> +                       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);
>                 rel_head->location = location;
>                 INIT_HLIST_NODE(&rel_head->node);
>                 if (!current_head->first) {
>                         bucket =
>                                 kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
> +
> +                       if (!bucket) {
> +                               kfree(entry);
> +                               kfree(rel_head);
> +                               kfree(rel_head->rel_entry);
> +                               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 +741,9 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
>         return 0;
>  }
>
> -unsigned int initialize_relocation_hashtable(unsigned int num_relocations)
> +static 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 +759,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 +782,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 +873,18 @@ 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
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[PATCH v4 2/2] riscv: Correct type casting in module loading

[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 53593fe58cd8..aac019ed63b1 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -55,7 +55,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;
@@ -68,7 +68,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


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

Re: [PATCH v4 2/2] riscv: Correct type casting in module loading

[Lad, Prabhakar]

On Mon, Nov 27, 2023 at 10:08 PM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> 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(-)
>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> #On
RZ/Five SMARC EVK

Cheers,
Prabhakar

> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 53593fe58cd8..aac019ed63b1 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -55,7 +55,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;
> @@ -68,7 +68,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
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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

[Björn Töpel]

Charlie Jenkins <charlie@rivosinc.com> writes:

> 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 v4:
> - Make functions only used internally static
> - Free data structures on kmalloc failure (Andreas)
> - Link to v3: https://lore.kernel.org/r/20231122-module_linking_freeing-v3-0-8e9e412a3305@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

I got hit by this on QEMU for 6.7-rc3:
  | Unable to handle kernel paging request at virtual address 6b6b6b6b6b6b6b83
  | Oops [#1]
  | Modules linked in:
  | CPU: 3 PID: 1 Comm: systemd Not tainted 6.7.0-rc3 #1
  | Hardware name: riscv-virtio,qemu (DT)
  | epc : process_accumulated_relocations+0x70/0x162
  |  ra : process_accumulated_relocations+0x96/0x162
  | epc : ffffffff8000c528 ra : ffffffff8000c54e sp : ff20000000023ab0
  |  gp : ffffffff8257fea0 tp : ff60000080860040 t0 : ff60000083934d68
  |  t1 : 0000000000000006 t2 : 0000000000000002 s0 : ff20000000023b30
  |  s1 : 6b6b6b6b6b6b6b6b a0 : ffffffff80ed48d8 a1 : 0000000000000000
  |  a2 : ff1c0000040a9610 a3 : 0000000000000000 a4 : 0000000000000000
  |  a5 : ff600000832d6990 a6 : ff1c0000040e4d10 a7 : 000000000bbb1d6d
  |  s2 : 0000000000000023 s3 : 0000000000000230 s4 : ffffffff0264dd40
  |  s5 : 000000000000003e s6 : ffffffff81401898 s7 : ffffffff0264d040
  |  s8 : ff60000083934e80 s9 : ffffffff825b7240 s10: ff60000083934de0
  |  s11: ff200000007a25b0 t3 : ffffffff0264d000 t4 : ffffffff0264d040
  |  t5 : 0000000000000017 t6 : ff60000083367eb8
  | status: 0000000200000120 badaddr: 6b6b6b6b6b6b6b83 cause: 000000000000000d
  | [<ffffffff8000c528>] process_accumulated_relocations+0x70/0x162
  | [<ffffffff8000ca9a>] apply_relocate_add+0x142/0x37a
  | [<ffffffff800e79f0>] load_module+0x1720/0x227c
  | [<ffffffff800e8790>] init_module_from_file+0x82/0xba
  | [<ffffffff800e8940>] __riscv_sys_finit_module+0x178/0x300
  | [<ffffffff80ec9160>] do_trap_ecall_u+0xc6/0x142
  | [<ffffffff80ed5cac>] ret_from_exception+0x0/0x64
  | Code: 060d 3783 010d 3823 f804 3b83 018d 6384 8363 0a97 (a903) 0184 
  | ---[ end trace 0000000000000000 ]---

This series resolved the issue for me.

Tested-by: Björn Töpel <bjorn@rivosinc.com>

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

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

[patchwork-bot+linux-riscv]

Hello:

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

On Mon, 27 Nov 2023 14:04:58 -0800 you wrote:
> 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 v4:
> - Make functions only used internally static
> - Free data structures on kmalloc failure (Andreas)
> - Link to v3: https://lore.kernel.org/r/20231122-module_linking_freeing-v3-0-8e9e412a3305@rivosinc.com
> 
> [...]

Here is the summary with links:
  - [v4,1/2] riscv: Safely remove entries from relocation list
    https://git.kernel.org/riscv/c/d8792a5734b0
  - [v4,2/2] riscv: Correct type casting in module loading
    https://git.kernel.org/riscv/c/4a92a87950c4

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