From: Minchan Kim <minchan@kernel.org>
To: Eric Dumazet <eric.dumazet@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
Christian Brauner <christian.brauner@ubuntu.com>,
linux-mm <linux-mm@kvack.org>,
linux-api@vger.kernel.org, oleksandr@redhat.com,
Suren Baghdasaryan <surenb@google.com>,
Tim Murray <timmurray@google.com>,
Sandeep Patil <sspatil@google.com>,
Sonny Rao <sonnyrao@google.com>,
Brian Geffon <bgeffon@google.com>, Michal Hocko <mhocko@suse.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Shakeel Butt <shakeelb@google.com>,
John Dias <joaodias@google.com>,
Joel Fernandes <joel@joelfernandes.org>,
Jann Horn <jannh@google.com>,
alexander.h.duyck@linux.intel.com, sj38.park@gmail.com,
David Rientjes <rientjes@google.com>,
Arjun Roy <arjunroy@google.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Vlastimil Babka <vbabka@suse.cz>,
Christian Brauner <christian@brauner.io>,
Daniel Colascione <dancol@google.com>,
Jens Axboe <axboe@kernel.dk>, Kirill Tkhai <ktkhai@virtuozzo.com>,
SeongJae Park <sjpark@amazon.de>,
linux-man@vger.kernel.org
Subject: Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
Date: Mon, 16 Nov 2020 07:51:32 -0800 [thread overview]
Message-ID: <20201116155132.GA3805951@google.com> (raw)
In-Reply-To: <a376191d-908d-7d3c-a810-8ef51cc45f49@gmail.com>
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
next prev parent reply other threads:[~2020-11-16 15:51 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-01 0:06 [PATCH v9 0/3] introduce memory hinting API for external process Minchan Kim
2020-09-01 0:06 ` [PATCH v9 1/3] mm/madvise: pass mm to do_madvise Minchan Kim
2020-09-01 0:06 ` [PATCH v9 2/3] pid: move pidfd_get_pid() to pid.c Minchan Kim
2020-09-01 0:06 ` [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API Minchan Kim
2020-09-01 18:46 ` Florian Weimer
2020-09-03 17:26 ` Minchan Kim
2020-09-03 17:34 ` Florian Weimer
2020-09-03 17:59 ` Minchan Kim
2020-09-04 9:36 ` Christian Brauner
2020-09-21 6:56 ` Christoph Hellwig
2020-09-21 7:14 ` Michal Hocko
2020-09-21 17:55 ` Minchan Kim
2020-11-16 9:02 ` Eric Dumazet
2020-11-16 15:51 ` Minchan Kim [this message]
2020-11-17 20:06 ` Linus Torvalds
2020-11-17 20:31 ` Minchan Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201116155132.GA3805951@google.com \
--to=minchan@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=alexander.h.duyck@linux.intel.com \
--cc=arjunroy@google.com \
--cc=axboe@kernel.dk \
--cc=bgeffon@google.com \
--cc=christian.brauner@ubuntu.com \
--cc=christian@brauner.io \
--cc=dancol@google.com \
--cc=eric.dumazet@gmail.com \
--cc=hannes@cmpxchg.org \
--cc=hch@infradead.org \
--cc=jannh@google.com \
--cc=joaodias@google.com \
--cc=joel@joelfernandes.org \
--cc=ktkhai@virtuozzo.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-man@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@suse.com \
--cc=oleksandr@redhat.com \
--cc=rientjes@google.com \
--cc=shakeelb@google.com \
--cc=sj38.park@gmail.com \
--cc=sjpark@amazon.de \
--cc=sonnyrao@google.com \
--cc=sspatil@google.com \
--cc=surenb@google.com \
--cc=timmurray@google.com \
--cc=torvalds@linux-foundation.org \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).