[RFC PATCH v3] RDMA/siw: use kzalloc_flex

[RFC PATCH v3] RDMA/siw: use kzalloc_flex

[bernard.metzler]

From: Bernard Metzler <bernard.metzler@linux.dev>

Simplify umem allocation by using flexible array member.
Add __counted_by to get extra runtime analysis.

Suggested-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Bernard Metzler <bernard.metzler@linux.dev>

---
v3:
Considering comments from sashiko.dev to v2:
- To minimize memory footprint of user page list, revert array
  allocation to original kzalloc_objs(),
- Revert siw_get_upage() to original bounds check.

v2:
1. Considering comments from sashiko.dev to original patch regarding:
	- avoiding allocation of extra empty page chunk entry,
	  if number of pages is an exact multiplier
	  of PAGES_PER_CHUNK,
	- fix potential signed index overflow case for
	  page list generation and access,
	- restrict page list access in siw_get_upage() to
	  allocated length.

2. Extend flexible array allocation to page list.

3. Remove useless PAGE_CHUNK_SIZE definition.
---
 drivers/infiniband/sw/siw/siw.h     |  5 +--
 drivers/infiniband/sw/siw/siw_mem.c | 54 ++++++++++++++---------------
 drivers/infiniband/sw/siw/siw_mem.h |  3 +-
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index f5fd71717b80..dbc998248b1e 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -119,9 +119,10 @@ struct siw_page_chunk {
 
 struct siw_umem {
 	struct ib_umem *base_mem;
-	struct siw_page_chunk *page_chunk;
-	int num_pages;
+	unsigned int num_pages;
+	unsigned int num_chunks;
 	u64 fp_addr; /* First page base address */
+	struct siw_page_chunk page_chunk[] __counted_by(num_chunks);
 };
 
 struct siw_pble {
diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index 98c802b3ed72..88ec3cacfa00 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -41,16 +41,14 @@ struct siw_mem *siw_mem_id2obj(struct siw_device *sdev, int stag_index)
 
 void siw_umem_release(struct siw_umem *umem)
 {
-	int i, num_pages = umem->num_pages;
+	unsigned int i, num_chunks = umem->num_chunks;
 
 	if (umem->base_mem)
 		ib_umem_release(umem->base_mem);
 
-	for (i = 0; num_pages > 0; i++) {
+	for (i = 0; i < num_chunks; i++)
 		kfree(umem->page_chunk[i].plist);
-		num_pages -= PAGES_PER_CHUNK;
-	}
-	kfree(umem->page_chunk);
+
 	kfree(umem);
 }
 
@@ -188,7 +186,7 @@ int siw_check_mem(struct ib_pd *pd, struct siw_mem *mem, u64 addr,
  * lookup is being done and mem is not released it check fails.
  */
 int siw_check_sge(struct ib_pd *pd, struct siw_sge *sge, struct siw_mem *mem[],
-		  enum ib_access_flags perms, u32 off, int len)
+		  enum ib_access_flags perms, u32 off, u32 len)
 {
 	struct siw_device *sdev = to_siw_dev(pd->device);
 	struct siw_mem *new = NULL;
@@ -338,25 +336,20 @@ struct siw_umem *siw_umem_get(struct ib_device *base_dev, u64 start,
 	struct sg_page_iter sg_iter;
 	struct sg_table *sgt;
 	u64 first_page_va;
-	int num_pages, num_chunks, i, rv = 0;
+	unsigned int num_pages, num_chunks, i;
+	int rv = 0;
 
 	if (!len)
 		return ERR_PTR(-EINVAL);
 
 	first_page_va = start & PAGE_MASK;
 	num_pages = PAGE_ALIGN(start + len - first_page_va) >> PAGE_SHIFT;
-	num_chunks = (num_pages >> CHUNK_SHIFT) + 1;
+	num_chunks = ((num_pages - 1) >> CHUNK_SHIFT) + 1;
 
-	umem = kzalloc_obj(*umem);
+	umem = kzalloc_flex(*umem, page_chunk, num_chunks);
 	if (!umem)
 		return ERR_PTR(-ENOMEM);
 
-	umem->page_chunk =
-		kzalloc_objs(struct siw_page_chunk, num_chunks);
-	if (!umem->page_chunk) {
-		rv = -ENOMEM;
-		goto err_out;
-	}
 	base_mem = ib_umem_get(base_dev, start, len, rights);
 	if (IS_ERR(base_mem)) {
 		rv = PTR_ERR(base_mem);
@@ -365,33 +358,38 @@ struct siw_umem *siw_umem_get(struct ib_device *base_dev, u64 start,
 	}
 	umem->fp_addr = first_page_va;
 	umem->base_mem = base_mem;
+	umem->num_pages = num_pages;
 
 	sgt = &base_mem->sgt_append.sgt;
 	__sg_page_iter_start(&sg_iter, sgt->sgl, sgt->orig_nents, 0);
 
-	if (!__sg_page_iter_next(&sg_iter)) {
-		rv = -EINVAL;
-		goto err_out;
-	}
-	for (i = 0; num_pages > 0; i++) {
-		int nents = min_t(int, num_pages, PAGES_PER_CHUNK);
-		struct page **plist =
-			kzalloc_objs(struct page *, nents);
+	for (i = 0; i < num_chunks; i++) {
+		struct page **plist;
+		unsigned int pix, nents = min(num_pages, PAGES_PER_CHUNK);
 
+		plist = kzalloc_objs(struct page *, nents);
 		if (!plist) {
 			rv = -ENOMEM;
 			goto err_out;
 		}
 		umem->page_chunk[i].plist = plist;
-		while (nents--) {
-			*plist = sg_page_iter_page(&sg_iter);
-			umem->num_pages++;
-			num_pages--;
-			plist++;
+
+		for (pix = 0; pix < nents; pix++) {
 			if (!__sg_page_iter_next(&sg_iter))
 				break;
+			plist[pix] = sg_page_iter_page(&sg_iter);
+			num_pages--;
 		}
 	}
+	if (unlikely(num_pages)) {
+		/*
+		 * Unexpected size of sg list provided by ib_umem_get()
+		 */
+		siw_dbg(base_dev, "Short SG list, missing %u pages\n",
+			num_pages);
+		rv = -EINVAL;
+		goto err_out;
+	}
 	return umem;
 err_out:
 	siw_umem_release(umem);
diff --git a/drivers/infiniband/sw/siw/siw_mem.h b/drivers/infiniband/sw/siw/siw_mem.h
index 8e769d30e2ac..2dff6640da5f 100644
--- a/drivers/infiniband/sw/siw/siw_mem.h
+++ b/drivers/infiniband/sw/siw/siw_mem.h
@@ -17,7 +17,7 @@ int siw_check_mem(struct ib_pd *pd, struct siw_mem *mem, u64 addr,
 		  enum ib_access_flags perms, int len);
 int siw_check_sge(struct ib_pd *pd, struct siw_sge *sge,
 		  struct siw_mem *mem[], enum ib_access_flags perms,
-		  u32 off, int len);
+		  u32 off, u32 len);
 void siw_wqe_put_mem(struct siw_wqe *wqe, enum siw_opcode op);
 int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
 		   u64 start, u64 len, int rights);
@@ -45,7 +45,6 @@ static inline void siw_unref_mem_sgl(struct siw_mem **mem, unsigned int num_sge)
 #define CHUNK_SHIFT 9 /* sets number of pages per chunk */
 #define PAGES_PER_CHUNK (_AC(1, UL) << CHUNK_SHIFT)
 #define CHUNK_MASK (~(PAGES_PER_CHUNK - 1))
-#define PAGE_CHUNK_SIZE (PAGES_PER_CHUNK * sizeof(struct page *))
 
 /*
  * siw_get_upage()
-- 
2.50.0

Re: [RFC PATCH v3] RDMA/siw: use kzalloc_flex

[Bernard Metzler]

Before bothering the list with v4,5,6 patches, I hereby
reach out for advise. Please see below my questions related
to recent findings of sashiko, which are really useful
and very relevant.


On 11.05.2026 16:11, bernard.metzler@linux.dev wrote:
> From: Bernard Metzler <bernard.metzler@linux.dev>
> 
> Simplify umem allocation by using flexible array member.
> Add __counted_by to get extra runtime analysis.
> 
> Suggested-by: Rosen Penev <rosenp@gmail.com>
> Signed-off-by: Bernard Metzler <bernard.metzler@linux.dev>
> 
> ---
> v3:
> Considering comments from sashiko.dev to v2:
> - To minimize memory footprint of user page list, revert array
>    allocation to original kzalloc_objs(),
> - Revert siw_get_upage() to original bounds check.
> 
> v2:
> 1. Considering comments from sashiko.dev to original patch regarding:
> 	- avoiding allocation of extra empty page chunk entry,
> 	  if number of pages is an exact multiplier
> 	  of PAGES_PER_CHUNK,
> 	- fix potential signed index overflow case for
> 	  page list generation and access,
> 	- restrict page list access in siw_get_upage() to
> 	  allocated length.
> 
> 2. Extend flexible array allocation to page list.
> 
> 3. Remove useless PAGE_CHUNK_SIZE definition.
> ---
>   drivers/infiniband/sw/siw/siw.h     |  5 +--
>   drivers/infiniband/sw/siw/siw_mem.c | 54 ++++++++++++++---------------
>   drivers/infiniband/sw/siw/siw_mem.h |  3 +-
>   3 files changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
> index f5fd71717b80..dbc998248b1e 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -119,9 +119,10 @@ struct siw_page_chunk {
>   
>   struct siw_umem {
>   	struct ib_umem *base_mem;
> -	struct siw_page_chunk *page_chunk;
> -	int num_pages;
> +	unsigned int num_pages;
> +	unsigned int num_chunks;
>   	u64 fp_addr; /* First page base address */
> +	struct siw_page_chunk page_chunk[] __counted_by(num_chunks);
>   };
>   
>   struct siw_pble {
> diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
> index 98c802b3ed72..88ec3cacfa00 100644
> --- a/drivers/infiniband/sw/siw/siw_mem.c
> +++ b/drivers/infiniband/sw/siw/siw_mem.c
> @@ -41,16 +41,14 @@ struct siw_mem *siw_mem_id2obj(struct siw_device *sdev, int stag_index)
>   
>   void siw_umem_release(struct siw_umem *umem)
>   {
> -	int i, num_pages = umem->num_pages;
> +	unsigned int i, num_chunks = umem->num_chunks;
>   
>   	if (umem->base_mem)
>   		ib_umem_release(umem->base_mem);
>   
> -	for (i = 0; num_pages > 0; i++) {
> +	for (i = 0; i < num_chunks; i++)
>   		kfree(umem->page_chunk[i].plist);
> -		num_pages -= PAGES_PER_CHUNK;
> -	}
> -	kfree(umem->page_chunk);
> +
>   	kfree(umem);
>   }
>   
> @@ -188,7 +186,7 @@ int siw_check_mem(struct ib_pd *pd, struct siw_mem *mem, u64 addr,
>    * lookup is being done and mem is not released it check fails.
>    */
>   int siw_check_sge(struct ib_pd *pd, struct siw_sge *sge, struct siw_mem *mem[],
> -		  enum ib_access_flags perms, u32 off, int len)
> +		  enum ib_access_flags perms, u32 off, u32 len)
>   {
>   	struct siw_device *sdev = to_siw_dev(pd->device);
>   	struct siw_mem *new = NULL;
> @@ -338,25 +336,20 @@ struct siw_umem *siw_umem_get(struct ib_device *base_dev, u64 start,
>   	struct sg_page_iter sg_iter;
>   	struct sg_table *sgt;
>   	u64 first_page_va;
> -	int num_pages, num_chunks, i, rv = 0;
> +	unsigned int num_pages, num_chunks, i;
> +	int rv = 0;
>   
>   	if (!len)
>   		return ERR_PTR(-EINVAL);
>   
>   	first_page_va = start & PAGE_MASK;
>   	num_pages = PAGE_ALIGN(start + len - first_page_va) >> PAGE_SHIFT;
> -	num_chunks = (num_pages >> CHUNK_SHIFT) + 1;
> +	num_chunks = ((num_pages - 1) >> CHUNK_SHIFT) + 1;
>   
> -	umem = kzalloc_obj(*umem);
> +	umem = kzalloc_flex(*umem, page_chunk, num_chunks);
>   	if (!umem)
>   		return ERR_PTR(-ENOMEM);
>   
I got the following comment from sashiko:

"Does umem->num_chunks need to be explicitly initialized here?"

sashiko correctly points out that only newer compilers do that
initialization (I checked: only supported since gcc >= 15,
clang >= 19).

The patch intentionally does not set that value, since I felt
old compilers will not be used for v7 kernels. Shall we better
do the redundant assignment? This maybe is a general question for
the new alloc_flex api.


> -	umem->page_chunk =
> -		kzalloc_objs(struct siw_page_chunk, num_chunks);
> -	if (!umem->page_chunk) {
> -		rv = -ENOMEM;
> -		goto err_out;
> -	}
>   	base_mem = ib_umem_get(base_dev, start, len, rights);
>   	if (IS_ERR(base_mem)) {
>   		rv = PTR_ERR(base_mem);
> @@ -365,33 +358,38 @@ struct siw_umem *siw_umem_get(struct ib_device *base_dev, u64 start,
>   	}
>   	umem->fp_addr = first_page_va;
>   	umem->base_mem = base_mem;
> +	umem->num_pages = num_pages;
>   
>   	sgt = &base_mem->sgt_append.sgt;
>   	__sg_page_iter_start(&sg_iter, sgt->sgl, sgt->orig_nents, 0);
>   
> -	if (!__sg_page_iter_next(&sg_iter)) {
> -		rv = -EINVAL;
> -		goto err_out;
> -	}
> -	for (i = 0; num_pages > 0; i++) {
> -		int nents = min_t(int, num_pages, PAGES_PER_CHUNK);
> -		struct page **plist =
> -			kzalloc_objs(struct page *, nents);
> +	for (i = 0; i < num_chunks; i++) {
> +		struct page **plist;
> +		unsigned int pix, nents = min(num_pages, PAGES_PER_CHUNK);
>   
> +		plist = kzalloc_objs(struct page *, nents);
>   		if (!plist) {
>   			rv = -ENOMEM;
>   			goto err_out;
>   		}
>   		umem->page_chunk[i].plist = plist;
> -		while (nents--) {
> -			*plist = sg_page_iter_page(&sg_iter);
> -			umem->num_pages++;
> -			num_pages--;
> -			plist++;
> +
> +		for (pix = 0; pix < nents; pix++) {
>   			if (!__sg_page_iter_next(&sg_iter))
>   				break;
> +			plist[pix] = sg_page_iter_page(&sg_iter);
> +			num_pages--;
>   		}
>   	}
> +	if (unlikely(num_pages)) {
> +		/*
> +		 * Unexpected size of sg list provided by ib_umem_get()
> +		 */
> +		siw_dbg(base_dev, "Short SG list, missing %u pages\n",
> +			num_pages);
> +		rv = -EINVAL;
> +		goto err_out;
> +	}
>   	return umem;
>   err_out:
>   	siw_umem_release(umem);
> diff --git a/drivers/infiniband/sw/siw/siw_mem.h b/drivers/infiniband/sw/siw/siw_mem.h
> index 8e769d30e2ac..2dff6640da5f 100644
> --- a/drivers/infiniband/sw/siw/siw_mem.h
> +++ b/drivers/infiniband/sw/siw/siw_mem.h
> @@ -17,7 +17,7 @@ int siw_check_mem(struct ib_pd *pd, struct siw_mem *mem, u64 addr,
>   		  enum ib_access_flags perms, int len);
>   int siw_check_sge(struct ib_pd *pd, struct siw_sge *sge,
>   		  struct siw_mem *mem[], enum ib_access_flags perms,
> -		  u32 off, int len);
> +		  u32 off, u32 len);

Sashiko says:

"The parameter len in siw_check_sge() is changed to u32, but it looks like
siw_check_mem() still takes an int for its len parameter."


This is correct, but shall I correct code not related to that
kzalloc_flex patch? I propose sending an extra patch fixing that
broken signed buffer access len/offset treatment all over driver code
in an extra patch. It is indeed broken and a great finding, but I'd
like to keep things separately.

Thanks very much!
Bernard.

>   void siw_wqe_put_mem(struct siw_wqe *wqe, enum siw_opcode op);
>   int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
>   		   u64 start, u64 len, int rights);
> @@ -45,7 +45,6 @@ static inline void siw_unref_mem_sgl(struct siw_mem **mem, unsigned int num_sge)
>   #define CHUNK_SHIFT 9 /* sets number of pages per chunk */
>   #define PAGES_PER_CHUNK (_AC(1, UL) << CHUNK_SHIFT)
>   #define CHUNK_MASK (~(PAGES_PER_CHUNK - 1))
> -#define PAGE_CHUNK_SIZE (PAGES_PER_CHUNK * sizeof(struct page *))
>   
>   /*
>    * siw_get_upage()

Re: [RFC PATCH v3] RDMA/siw: use kzalloc_flex

[Jason Gunthorpe]

On Wed, May 13, 2026 at 04:17:03PM +0200, Bernard Metzler wrote:
> > @@ -338,25 +336,20 @@ struct siw_umem *siw_umem_get(struct ib_device *base_dev, u64 start,
> >   	struct sg_page_iter sg_iter;
> >   	struct sg_table *sgt;
> >   	u64 first_page_va;
> > -	int num_pages, num_chunks, i, rv = 0;
> > +	unsigned int num_pages, num_chunks, i;
> > +	int rv = 0;
> >   	if (!len)
> >   		return ERR_PTR(-EINVAL);
> >   	first_page_va = start & PAGE_MASK;
> >   	num_pages = PAGE_ALIGN(start + len - first_page_va) >> PAGE_SHIFT;
> > -	num_chunks = (num_pages >> CHUNK_SHIFT) + 1;
> > +	num_chunks = ((num_pages - 1) >> CHUNK_SHIFT) + 1;
> > -	umem = kzalloc_obj(*umem);
> > +	umem = kzalloc_flex(*umem, page_chunk, num_chunks);
> >   	if (!umem)
> >   		return ERR_PTR(-ENOMEM);
> I got the following comment from sashiko:
> 
> "Does umem->num_chunks need to be explicitly initialized here?"
> 
> sashiko correctly points out that only newer compilers do that
> initialization (I checked: only supported since gcc >= 15,
> clang >= 19).

Ignore this, kxalloc_flex should not be assumed to initialize anything
but 0. IDK why they put in the auto-initialize when it cannot be
relied on, security in depth I guess.

> > --- a/drivers/infiniband/sw/siw/siw_mem.h
> > +++ b/drivers/infiniband/sw/siw/siw_mem.h
> > @@ -17,7 +17,7 @@ int siw_check_mem(struct ib_pd *pd, struct siw_mem *mem, u64 addr,
> >   		  enum ib_access_flags perms, int len);
> >   int siw_check_sge(struct ib_pd *pd, struct siw_sge *sge,
> >   		  struct siw_mem *mem[], enum ib_access_flags perms,
> > -		  u32 off, int len);
> > +		  u32 off, u32 len);
> 
> Sashiko says:
> 
> "The parameter len in siw_check_sge() is changed to u32, but it looks like
> siw_check_mem() still takes an int for its len parameter."
> 
> 
> This is correct, but shall I correct code not related to that
> kzalloc_flex patch? I propose sending an extra patch fixing that
> broken signed buffer access len/offset treatment all over driver code
> in an extra patch. It is indeed broken and a great finding, but I'd
> like to keep things separately.

Seperate patch is fine

Jason

Re: [RFC PATCH v3] RDMA/siw: use kzalloc_flex

[Leon Romanovsky]

On Mon, 11 May 2026 16:11:49 +0200, bernard.metzler@linux.dev wrote:
> Simplify umem allocation by using flexible array member.
> Add __counted_by to get extra runtime analysis.

Applied, thanks!

[1/1] RDMA/siw: use kzalloc_flex
      https://git.kernel.org/rdma/rdma/c/79678bea399052

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>

Re: [RFC PATCH v3] RDMA/siw: use kzalloc_flex

[Bernard Metzler]

On 13.05.2026 19:45, Leon Romanovsky wrote:
> 
> On Mon, 11 May 2026 16:11:49 +0200, bernard.metzler@linux.dev wrote:
>> Simplify umem allocation by using flexible array member.
>> Add __counted_by to get extra runtime analysis.
> 
> Applied, thanks!
> 
> [1/1] RDMA/siw: use kzalloc_flex
>        https://git.kernel.org/rdma/rdma/c/79678bea399052
> 
> Best regards,
Hi Leon,

per Jason's suggestion, please amend the patch by
adding below line. We should NOT assume kzalloc_flex
to initialize anything but 0. So we shall set
umem->num_chunks explicitly.

Many thanks,
Bernard.
----

diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index 88ec3cacfa00..40455616d9ec 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -359,6 +359,7 @@ struct siw_umem *siw_umem_get(struct ib_device *base_dev, u64 start,
	umem->fp_addr = first_page_va;
	umem->base_mem = base_mem;
	umem->num_pages = num_pages;
+	umem->num_chunks = num_chunks;

	sgt = &base_mem->sgt_append.sgt;
	__sg_page_iter_start(&sg_iter, sgt->sgl, sgt->orig_nents, 0);

Re: [RFC PATCH v3] RDMA/siw: use kzalloc_flex

[Kees Cook]

On Wed, May 13, 2026 at 11:28:46AM -0300, Jason Gunthorpe wrote:
> Ignore this, kxalloc_flex should not be assumed to initialize anything
> but 0. IDK why they put in the auto-initialize when it cannot be
> relied on, security in depth I guess.

Correct, manual initialization is still always required. The auto-init
(when counted_by is supported) is to deal with adding counted_by to code
that doesn't initialize the counter before access. i.e. this code used
to work:

thing = kzalloc(sizeof(*thing) + n * sizeof(*thing->fam));
access(thing->fam);
thing->fam_count = n;

but if we add counted_by and use kzalloc_flex, suddenly it breaks (at
access() time) unless kzalloc_flex also performs the "fam_count = n"
at alloc time.

-- 
Kees Cook