Messages in this thread | | | Date | Tue, 21 Nov 2023 22:41:26 -0500 | From | Charlie Jenkins <> | Subject | Re: [PATCH v2] riscv: Safely remove entries from relocation list |
| |
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 >
| |