RDS: Fix memory leak in rds_rdma_extra_size()

PATCH In review Xiaobo Liu 3 participants 4 messages Apr 13 – Apr 16 1 message not in the archive

Subsystems (4)

Networking Network drivers InfiniBand RDS - reliable datagram sockets

lore

Share & tools

lore

Xiaobo Liu

author

2026-04-13 07:00 UTC #0 lore raw

PATCH v2 1/1 RDS: Fix memory leak in rds_rdma_extra_size()

Free iov->iov when copy_from_user() or page count validation fails in rds_rdma_extra_size().

This preserves the existing success path and avoids leaking the allocated iovec array on error.

Signed-off-by Xiaobo Liu <[email protected]>

---
 net/rds/rdma.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

net/rds/rdma.c +21 -7

index aa6465dc7..91a20c1e2 100644
@@ -560,6 +560,7 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 	struct rds_iovec *vec;
 	struct rds_iovec __user *local_vec;
 	int tot_pages = 0;
+	int ret = 0;
 	unsigned int nr_pages;
 	unsigned int i;

@@ -578,16 +579,20 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 	vec = &iov->iov[0];
 
 	if (copy_from_user(vec, local_vec, args->nr_local *
-			   sizeof(struct rds_iovec)))
-		return -EFAULT;
+			   sizeof(struct rds_iovec))) {
+		ret = -EFAULT;
+		goto out;
+	}
 	iov->len = args->nr_local;
 
 	/* figure out the number of pages in the vector */
 	for (i = 0; i < args->nr_local; i++, vec++) {
 
 		nr_pages = rds_pages_in_vec(vec);
-		if (nr_pages == 0)
-			return -EINVAL;
+		if (nr_pages == 0) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		tot_pages += nr_pages;

@@ -595,11 +600,20 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 		 * nr_pages for one entry is limited to (UINT_MAX>>PAGE_SHIFT)+1,
 		 * so tot_pages cannot overflow without first going negative.
 		 */
-		if (tot_pages < 0)
-			return -EINVAL;
+		if (tot_pages < 0) {
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
-	return tot_pages * sizeof(struct scatterlist);
+	ret = tot_pages * sizeof(struct scatterlist);
+
+out:
+	if (ret < 0) {
+		kfree(iov->iov);
+		iov->iov = NULL;
+	}
+	return ret;
 }
 
 /*

Signature · 1 line

2.34.1

Allison Henderson

maintainer Reviewed-by on v2 1/1

2026-04-14 00:15 UTC #1 lore raw

Quoted 7 lines · Xiaobo Liu

Free iov->iov when copy_from_user() or page count validation fails in rds_rdma_extra_size().
This preserves the existing success path and avoids leaking the allocated iovec array on error.
Signed-off-by Xiaobo Liu <[email protected]>

I think this looks good now. Thanks Xiaobo.

Reviewed-by Allison Henderson <[email protected]>

Quoted 65 lines

---
 net/rds/rdma.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

net/rds/rdma.c +21 -7

index aa6465dc7..91a20c1e2 100644
@@ -560,6 +560,7 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 	struct rds_iovec *vec;
 	struct rds_iovec __user *local_vec;
 	int tot_pages = 0;
+	int ret = 0;
 	unsigned int nr_pages;
 	unsigned int i;

@@ -578,16 +579,20 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 	vec = &iov->iov[0];
 
 	if (copy_from_user(vec, local_vec, args->nr_local *
-			   sizeof(struct rds_iovec)))
-		return -EFAULT;
+			   sizeof(struct rds_iovec))) {
+		ret = -EFAULT;
+		goto out;
+	}
 	iov->len = args->nr_local;
 
 	/* figure out the number of pages in the vector */
 	for (i = 0; i < args->nr_local; i++, vec++) {
 
 		nr_pages = rds_pages_in_vec(vec);
-		if (nr_pages == 0)
-			return -EINVAL;
+		if (nr_pages == 0) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		tot_pages += nr_pages;

@@ -595,11 +600,20 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 		 * nr_pages for one entry is limited to (UINT_MAX>>PAGE_SHIFT)+1,
 		 * so tot_pages cannot overflow without first going negative.
 		 */
-		if (tot_pages < 0)
-			return -EINVAL;
+		if (tot_pages < 0) {
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
-	return tot_pages * sizeof(struct scatterlist);
+	ret = tot_pages * sizeof(struct scatterlist);
+
+out:
+	if (ret < 0) {
+		kfree(iov->iov);
+		iov->iov = NULL;
+	}
+	return ret;
 }
 
 /*

Paolo Abeni

maintainer

2026-04-16 08:20 UTC #2 lore raw

In reply to Xiaobo Liu

Quoted 9 lines · Xiaobo Liu

net/rds/rdma.c +3 -2

@@ -595,11 +600,20 @@ int rds_rdma_extra_size(struct rds_rdma_args *args,
 		 * nr_pages for one entry is limited to (UINT_MAX>>PAGE_SHIFT)+1,
 		 * so tot_pages cannot overflow without first going negative.
 		 */
-		if (tot_pages < 0)
-			return -EINVAL;
+		if (tot_pages < 0) {
+			ret = -EINVAL;
+			goto out;

net/rds/rdma.c int rds_rdma_extra_size(struct rds_rdma_args *args, · line 614

Quoted 10 lines

net/rds/rdma.c +7 -1

+		}
 	}
 
-	return tot_pages * sizeof(struct scatterlist);
+	ret = tot_pages * sizeof(struct scatterlist);
+
+out:
+	if (ret < 0) {
+		kfree(iov->iov);
+		iov->iov = NULL;

Is this really needed?!? AFAICS rds_rdma_extra_size() is invoked only
via: rds_sendmsg() -> rds_rm_size() -> rds_rdma_extra_size(), and the rds_sendmsg() error path already frees any non NULL iov.

A message that is not in the archive (sent off-list or not yet received): CAJeqHv+kCScdMLYgOPG0TaRwTH5-Vo-=HEPs+oX24OprbmtbwA@mail.gmail.com

Allison Henderson

maintainer

2026-04-16 22:18 UTC #4 lore raw

Quoted 4 lines · Xiaobo Liu

The internal addition of kfree and setting the pointer to NULL in rds_rdma_extra_size makes the function more self‑consistent and secure. After applying this patch, kfree(NULL) in rds_sendmsg is also safe and will not cause a double‑free.

Hi Xiaobo,

Paolo makes a good point that I had missed in that rds_sendmsg owns the cleanup. So even though iov->iov isn't freed here, it isn't leaked either. Self-consistency is fair as a style point, but it's not strong enough to justify the change on its own since it isn't a bug fix. That said, thank you for taking the time to look at this area; we appreciate the effort to help track down and fix bugs.

Thanks,
Allison

Quoted 32 lines · On 4/16/2616:20 Paolo Abeni <[email protected]> wrote:

Quoted 3 lines · Xiaobo Liu
Review int rds_rdma_extra_size(struct rds_rdma_args
Quoted 1 line
(quoted diff) +0 -0
@@ -595,11 +600,20 @@ int rds_rdma_extra_size(struct rds_rdma_args
*args,
Quoted 1 line
Review int rds_rdma_extra_size(struct rds_rdma_args · line 600
Quoted 1 line
(quoted diff) +0 -0
               * nr_pages for one entry is limited to
(UINT_MAX>>PAGE_SHIFT)+1,
Quoted 1 line
Review int rds_rdma_extra_size(struct rds_rdma_args · line 601
Quoted 1 line
(quoted diff) +0 -0
               * so tot_pages cannot overflow without first going
negative.
Quoted 22 lines
Quoted 6 lines
net/rds/rdma.c +3 -2
               */
-             if (tot_pages < 0)
-                     return -EINVAL;
+             if (tot_pages < 0) {
+                     ret = -EINVAL;
+                     goto out;
net/rds/rdma.c int rds_rdma_extra_size(struct rds_rdma_args *args, · line 614
Quoted 10 lines
net/rds/rdma.c +7 -1
+             }
      }
 
-     return tot_pages * sizeof(struct scatterlist);
+     ret = tot_pages * sizeof(struct scatterlist);
+
+out:
+     if (ret < 0) {
+             kfree(iov->iov);
+             iov->iov = NULL;
Is this really needed?!? AFAICS rds_rdma_extra_size() is invoked only
via: rds_sendmsg() -> rds_rm_size() -> rds_rdma_extra_size(), and the rds_sendmsg() error path already frees any non NULL iov.
/P