[PATCH net v2] RDS: Fix memory leak in rds_rdma_extra_size()

[PATCH net v2] RDS: Fix memory leak in rds_rdma_extra_size()

[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 <cppcoffee@gmail.com>
---
 net/rds/rdma.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index aa6465dc7..91a20c1e2 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -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;
 }
 
 /*
-- 
2.34.1

Re: [PATCH net v2] RDS: Fix memory leak in rds_rdma_extra_size()

[Allison Henderson]

On Mon, 2026-04-13 at 15:00 +0800, Xiaobo Liu wrote:
> 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 <cppcoffee@gmail.com>

I think this looks good now.  Thanks Xiaobo.
Reviewed-by: Allison Henderson <achender@kernel.org>

> ---
>  net/rds/rdma.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/net/rds/rdma.c b/net/rds/rdma.c
> index aa6465dc7..91a20c1e2 100644
> --- a/net/rds/rdma.c
> +++ b/net/rds/rdma.c
> @@ -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;
>  }
>  
>  /*

Re: [PATCH net v2] RDS: Fix memory leak in rds_rdma_extra_size()

[Paolo Abeni]

On 4/13/26 9:00 AM, Xiaobo Liu wrote:
> @@ -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;

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

Re: [PATCH net v2] RDS: Fix memory leak in rds_rdma_extra_size()

[Allison Henderson]

On Thu, 2026-04-16 at 18:00 +0800, Xiaobo Liu wrote:
> 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

> 
> On 4/16/2616:20 Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On 4/13/26 9:00 AM, Xiaobo Liu wrote:
> > > @@ -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;
> > 
> > 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