* [PATCH v2] riscv: Safely remove entries from relocation list
@ 2023-11-21 22:50 Charlie Jenkins
2023-11-22 0:04 ` Samuel Holland
2023-11-22 1:22 ` Ron Economos
0 siblings, 2 replies; 4+ messages in thread
From: Charlie Jenkins @ 2023-11-21 22:50 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Ron Economos
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>
---
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
---
arch/riscv/kernel/module.c | 76 +++++++++++++++++++++++++++++++---------------
1 file changed, 51 insertions(+), 25 deletions(-)
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index 56a8c78e9e21..f53e82b70dff 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -40,14 +40,17 @@ 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,
+ struct list_head *used_buckets_list);
+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 +607,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 +629,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 +660,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;
@@ -667,7 +681,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
hash = hash_min((uintptr_t)location, hashtable_bits);
- current_head = &relocation_hashtable[hash];
+ current_head = &((*relocation_hashtable)[hash]);
/* Find matching location (if any) */
bool found = false;
@@ -693,7 +707,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
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 +718,10 @@ 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,
+ struct list_head *used_buckets_list)
{
/* Can safely assume that bits is not greater than sizeof(long) */
unsigned long hashtable_size = roundup_pow_of_two(num_relocations);
@@ -720,12 +737,12 @@ 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);
+ __hash_init(*relocation_hashtable, hashtable_size);
- INIT_LIST_HEAD(&used_buckets_list);
+ INIT_LIST_HEAD(used_buckets_list);
return hashtable_bits;
}
@@ -742,7 +759,13 @@ 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,
+ &used_buckets_list);
pr_debug("Applying relocate section %u to %u\n", relsec,
sechdrs[relsec].sh_info);
@@ -823,14 +846,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;
}
---
base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
change-id: 20231120-module_linking_freeing-2b5a3b255b5e
--
- Charlie
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] riscv: Safely remove entries from relocation list
2023-11-21 22:50 [PATCH v2] riscv: Safely remove entries from relocation list Charlie Jenkins
@ 2023-11-22 0:04 ` Samuel Holland
2023-11-22 3:41 ` Charlie Jenkins
2023-11-22 1:22 ` Ron Economos
1 sibling, 1 reply; 4+ messages in thread
From: Samuel Holland @ 2023-11-22 0:04 UTC (permalink / raw)
To: Charlie Jenkins
Cc: linux-riscv, linux-kernel, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Ron Economos
Hi Charlie,
On 2023-11-21 4:50 PM, Charlie Jenkins 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>
> ---
> 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
> ---
> arch/riscv/kernel/module.c | 76 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 56a8c78e9e21..f53e82b70dff 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -40,14 +40,17 @@ 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,
> + struct list_head *used_buckets_list);
> +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;
This hunk conflicts with your other patch, which is still needed for the __le16
change. Since they are both fixes, do you intend to rebase and send them together?
> + unsigned int hashtable_bits, Elf_Addr v,
> + struct hlist_head **relocation_hashtable,
> + struct list_head *used_buckets_list);
minor: the indentation is off by one here.
>
> /*
> * The auipc+jalr instruction pair can reach any PC-relative offset
> @@ -604,7 +607,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,
You only need the double pointer in initialize_relocation_hashtable(). If you
pass the single pointer here and in add_relocation_to_accumulate(), you can
avoid the extra dereference operations.
> + struct list_head *used_buckets_list)
> {
> /*
> * Only ADD/SUB/SET/ULEB128 should end up here.
> @@ -624,18 +629,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 +660,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;
> @@ -667,7 +681,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
>
> hash = hash_min((uintptr_t)location, hashtable_bits);
>
> - current_head = &relocation_hashtable[hash];
> + current_head = &((*relocation_hashtable)[hash]);
>
> /* Find matching location (if any) */
> bool found = false;
> @@ -693,7 +707,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
> kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
> 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 +718,10 @@ 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,
> + struct list_head *used_buckets_list)
> {
> /* Can safely assume that bits is not greater than sizeof(long) */
> unsigned long hashtable_size = roundup_pow_of_two(num_relocations);
> @@ -720,12 +737,12 @@ 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);
You need to check for allocation failure here and inside
add_relocation_to_accumulate(). Module loading under memory pressure is a
reasonably likely scenario.
> + __hash_init(*relocation_hashtable, hashtable_size);
>
> - INIT_LIST_HEAD(&used_buckets_list);
> + INIT_LIST_HEAD(used_buckets_list);
This is the only place used_buckets_list is used in this function. If you move
this line out to apply_relocate_add, you can drop the parameter.
Regards,
Samuel
>
> return hashtable_bits;
> }
> @@ -742,7 +759,13 @@ 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,
> + &used_buckets_list);
>
> pr_debug("Applying relocate section %u to %u\n", relsec,
> sechdrs[relsec].sh_info);
> @@ -823,14 +846,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;
> }
>
> ---
> base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
> change-id: 20231120-module_linking_freeing-2b5a3b255b5e
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] riscv: Safely remove entries from relocation list
2023-11-22 0:04 ` Samuel Holland
@ 2023-11-22 3:41 ` Charlie Jenkins
0 siblings, 0 replies; 4+ messages in thread
From: Charlie Jenkins @ 2023-11-22 3:41 UTC (permalink / raw)
To: Samuel Holland
Cc: linux-riscv, linux-kernel, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Ron Economos
On Tue, Nov 21, 2023 at 06:04:14PM -0600, Samuel Holland wrote:
> Hi Charlie,
>
> On 2023-11-21 4:50 PM, Charlie Jenkins 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>
> > ---
> > 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
> > ---
> > arch/riscv/kernel/module.c | 76 +++++++++++++++++++++++++++++++---------------
> > 1 file changed, 51 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> > index 56a8c78e9e21..f53e82b70dff 100644
> > --- a/arch/riscv/kernel/module.c
> > +++ b/arch/riscv/kernel/module.c
> > @@ -40,14 +40,17 @@ 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,
> > + struct list_head *used_buckets_list);
> > +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;
>
> This hunk conflicts with your other patch, which is still needed for the __le16
> change. Since they are both fixes, do you intend to rebase and send them together?
I wasn't sure the best way of doing that. I will bring the __le16 changes into this series.
>
> > + unsigned int hashtable_bits, Elf_Addr v,
> > + struct hlist_head **relocation_hashtable,
> > + struct list_head *used_buckets_list);
>
> minor: the indentation is off by one here.
>
> >
> > /*
> > * The auipc+jalr instruction pair can reach any PC-relative offset
> > @@ -604,7 +607,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,
>
> You only need the double pointer in initialize_relocation_hashtable(). If you
> pass the single pointer here and in add_relocation_to_accumulate(), you can
> avoid the extra dereference operations.
>
> > + struct list_head *used_buckets_list)
> > {
> > /*
> > * Only ADD/SUB/SET/ULEB128 should end up here.
> > @@ -624,18 +629,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 +660,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;
> > @@ -667,7 +681,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
> >
> > hash = hash_min((uintptr_t)location, hashtable_bits);
> >
> > - current_head = &relocation_hashtable[hash];
> > + current_head = &((*relocation_hashtable)[hash]);
> >
> > /* Find matching location (if any) */
> > bool found = false;
> > @@ -693,7 +707,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
> > kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
> > 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 +718,10 @@ 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,
> > + struct list_head *used_buckets_list)
> > {
> > /* Can safely assume that bits is not greater than sizeof(long) */
> > unsigned long hashtable_size = roundup_pow_of_two(num_relocations);
> > @@ -720,12 +737,12 @@ 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);
>
> You need to check for allocation failure here and inside
> add_relocation_to_accumulate(). Module loading under memory pressure is a
> reasonably likely scenario.
>
> > + __hash_init(*relocation_hashtable, hashtable_size);
> >
> > - INIT_LIST_HEAD(&used_buckets_list);
> > + INIT_LIST_HEAD(used_buckets_list);
>
> This is the only place used_buckets_list is used in this function. If you move
> this line out to apply_relocate_add, you can drop the parameter.
>
> Regards,
> Samuel
>
Thanks for the suggestions, I will send out another version.
- Charlie
> >
> > return hashtable_bits;
> > }
> > @@ -742,7 +759,13 @@ 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,
> > + &used_buckets_list);
> >
> > pr_debug("Applying relocate section %u to %u\n", relsec,
> > sechdrs[relsec].sh_info);
> > @@ -823,14 +846,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;
> > }
> >
> > ---
> > base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
> > change-id: 20231120-module_linking_freeing-2b5a3b255b5e
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] riscv: Safely remove entries from relocation list
2023-11-21 22:50 [PATCH v2] riscv: Safely remove entries from relocation list Charlie Jenkins
2023-11-22 0:04 ` Samuel Holland
@ 2023-11-22 1:22 ` Ron Economos
1 sibling, 0 replies; 4+ messages in thread
From: Ron Economos @ 2023-11-22 1:22 UTC (permalink / raw)
To: Charlie Jenkins, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: linux-riscv, linux-kernel
On 11/21/23 2:50 PM, Charlie Jenkins 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>
> ---
> 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
> ---
> arch/riscv/kernel/module.c | 76 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index 56a8c78e9e21..f53e82b70dff 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -40,14 +40,17 @@ 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,
> + struct list_head *used_buckets_list);
> +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 +607,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 +629,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 +660,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;
> @@ -667,7 +681,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
>
> hash = hash_min((uintptr_t)location, hashtable_bits);
>
> - current_head = &relocation_hashtable[hash];
> + current_head = &((*relocation_hashtable)[hash]);
>
> /* Find matching location (if any) */
> bool found = false;
> @@ -693,7 +707,7 @@ int add_relocation_to_accumulate(struct module *me, int type, void *location,
> kmalloc(sizeof(struct used_bucket), GFP_KERNEL);
> 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 +718,10 @@ 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,
> + struct list_head *used_buckets_list)
> {
> /* Can safely assume that bits is not greater than sizeof(long) */
> unsigned long hashtable_size = roundup_pow_of_two(num_relocations);
> @@ -720,12 +737,12 @@ 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);
> + __hash_init(*relocation_hashtable, hashtable_size);
>
> - INIT_LIST_HEAD(&used_buckets_list);
> + INIT_LIST_HEAD(used_buckets_list);
>
> return hashtable_bits;
> }
> @@ -742,7 +759,13 @@ 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,
> + &used_buckets_list);
>
> pr_debug("Applying relocate section %u to %u\n", relsec,
> sechdrs[relsec].sh_info);
> @@ -823,14 +846,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;
> }
>
> ---
> base-commit: 98b1cc82c4affc16f5598d4fa14b1858671b2263
> change-id: 20231120-module_linking_freeing-2b5a3b255b5e
That did the trick. Booted ten times on Unmatched with no problems.
Tested-by: Ron Economos <re@w6rz.net>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-22 3:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-21 22:50 [PATCH v2] riscv: Safely remove entries from relocation list Charlie Jenkins
2023-11-22 0:04 ` Samuel Holland
2023-11-22 3:41 ` Charlie Jenkins
2023-11-22 1:22 ` Ron Economos
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox