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 <minc...@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 <minc...@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 <eduma...@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 <eduma...@google.com>
Signed-off-by: Minchan Kim <minc...@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
