Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API

[Minchan Kim <minchan@kernel.org>]

On Mon, Nov 16, 2020 at 10:02:42AM +0100, Eric Dumazet wrote:

< snip >

> > From 02d63c6b3f61a1085f4eab80f5171bd2627b5ab0 Mon Sep 17 00:00:00 2001
> > From: Minchan Kim <minchan@kernel.org>
> > Date: Mon, 21 Sep 2020 09:31:25 -0700
> > Subject: [PATCH] mm: do not use helper functions for process_madvise
> > 
> > This patch removes helper functions process_madvise_vec,
> > do_process_madvise and madv_import_iovec and use them inline.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/madvise.c | 97 +++++++++++++++++++++++-----------------------------
> >  1 file changed, 43 insertions(+), 54 deletions(-)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index ae266dfede8a..aa8bc65dbdb6 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1166,37 +1166,40 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  	return do_madvise(current->mm, start, len_in, behavior);
> >  }
> >  
> > -static int process_madvise_vec(struct mm_struct *mm, struct iov_iter *iter, int behavior)
> > -{
> > -	struct iovec iovec;
> > -	int ret = 0;
> > -
> > -	while (iov_iter_count(iter)) {
> > -		iovec = iov_iter_iovec(iter);
> > -		ret = do_madvise(mm, (unsigned long)iovec.iov_base, iovec.iov_len, behavior);
> > -		if (ret < 0)
> > -			break;
> > -		iov_iter_advance(iter, iovec.iov_len);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -static ssize_t do_process_madvise(int pidfd, struct iov_iter *iter,
> > -				int behavior, unsigned int flags)
> > +SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > +		size_t, vlen, int, behavior, unsigned int, flags)
> >  {
> >  	ssize_t ret;
> > +	struct iovec iovstack[UIO_FASTIOV], iovec;
> > +	struct iovec *iov = iovstack;
> > +	struct iov_iter iter;
> >  	struct pid *pid;
> >  	struct task_struct *task;
> >  	struct mm_struct *mm;
> > -	size_t total_len = iov_iter_count(iter);
> > +	size_t total_len;
> >  
> > -	if (flags != 0)
> > -		return -EINVAL;
> > +	if (flags != 0) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +#ifdef CONFIG_COMPAT
> > +	if (in_compat_syscall())
> > +		ret = compat_import_iovec(READ,
> > +				(struct compat_iovec __user *)vec, vlen,
> > +				ARRAY_SIZE(iovstack), &iov, &iter);
> > +	else
> > +#endif
> > +		ret = import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack),
> > +				&iov, &iter);
> > +	if (ret < 0)
> > +		goto out;
> >  
> >  	pid = pidfd_get_pid(pidfd);
> > -	if (IS_ERR(pid))
> > -		return PTR_ERR(pid);
> > +	if (IS_ERR(pid)) {
> > +		ret = PTR_ERR(pid);
> > +		goto free_iov;
> > +	}
> >  
> >  	task = get_pid_task(pid, PIDTYPE_PID);
> >  	if (!task) {
> > @@ -1216,43 +1219,29 @@ static ssize_t do_process_madvise(int pidfd, struct iov_iter *iter,
> >  		goto release_task;
> >  	}
> >  
> > -	ret = process_madvise_vec(mm, iter, behavior);
> > -	if (ret >= 0)
> > -		ret = total_len - iov_iter_count(iter);
> > +	total_len = iov_iter_count(&iter);
> > +
> > +	while (iov_iter_count(&iter)) {
> > +		iovec = iov_iter_iovec(&iter);
> > +		ret = do_madvise(mm, (unsigned long)iovec.iov_base,
> > +					iovec.iov_len, behavior);
> > +		if (ret < 0)
> > +			break;
> > +		iov_iter_advance(&iter, iovec.iov_len);
> > +	}
> > +
> > +	if (ret == 0)
> > +		ret = total_len - iov_iter_count(&iter);
> >  
> >  	mmput(mm);
> > +	return ret;
> 
> This "return ret;" seems quite wrong...

/me slaps self.

> 
> I will send the following :
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 416a56b8e757bf3465ab13cea51e0751ade2c745..cc9224a59e9fa07e41f9b4ad2e58b9c97889299b 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1231,7 +1231,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
>                 ret = total_len - iov_iter_count(&iter);
>  
>         mmput(mm);
> -       return ret;
>  
>  release_task:
>         put_task_struct(task);
> 
> 
> 
> 
> > +
> >  release_task:
> >  	put_task_struct(task);
> >  put_pid:
> >  	put_pid(pid);
> > -	return ret;
> > -}

Thanks, Eric!

Let me send a patch with your SoB if you don't mind.

From 0f37d5295324721ee19a3ad40fe58e3002cd9934 Mon Sep 17 00:00:00 2001
From: Eric Dumazet <edumazet@google.com>
Date: Mon, 16 Nov 2020 07:34:02 -0800
Subject: [PATCH] mm/madvise: fix memory leak from process_madvise

The eary return in process_madvise will produce memory leak.
Fix it.

Fixes: ecb8ac8b1f14 ("mm/madvise: introduce process_madvise() syscall: an external memory hinting API")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/madvise.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 416a56b8e757..7e773f949ef9 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1231,8 +1231,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
 		ret = total_len - iov_iter_count(&iter);
 
 	mmput(mm);
-	return ret;
-
 release_task:
 	put_task_struct(task);
 put_pid:
-- 
2.29.2.299.gdc1121823c-goog