lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 9 May 2020 16:14:41 -0700
From:   Minchan Kim <>
To:     Christian Brauner <>
Cc:     Andrew Morton <>,
        Vlastimil Babka <>,
        LKML <>,
        linux-mm <>,,, Suren Baghdasaryan <>,
        Tim Murray <>,
        Daniel Colascione <>,
        Sandeep Patil <>,
        Sonny Rao <>,
        Brian Geffon <>,
        Michal Hocko <>,
        Johannes Weiner <>,
        Shakeel Butt <>,
        John Dias <>,
        Joel Fernandes <>,
        Jann Horn <>,,,
        Christian Brauner <>,
        Kirill Tkhai <>
Subject: Re: [PATCH v7 5/7] mm: support both pid and pidfd for process_madvise

Hi Christian,

On Sat, May 09, 2020 at 02:48:17PM +0200, Christian Brauner wrote:
> On Fri, May 08, 2020 at 04:04:15PM -0700, Andrew Morton wrote:
> > On Fri, 8 May 2020 11:36:53 -0700 Minchan Kim <> wrote:
> > 
> > > 
> > > ...
> > >
> > > Per Vlastimil's request, I changed "which and advise" with "idtype and
> > > advice" in function prototype of description.
> > > Could you replace the part in the description? Code is never changed.
> > > 
> > 
> > Done, but...
> > 
> > >
> > > ...
> > >
> > > There is a demand[1] to support pid as well pidfd for process_madvise to
> > > reduce unnecessary syscall to get pidfd if the user has control of the
> > > target process(ie, they could guarantee the process is not gone or pid is
> > > not reused).
> > > 
> > > This patch aims for supporting both options like waitid(2).  So, the
> > > syscall is currently,
> > > 
> > >         int process_madvise(idtype_t idtype, id_t id, void *addr,
> > >                 size_t length, int advice, unsigned long flags);
> > > 
> > > @which is actually idtype_t for userspace libray and currently, it
> > > supports P_PID and P_PIDFD.
> > 
> > What does "@which is actually idtype_t for userspace libray" mean?  Can
> > you clarify and expand?
> If I may clarify, the only case where we've supported both pidfd and pid
> in the same system call is waitid() to avoid adding a dedicated system
> call for waiting and because waitid() already had this (imho insane)
> argument type switching. The idtype_t thing comes from waitid() and is
> located int sys/wait.h and is defined as
> "The type idtype_t is defined as an enumeration type whose possible
> values include at least the following:
> "
> int waitid(idtype_t idtype, id_t id, siginfo_t *infop, int options);
> If idtype is P_PID, waitid() shall wait for the child with a process ID equal to (pid_t)id.
> If idtype is P_PGID, waitid() shall wait for any child with a process group ID equal to (pid_t)id.
> If idtype is P_ALL, waitid() shall wait for any children and id is ignored.
> I'm personally not a fan of this idtype_t thing and think this should
> just have been 
> > >         int pidfd_madvise(int pidfd, void *addr,
> > >                 size_t length, int advice, unsigned long flags);
> and call it a day.

That was the argument at that time, Daniel and I didn't want to have
pid along with pidfd even though Kirill strongly wanted to have it.
However you said " Overall, I don't particularly care how or if you
integrate pidfd here." at that time.

I asked a question to Kirll at that time.

> Sounds like that you want to support both options for every upcoming API
> which deals with pid. I'm not sure how it's critical for process_madvise
> API this case. In general, we sacrifice some performance for the nicer one
> and later, once it's reported as hurdle for some workload, we could fix it
> via introducing new flag. What I don't like at this moment is to make
> syscall complicated with potential scenarios without real workload.

Yes, I suggest allowing both options for every new process api

You didn't give the opinion at that time, either(I expected you will
make some voice then). What I could do to proceed work was separate it
as different patch like this one to get more attention in future.
And now it works.

Let me clarify my side: I still don't like to introduce pid for new API
since we have pidfd. Since you just brought this issue again, I want to
hear *opinions* from others, again.

> Also, if I may ask, why is the flag argument "unsigned long"?
> That's pretty unorthodox. The expectation is that flag arguments are
> not word-size dependent and should usually use "unsigned int". All new
> system calls follow this pattern too.

Nothing special in this flag: Let me change it as "unsigned int".
I will send the change once we have an agreement on "pidfd" argument.

Thanks for the review, Christian!

Powered by blists - more mailing lists