[PATCH] sparc: Replace one-element array with flexible array member

[PATCH] sparc: Replace one-element array with flexible array member

[Thorsten Blum]

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(-)

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 "
-- 
2.47.0

Re: [PATCH] sparc: Replace one-element array with flexible array member

[Gustavo A. R. Silva]

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

Re: [PATCH] sparc: Replace one-element array with flexible array member

[Thorsten Blum]

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

Re: [PATCH] sparc: Replace one-element array with flexible array member

[Gustavo A. R. Silva]

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