Messages in this thread | | | Subject | Re: [PATCH v2] riscv: Safely remove entries from relocation list | From | Ron Economos <> | Date | Tue, 21 Nov 2023 17:22:30 -0800 |
| |
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>
| |