| Message ID | 20241111200155.164621-3-thorsten.blum@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sparc: Replace one-element array with flexible array member | expand |
On 11/11/24 14:01, Thorsten Blum wrote: > Replace the deprecated one-element array with a modern flexible array > member in the struct hvtramp_descr. > > Additionally, 15 unnecessary bytes are allocated for hdesc, but instead 15? unnecessary? > of fixing the parentheses in the open-coded version, use struct_size() > to calculate the correct number of bytes. > > Link: https://github.com/KSPP/linux/issues/79 > Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> > --- > arch/sparc/include/asm/hvtramp.h | 2 +- > arch/sparc/kernel/smp_64.c | 4 +--- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/arch/sparc/include/asm/hvtramp.h b/arch/sparc/include/asm/hvtramp.h > index 688ea43af0f5..ce2453ea4f2b 100644 > --- a/arch/sparc/include/asm/hvtramp.h > +++ b/arch/sparc/include/asm/hvtramp.h > @@ -17,7 +17,7 @@ struct hvtramp_descr { > __u64 fault_info_va; > __u64 fault_info_pa; > __u64 thread_reg; > - struct hvtramp_mapping maps[1]; > + struct hvtramp_mapping maps[]; > }; It seems this struct is a candidate for `__counted_by()` > > void hv_cpu_startup(unsigned long hvdescr_pa); > diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c > index e40c395db202..24d980220bf1 100644 > --- a/arch/sparc/kernel/smp_64.c > +++ b/arch/sparc/kernel/smp_64.c > @@ -297,9 +297,7 @@ static void ldom_startcpu_cpuid(unsigned int cpu, unsigned long thread_reg, > unsigned long hv_err; > int i; > > - hdesc = kzalloc(sizeof(*hdesc) + > - (sizeof(struct hvtramp_mapping) * > - num_kernel_image_mappings - 1), > + hdesc = kzalloc(struct_size(hdesc, maps, num_kernel_image_mappings - 1), > GFP_KERNEL); Now the code is broken because it's allocating `num_kernel_image_mappings - 1` elements instead of `num_kernel_image_mappings`. -- Gustavo
On 11. Nov 2024, at 21:20, Gustavo A. R. Silva wrote: > On 11/11/24 14:01, Thorsten Blum wrote: >> Replace the deprecated one-element array with a modern flexible array >> member in the struct hvtramp_descr. >> Additionally, 15 unnecessary bytes are allocated for hdesc, but instead > > 15? unnecessary? hvtramp_mapping is 16 bytes and the size is calculated as follows: (16 * num_kernel_image_mappings - 1) which is 15 bytes too many for any number of mappings because hdesc includes the first map. It probably should have been: 16 * (num_kernel_image_mappings - 1) unless I'm missing something. > It seems this struct is a candidate for `__counted_by()` Yes, but sparc doesn't seem to support it? > Now the code is broken because it's allocating `num_kernel_image_mappings - 1` > elements instead of `num_kernel_image_mappings`. Ah sorry, missed that and will fix in v2 shortly. Thanks, Thorsten
On 11/11/24 14:45, Thorsten Blum wrote: > On 11. Nov 2024, at 21:20, Gustavo A. R. Silva wrote: >> On 11/11/24 14:01, Thorsten Blum wrote: >>> Replace the deprecated one-element array with a modern flexible array >>> member in the struct hvtramp_descr. >>> Additionally, 15 unnecessary bytes are allocated for hdesc, but instead >> >> 15? unnecessary? > > hvtramp_mapping is 16 bytes and the size is calculated as follows: > > (16 * num_kernel_image_mappings - 1) > > which is 15 bytes too many for any number of mappings because hdesc > includes the first map. It probably should have been: > > 16 * (num_kernel_image_mappings - 1) Ah yes, that opening parenthesis before `sizeof(struct hvtramp_mapping)` was misplaced. > > unless I'm missing something. > >> It seems this struct is a candidate for `__counted_by()` > > Yes, but sparc doesn't seem to support it? > >> Now the code is broken because it's allocating `num_kernel_image_mappings - 1` >> elements instead of `num_kernel_image_mappings`. > > Ah sorry, missed that and will fix in v2 shortly. Thanks -- Gustavo
diff --git a/arch/sparc/include/asm/hvtramp.h b/arch/sparc/include/asm/hvtramp.h index 688ea43af0f5..ce2453ea4f2b 100644 --- a/arch/sparc/include/asm/hvtramp.h +++ b/arch/sparc/include/asm/hvtramp.h @@ -17,7 +17,7 @@ struct hvtramp_descr { __u64 fault_info_va; __u64 fault_info_pa; __u64 thread_reg; - struct hvtramp_mapping maps[1]; + struct hvtramp_mapping maps[]; }; void hv_cpu_startup(unsigned long hvdescr_pa); diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c index e40c395db202..24d980220bf1 100644 --- a/arch/sparc/kernel/smp_64.c +++ b/arch/sparc/kernel/smp_64.c @@ -297,9 +297,7 @@ static void ldom_startcpu_cpuid(unsigned int cpu, unsigned long thread_reg, unsigned long hv_err; int i; - hdesc = kzalloc(sizeof(*hdesc) + - (sizeof(struct hvtramp_mapping) * - num_kernel_image_mappings - 1), + hdesc = kzalloc(struct_size(hdesc, maps, num_kernel_image_mappings - 1), GFP_KERNEL); if (!hdesc) { printk(KERN_ERR "ldom_startcpu_cpuid: Cannot allocate "
Replace the deprecated one-element array with a modern flexible array member in the struct hvtramp_descr. Additionally, 15 unnecessary bytes are allocated for hdesc, but instead of fixing the parentheses in the open-coded version, use struct_size() to calculate the correct number of bytes. Link: https://github.com/KSPP/linux/issues/79 Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev> --- arch/sparc/include/asm/hvtramp.h | 2 +- arch/sparc/kernel/smp_64.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-)