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

[Minchan Kim]

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@xxxxxxxxxxxxx/
> > > Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> > 
> > 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!