Re: [PATCH] mm: use only pidfd for process_madvise syscall

[Minchan Kim <minchan@kernel.org>]

On Tue, May 19, 2020 at 09:19:56AM -0700, Minchan Kim wrote:
> Hi Christian,
> 
> On Tue, May 19, 2020 at 09:45:18AM +0200, Christian Brauner wrote:
> > On Fri, May 15, 2020 at 06:20:55PM -0700, Minchan Kim wrote:
> > > Based on discussion[1], people didn't feel we need to support both
> > > pid and pidfd for every new coming API[2] so this patch keeps only
> > > pidfd. This patch also changes flags's type with "unsigned int".
> > > So finally, the API is as follows,
> > > 
> > >       ssize_t process_madvise(int pidfd, const struct iovec *iovec,
> > >       		unsigned long vlen, int advice, unsigned int flags);
> > > 
> > >     DESCRIPTION
> > >       The process_madvise() system call is used to give advice or directions
> > >       to the kernel about the address ranges from external process as well as
> > >       local process. It provides the advice to address ranges of process
> > >       described by iovec and vlen. The goal of such advice is to improve system
> > >       or application performance.
> > > 
> > >       The pidfd selects the process referred to by the PID file descriptor
> > >       specified in pidfd. (See pidofd_open(2) for further information)
> > > 
> > >       The pointer iovec points to an array of iovec structures, defined in
> > >       <sys/uio.h> as:
> > > 
> > >         struct iovec {
> > >             void *iov_base;         /* starting address */
> > >             size_t iov_len;         /* number of bytes to be advised */
> > >         };
> > > 
> > >       The iovec describes address ranges beginning at address(iov_base)
> > >       and with size length of bytes(iov_len).
> > > 
> > >       The vlen represents the number of elements in iovec.
> > > 
> > >       The advice is indicated in the advice argument, which is one of the
> > >       following at this moment if the target process specified by idtype and
> > >       id is external.
> > > 
> > >         MADV_COLD
> > >         MADV_PAGEOUT
> > >         MADV_MERGEABLE
> > >         MADV_UNMERGEABLE
> > > 
> > >       Permission to provide a hint to external process is governed by a
> > >       ptrace access mode PTRACE_MODE_ATTACH_FSCREDS check; see ptrace(2).
> > > 
> > >       The process_madvise supports every advice madvise(2) has if target
> > >       process is in same thread group with calling process so user could
> > >       use process_madvise(2) to extend existing madvise(2) to support
> > >       vector address ranges.
> > > 
> > >     RETURN VALUE
> > >       On success, process_madvise() returns the number of bytes advised.
> > >       This return value may be less than the total number of requested
> > >       bytes, if an error occurred. The caller should check return value
> > >       to determine whether a partial advice occurred.
> > > 
> > > [1] https://lore.kernel.org/linux-mm/20200509124817.xmrvsrq3mla6b76k@wittgenstein/
> > > [2] https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/
> > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > 
> > Thanks for the ping Minchan, and sorry for not replying earlier to this.
> > 
> > Also, sorry that i delayed the patch but this here really seems a way
> > cleaner api to me and feels less hackish. In general this patch seems
> > fine to me.
> 
> Thanks.
> 
> > But two comments below:
> > 
> > > ---
> > >  mm/madvise.c | 42 +++++++++++++-----------------------------
> > >  1 file changed, 13 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index d3fbbe52d230..35c9b220146a 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1229,8 +1229,8 @@ static int process_madvise_vec(struct task_struct *target_task,
> > >  	return ret;
> > >  }
> > >  
> > > -static ssize_t do_process_madvise(int which, pid_t upid, struct iov_iter *iter,
> > > -				       int behavior, unsigned long flags)
> > > +static ssize_t do_process_madvise(int pidfd, struct iov_iter *iter,
> > > +				int behavior, unsigned int flags)
> > >  {
> > >  	ssize_t ret;
> > >  	struct pid *pid;
> > > @@ -1241,26 +1241,12 @@ static ssize_t do_process_madvise(int which, pid_t upid, struct iov_iter *iter,
> > >  	if (flags != 0)
> > >  		return -EINVAL;
> > >  
> > > -	switch (which) {
> > > -	case P_PID:
> > > -		if (upid <= 0)
> > > -			return -EINVAL;
> > > -
> > > -		pid = find_get_pid(upid);
> > > -		if (!pid)
> > > -			return -ESRCH;
> > > -		break;
> > > -	case P_PIDFD:
> > > -		if (upid < 0)
> > > -			return -EINVAL;
> > > -
> > > -		pid = pidfd_get_pid(upid);
> > > -		if (IS_ERR(pid))
> > > -			return PTR_ERR(pid);
> > > -		break;
> > > -	default:
> > > +	if (pidfd < 0)
> > >  		return -EINVAL;
> > 
> > When garbage file descriptors are passed EBADF needs to be returned, not
> > EINVAL. That's the case with most apis and also with pidfds, compare:
> 
> True. Let me cook a patch for that.

Hi Andrew,

Please fold this patch against on last patch you have for process_madvise.
Thanks!

From 939e4c5b7ca12efc5d5eeb8ff55fc02752f70544 Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Tue, 19 May 2020 11:06:57 -0700
Subject: [PATCH] mm: return EBADF if pidfd is invalid

This patch makes returning of EBADF when the fd passed as argument is
invalid. The implementaion relies on pidfd_get_pid's error return.

This patch also fixes syscall declare part since we removed pid support.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/compat.h   | 6 +++---
 include/linux/syscalls.h | 5 ++---
 mm/madvise.c             | 3 ---
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/include/linux/compat.h b/include/linux/compat.h
index 2e2f0a2700ab..86b61e873947 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -827,10 +827,10 @@ asmlinkage long compat_sys_pwritev64v2(unsigned long fd,
 		unsigned long vlen, loff_t pos, rwf_t flags);
 #endif
 
-asmlinkage ssize_t compat_sys_process_madvise(compat_int_t which,
-		compat_pid_t upid, const struct compat_iovec __user *vec,
+asmlinkage ssize_t compat_sys_process_madvise(compat_int_t pidfd,
+		const struct compat_iovec __user *vec,
 		compat_ulong_t vlen, compat_int_t behavior,
-		compat_ulong_t flags);
+		compat_int_t flags);
 
 /*
  * Deprecated system calls which are still defined in
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2cbd3660e8e6..63ffa6dc9da3 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -879,9 +879,8 @@ asmlinkage long sys_munlockall(void);
 asmlinkage long sys_mincore(unsigned long start, size_t len,
 				unsigned char __user * vec);
 asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
-asmlinkage long sys_process_madvise(int which, pid_t upid,
-		const struct iovec __user *vec, unsigned long vlen,
-		int behavior, unsigned long flags);
+asmlinkage long sys_process_madvise(int pidfd, const struct iovec __user *vec,
+		unsigned long vlen, int behavior, unsigned int flags);
 asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
 			unsigned long prot, unsigned long pgoff,
 			unsigned long flags);
diff --git a/mm/madvise.c b/mm/madvise.c
index 35c9b220146a..3ac1eda1203f 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1241,9 +1241,6 @@ static ssize_t do_process_madvise(int pidfd, struct iov_iter *iter,
 	if (flags != 0)
 		return -EINVAL;
 
-	if (pidfd < 0)
-		return -EINVAL;
-
 	pid = pidfd_get_pid(pidfd);
 	if (IS_ERR(pid))
 		return PTR_ERR(pid);
-- 
2.26.2.761.g0e0b3e54be-goog