[v1] dma: map_benchmark: turn dma_sg_map_param buf into a flexible array

[PATCH] dma: map_benchmark: turn dma_sg_map_param buf into a flexible array

Posted by Rosen Penev 1 week, 6 days ago

The buf pointer was kmalloc_array()'d immediately after the parent
struct allocation, with the count (granule, validated to 1..1024 by
the ioctl) trivially available beforehand.  Move buf to the struct
tail as a flexible array member and fold the two allocations into a
single kzalloc_flex(), dropping the kfree(params->buf) in both the
prepare error path and unprepare.

Add __counted_by for extra runtime analysis.

Assisted-by: Claude:Opus-4.7
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 kernel/dma/map_benchmark.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
index 29eeb5fdf199..a65da5c7710c 100644
--- a/kernel/dma/map_benchmark.c
+++ b/kernel/dma/map_benchmark.c
@@ -121,35 +121,35 @@ static struct map_benchmark_ops dma_single_map_benchmark_ops = {
 struct dma_sg_map_param {
 	struct sg_table sgt;
 	struct device *dev;
-	void **buf;
 	u32 npages;
 	u32 dma_dir;
+	void *buf[] __counted_by(npages);
 };
 
 static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
 {
+	struct dma_sg_map_param *params;
 	struct scatterlist *sg;
+	u32 npages;
 	int i;
 
-	struct dma_sg_map_param *params = kzalloc(sizeof(*params), GFP_KERNEL);
-
-	if (!params)
-		return NULL;
 	/*
 	 * Set the number of scatterlist entries based on the granule.
 	 * In SG mode, 'granule' represents the number of scatterlist entries.
 	 * Each scatterlist entry corresponds to a single page.
 	 */
-	params->npages = map->bparam.granule;
+	npages = map->bparam.granule;
+
+	params = kzalloc_flex(*params, buf, npages);
+	if (!params)
+		return NULL;
+
+	params->npages = npages;
 	params->dma_dir = map->bparam.dma_dir;
 	params->dev = map->dev;
-	params->buf = kmalloc_array(params->npages, sizeof(*params->buf),
-				    GFP_KERNEL);
-	if (!params->buf)
-		goto out;
 
-	if (sg_alloc_table(&params->sgt, params->npages, GFP_KERNEL))
-		goto free_buf;
+	if (sg_alloc_table(&params->sgt, npages, GFP_KERNEL))
+		goto free_params;
 
 	for_each_sgtable_sg(&params->sgt, sg, i) {
 		params->buf[i] = (void *)__get_free_page(GFP_KERNEL);
@@ -166,9 +166,7 @@ static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
 		free_page((unsigned long)params->buf[i]);
 
 	sg_free_table(&params->sgt);
-free_buf:
-	kfree(params->buf);
-out:
+free_params:
 	kfree(params);
 	return NULL;
 }
@@ -183,7 +181,6 @@ static void dma_sg_map_benchmark_unprepare(void *mparam)
 
 	sg_free_table(&params->sgt);
 
-	kfree(params->buf);
 	kfree(params);
 }
 
-- 
2.54.0

Re: [PATCH] dma: map_benchmark: turn dma_sg_map_param buf into a flexible array

Posted by Kees Cook 6 days, 5 hours ago

On Mon, May 25, 2026 at 03:06:28PM -0700, Rosen Penev wrote:
> The buf pointer was kmalloc_array()'d immediately after the parent
> struct allocation, with the count (granule, validated to 1..1024 by
> the ioctl) trivially available beforehand.  Move buf to the struct
> tail as a flexible array member and fold the two allocations into a
> single kzalloc_flex(), dropping the kfree(params->buf) in both the
> prepare error path and unprepare.
> 
> Add __counted_by for extra runtime analysis.
> 
> Assisted-by: Claude:Opus-4.7
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  kernel/dma/map_benchmark.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> index 29eeb5fdf199..a65da5c7710c 100644
> --- a/kernel/dma/map_benchmark.c
> +++ b/kernel/dma/map_benchmark.c
> @@ -121,35 +121,35 @@ static struct map_benchmark_ops dma_single_map_benchmark_ops = {
>  struct dma_sg_map_param {
>  	struct sg_table sgt;
>  	struct device *dev;
> -	void **buf;
>  	u32 npages;
>  	u32 dma_dir;
> +	void *buf[] __counted_by(npages);
>  };
>  
>  static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
>  {
> +	struct dma_sg_map_param *params;
>  	struct scatterlist *sg;
> +	u32 npages;
>  	int i;
>  
> -	struct dma_sg_map_param *params = kzalloc(sizeof(*params), GFP_KERNEL);
> -
> -	if (!params)
> -		return NULL;
>  	/*
>  	 * Set the number of scatterlist entries based on the granule.
>  	 * In SG mode, 'granule' represents the number of scatterlist entries.
>  	 * Each scatterlist entry corresponds to a single page.
>  	 */
> -	params->npages = map->bparam.granule;
> +	npages = map->bparam.granule;
> +
> +	params = kzalloc_flex(*params, buf, npages);
> +	if (!params)
> +		return NULL;
> +
> +	params->npages = npages;
>  	params->dma_dir = map->bparam.dma_dir;
>  	params->dev = map->dev;
> -	params->buf = kmalloc_array(params->npages, sizeof(*params->buf),
> -				    GFP_KERNEL);
> -	if (!params->buf)
> -		goto out;
>  
> -	if (sg_alloc_table(&params->sgt, params->npages, GFP_KERNEL))
> -		goto free_buf;
> +	if (sg_alloc_table(&params->sgt, npages, GFP_KERNEL))

nit: I think it's better to use the params->npages here just because it
is obviously tied to params->sgt, and reduces code churn for this patch
(i.e. that line would be left alone).

Otherwise looks good.

-Kees

> +		goto free_params;
>  
>  	for_each_sgtable_sg(&params->sgt, sg, i) {
>  		params->buf[i] = (void *)__get_free_page(GFP_KERNEL);
> @@ -166,9 +166,7 @@ static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
>  		free_page((unsigned long)params->buf[i]);
>  
>  	sg_free_table(&params->sgt);
> -free_buf:
> -	kfree(params->buf);
> -out:
> +free_params:
>  	kfree(params);
>  	return NULL;
>  }
> @@ -183,7 +181,6 @@ static void dma_sg_map_benchmark_unprepare(void *mparam)
>  
>  	sg_free_table(&params->sgt);
>  
> -	kfree(params->buf);
>  	kfree(params);
>  }
>  
> -- 
> 2.54.0
> 

-- 
Kees Cook

Re: [PATCH] dma: map_benchmark: turn dma_sg_map_param buf into a flexible array

Posted by Rosen Penev 5 days, 18 hours ago

On Tue, Jun 2, 2026 at 9:52 AM Kees Cook <kees@kernel.org> wrote:
>
> On Mon, May 25, 2026 at 03:06:28PM -0700, Rosen Penev wrote:
> > The buf pointer was kmalloc_array()'d immediately after the parent
> > struct allocation, with the count (granule, validated to 1..1024 by
> > the ioctl) trivially available beforehand.  Move buf to the struct
> > tail as a flexible array member and fold the two allocations into a
> > single kzalloc_flex(), dropping the kfree(params->buf) in both the
> > prepare error path and unprepare.
> >
> > Add __counted_by for extra runtime analysis.
> >
> > Assisted-by: Claude:Opus-4.7
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> >  kernel/dma/map_benchmark.c | 29 +++++++++++++----------------
> >  1 file changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> > index 29eeb5fdf199..a65da5c7710c 100644
> > --- a/kernel/dma/map_benchmark.c
> > +++ b/kernel/dma/map_benchmark.c
> > @@ -121,35 +121,35 @@ static struct map_benchmark_ops dma_single_map_benchmark_ops = {
> >  struct dma_sg_map_param {
> >       struct sg_table sgt;
> >       struct device *dev;
> > -     void **buf;
> >       u32 npages;
> >       u32 dma_dir;
> > +     void *buf[] __counted_by(npages);
> >  };
> >
> >  static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
> >  {
> > +     struct dma_sg_map_param *params;
> >       struct scatterlist *sg;
> > +     u32 npages;
> >       int i;
> >
> > -     struct dma_sg_map_param *params = kzalloc(sizeof(*params), GFP_KERNEL);
> > -
> > -     if (!params)
> > -             return NULL;
> >       /*
> >        * Set the number of scatterlist entries based on the granule.
> >        * In SG mode, 'granule' represents the number of scatterlist entries.
> >        * Each scatterlist entry corresponds to a single page.
> >        */
> > -     params->npages = map->bparam.granule;
> > +     npages = map->bparam.granule;
> > +
> > +     params = kzalloc_flex(*params, buf, npages);
> > +     if (!params)
> > +             return NULL;
> > +
> > +     params->npages = npages;
> >       params->dma_dir = map->bparam.dma_dir;
> >       params->dev = map->dev;
> > -     params->buf = kmalloc_array(params->npages, sizeof(*params->buf),
> > -                                 GFP_KERNEL);
> > -     if (!params->buf)
> > -             goto out;
> >
> > -     if (sg_alloc_table(&params->sgt, params->npages, GFP_KERNEL))
> > -             goto free_buf;
> > +     if (sg_alloc_table(&params->sgt, npages, GFP_KERNEL))
>
> nit: I think it's better to use the params->npages here just because it
> is obviously tied to params->sgt, and reduces code churn for this patch
> (i.e. that line would be left alone).
Fixed. I only did it to reduce line length.
>
> Otherwise looks good.
>
> -Kees
>
> > +             goto free_params;
> >
> >       for_each_sgtable_sg(&params->sgt, sg, i) {
> >               params->buf[i] = (void *)__get_free_page(GFP_KERNEL);
> > @@ -166,9 +166,7 @@ static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
> >               free_page((unsigned long)params->buf[i]);
> >
> >       sg_free_table(&params->sgt);
> > -free_buf:
> > -     kfree(params->buf);
> > -out:
> > +free_params:
> >       kfree(params);
> >       return NULL;
> >  }
> > @@ -183,7 +181,6 @@ static void dma_sg_map_benchmark_unprepare(void *mparam)
> >
> >       sg_free_table(&params->sgt);
> >
> > -     kfree(params->buf);
> >       kfree(params);
> >  }
> >
> > --
> > 2.54.0
> >
>
> --
> Kees Cook

Re: [PATCH] dma: map_benchmark: turn dma_sg_map_param buf into a flexible array

Posted by Qinxin Xia 6 days, 6 hours ago


在 2026/5/26 6:06, Rosen Penev 写道:
> The buf pointer was kmalloc_array()'d immediately after the parent
> struct allocation, with the count (granule, validated to 1..1024 by
> the ioctl) trivially available beforehand.  Move buf to the struct
> tail as a flexible array member and fold the two allocations into a
> single kzalloc_flex(), dropping the kfree(params->buf) in both the
> prepare error path and unprepare.
> 
> Add __counted_by for extra runtime analysis.
> 
> Assisted-by: Claude:Opus-4.7
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   kernel/dma/map_benchmark.c | 29 +++++++++++++----------------
>   1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> index 29eeb5fdf199..a65da5c7710c 100644
> --- a/kernel/dma/map_benchmark.c
> +++ b/kernel/dma/map_benchmark.c
> @@ -121,35 +121,35 @@ static struct map_benchmark_ops dma_single_map_benchmark_ops = {
>   struct dma_sg_map_param {
>   	struct sg_table sgt;
>   	struct device *dev;
> -	void **buf;
>   	u32 npages;
>   	u32 dma_dir;
> +	void *buf[] __counted_by(npages);
>   };
>   
>   static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
>   {
> +	struct dma_sg_map_param *params;
>   	struct scatterlist *sg;
> +	u32 npages;
>   	int i;
>   
> -	struct dma_sg_map_param *params = kzalloc(sizeof(*params), GFP_KERNEL);
> -
> -	if (!params)
> -		return NULL;
>   	/*
>   	 * Set the number of scatterlist entries based on the granule.
>   	 * In SG mode, 'granule' represents the number of scatterlist entries.
>   	 * Each scatterlist entry corresponds to a single page.
>   	 */
> -	params->npages = map->bparam.granule;
> +	npages = map->bparam.granule;
> +
> +	params = kzalloc_flex(*params, buf, npages);
> +	if (!params)
> +		return NULL;
> +
> +	params->npages = npages;
>   	params->dma_dir = map->bparam.dma_dir;
>   	params->dev = map->dev;
> -	params->buf = kmalloc_array(params->npages, sizeof(*params->buf),
> -				    GFP_KERNEL);
> -	if (!params->buf)
> -		goto out;
>   
> -	if (sg_alloc_table(&params->sgt, params->npages, GFP_KERNEL))
> -		goto free_buf;
> +	if (sg_alloc_table(&params->sgt, npages, GFP_KERNEL))
> +		goto free_params;
>   
>   	for_each_sgtable_sg(&params->sgt, sg, i) {
>   		params->buf[i] = (void *)__get_free_page(GFP_KERNEL);
> @@ -166,9 +166,7 @@ static void *dma_sg_map_benchmark_prepare(struct map_benchmark_data *map)
>   		free_page((unsigned long)params->buf[i]);
>   
>   	sg_free_table(&params->sgt);
> -free_buf:
> -	kfree(params->buf);
> -out:
> +free_params:
>   	kfree(params);
>   	return NULL;
>   }
> @@ -183,7 +181,6 @@ static void dma_sg_map_benchmark_unprepare(void *mparam)
>   
>   	sg_free_table(&params->sgt);
>   
> -	kfree(params->buf);
>   	kfree(params);
>   }
>   

Thanks for the patch.

Sorry for the delay — your email was unfortunately flagged as phishing
and almost missed. I suspect the issue was the Cc list containing
descriptive text.

It's a nice clean code.
I've tested it on Kunpeng 920, and it looks good.
Feel free to add my Reviewed-by:
Reviewed-by: Qinxin Xia <xiaqinxin@huawei.com>

-- 
Thanks,
Qinxin