* Re: [PATCH 07/17] fpga: dfl: afu: add AFU state related sysfs interfaces
From: Alan Tull @ 2019-03-28 17:13 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Xu Yilun
In-Reply-To: <1553483264-5379-8-git-send-email-hao.wu@intel.com>
On Sun, Mar 24, 2019 at 10:24 PM Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
>
> This patch introduces more sysfs interfaces for Accelerated
> Function Unit (AFU). These interfaces allow users to read
> current AFU Power State (APx), read / clear AFU Power (APx)
> events which are sticky to identify transient APx state,
> and manage AFU's LTR (latency tolerance reporting).
>
> Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
Thanks,
Alan
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Lucas De Marchi @ 2019-03-28 17:41 UTC (permalink / raw)
To: Jessica Yu
Cc: Alexey Gladkov, Masahiro Yamada, Michal Marek,
Linux Kernel Mailing List, Linux Kbuild mailing list, linux-api,
Kirill A . Shutemov, Gleb Fotengauer-Malinovskiy, Dmitry V. Levin,
Dmitry Torokhov, Rusty Russell, linux-modules
In-Reply-To: <20190327154025.GB23293@linux-8ccs>
On Wed, Mar 27, 2019 at 8:40 AM Jessica Yu <jeyu@kernel.org> wrote:
> >No. There are definitely not all modules. I have a builtin sha256_generic,
> >but I can't find him in the /sys/module.
>
> Yeah, you'll only find builtin modules under /sys/module/ if it has any module
> parameters, otherwise you won't find it there. As Masahiro already mentioned,
> if a builtin module has any parameters, they would be accessible under /sys/module/.
Could we please change that and add the sysfs entry regardless of
what's being discussed
here? Not having the entry there simply because we don't have
parameters for that module
always annoyed me.
Lucas De Marchi
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Lucas De Marchi @ 2019-03-28 17:56 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Jessica Yu, Masahiro Yamada, Michal Marek,
Linux Kernel Mailing List, Linux Kbuild mailing list, linux-api,
Kirill A . Shutemov, Gleb Fotengauer-Malinovskiy, Dmitry V. Levin,
Dmitry Torokhov, Rusty Russell
In-Reply-To: <20190327160440.GE15936@Legion-PC.fortress>
On Wed, Mar 27, 2019 at 9:04 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> On Wed, Mar 27, 2019 at 04:40:25PM +0100, Jessica Yu wrote:
> > +++ Alexey Gladkov [26/03/19 18:24 +0100]:
> > >On Fri, Mar 22, 2019 at 02:34:12PM +0900, Masahiro Yamada wrote:
> > >> Hi.
> > >>
> > >> (added some people to CC)
> >
> > (Thanks Masahiro for the CC!)
> >
> > >>
> > >> On Fri, Mar 15, 2019 at 7:10 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> > >> >
> > >> > Problem:
> > >> >
> > >> > When a kernel module is compiled as a separate module, some important
> > >> > information about the kernel module is available via .modinfo section of
> > >> > the module. In contrast, when the kernel module is compiled into the
> > >> > kernel, that information is not available.
> > >>
> > >>
> > >> I might be missing something, but
> > >> vmlinux provides info of builtin modules
> > >> in /sys/module/.
> > >
> > >No. There are definitely not all modules. I have a builtin sha256_generic,
> > >but I can't find him in the /sys/module.
> >
> > Yeah, you'll only find builtin modules under /sys/module/ if it has any module
> > parameters, otherwise you won't find it there. As Masahiro already mentioned,
> > if a builtin module has any parameters, they would be accessible under /sys/module/.
> >
> > >> (Looks like currently only module_param and MODULE_VERSION)
> > >>
> > >> This patch is not exactly the same, but I see a kind of overwrap.
> > >> I'd like to be sure if we want this new scheme.
> > >
> > >The /sys/module is only for running kernel. One of my use cases is
> > >to create an initrd for a new kernel.
> > >
> > >>
> > >> > Information about built-in modules is necessary in the following cases:
> > >> >
> > >> > 1. When it is necessary to find out what additional parameters can be
> > >> > passed to the kernel at boot time.
> > >>
> > >>
> > >> Actually, /sys/module/<module>/parameters/
> > >> exposes this information.
> > >>
> > >> Doesn't it work for your purpose?
> > >
> > >No, since creating an initrd needs to know all the modalias before
> > >I get the sysfs for new kernel. Also there are no modalias at all.
> > >
> > >> > 2. When you need to know which module names and their aliases are in
> > >> > the kernel. This is very useful for creating an initrd image.
> > >> >
> >
> > Hm, I do see one possible additional use-case for preserving module alias
> > information for built-in modules - modprobe will currently error (I think,
> > correct me if I'm wrong) if we try invoking modprobe with an alias of a
> > built-in module, simply because this information is not in modules.builtin or
> > modules.alias.
>
> Yes. Patch for modprobe in my todo list. The reason I didn’t do it was
> because I wasn’t sure that the file format was final.
>
> > Since kbuild already outputs modules.builtin, I would suggest outputting
> > something like a modules.builtin.alias file (and I guess maybe a modules.builtin.param
> > file too if that's deemed useful), in a format that is consumable by kmod/modprobe,
> > so that modprobing an alias of a built-in module doesn't produce an error. I
> > think this should be easy to do if we keep and parse the resulting .modinfo for
> > builtin modules. This is just an idea, opinions welcome. I've added Lucas to CC
> > in case he has any thoughts.
>
> You don't like kernel.builtin.modinfo ?
>
> It is much easier to create and it has almost the same format as the
> modules. So I think it will be easier to parse in kmod.
adding a modules.builtin.alias with the same format of modules.alias
means during modprobe we would only need to load one more file to
lookup aliases. That doesn't mean the kernel built system should do it
though. The same way it's depmod job to create the
modules.alias{,.bin}, we could leave this to depmod if it's in fact
useful to split the information.
I think your version is simple enough and we would get more
information that would be useful for modinfo.
It would indeed be nice to output something useful in "modinfo ext4".
In kmod, I think we would create modules.builtin.modinfo{,.bin} and
just add the aliases to modules.alias{,.bin},
This would keep the names consistent with what is already there.
thanks
Lucas De Marchi
>
> --
> Rgrds, legion
>
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: Matthew Garrett @ 2019-03-28 18:07 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Stephen Hemminger, Linux API, LSM List, LKML,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <alpine.LRH.2.21.1903281403540.27561@namei.org>
On Wed, Mar 27, 2019 at 8:15 PM James Morris <jmorris@namei.org> wrote:
> OTOH, this seems like a combination of mechanism and policy. The 3 modes
> are a help here, but I wonder if they may be too coarse grained still,
> e.g. if someone wants to allow a specific mechanism according to their own
> threat model and mitigations.
In general the interfaces blocked by these patches could also be
blocked with an LSM, and I'd guess that people with more fine-grained
requirements would probably take that approach.
> Secure boot gives you some assurance of the static state of the system at
> boot time, and lockdown is certainly useful (with or without secure boot),
> but it's not a complete solution to runtime kernel integrity protection by
> any stretch of the imagination. I'm concerned about it being perceived as
> such.
What do you think the functionality gaps are in terms of ensuring
kernel integrity (other than kernel flaws that allow the restrictions
to be bypassed)?
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Greg KH @ 2019-03-28 18:45 UTC (permalink / raw)
To: Lucas De Marchi
Cc: Jessica Yu, Alexey Gladkov, Masahiro Yamada, Michal Marek,
Linux Kernel Mailing List, Linux Kbuild mailing list, linux-api,
Kirill A . Shutemov, Gleb Fotengauer-Malinovskiy, Dmitry V. Levin,
Dmitry Torokhov, Rusty Russell, linux-modules
In-Reply-To: <CAKi4VA+14kihzGFbMT623BwMbZEzgYU6+jz8MpR9jqoC059oog@mail.gmail.com>
On Thu, Mar 28, 2019 at 10:41:59AM -0700, Lucas De Marchi wrote:
> On Wed, Mar 27, 2019 at 8:40 AM Jessica Yu <jeyu@kernel.org> wrote:
> > >No. There are definitely not all modules. I have a builtin sha256_generic,
> > >but I can't find him in the /sys/module.
> >
> > Yeah, you'll only find builtin modules under /sys/module/ if it has any module
> > parameters, otherwise you won't find it there. As Masahiro already mentioned,
> > if a builtin module has any parameters, they would be accessible under /sys/module/.
>
> Could we please change that and add the sysfs entry regardless of
> what's being discussed here? Not having the entry there simply because
> we don't have parameters for that module always annoyed me.
What is the sysfs directory going to show? Will it just be empty?
Feel free to send a patch for this, but from what I remember, it wasn't
the easiest thing to do for some reason. But given that the code was
implemented before git was, I can't quite remember.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 02/17] fpga: dfl: fme: align PR buffer size per PR datawidth
From: Alan Tull @ 2019-03-28 18:50 UTC (permalink / raw)
To: Wu Hao; +Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Xu Yilun
In-Reply-To: <20190326002852.GA2901@hao-dev>
On Mon, Mar 25, 2019 at 7:44 PM Wu Hao <hao.wu@intel.com> wrote:
>
> On Mon, Mar 25, 2019 at 12:50:40PM -0500, Alan Tull wrote:
> > On Sun, Mar 24, 2019 at 10:23 PM Wu Hao <hao.wu@intel.com> wrote:
> >
> > Hi Hao,
> >
> > Looks good, one question below.
> >
> > >
> > > Current driver checks if input bitstream file size is aligned or
> > > not per PR data width (default 32bits). It requires one additional
> > > step for end user when they generate the bitstream file, padding
> > > extra zeros to bitstream file to align its size per PR data width,
> > > but they don't have to as hardware will drop extra padding bytes
> > > automatically.
> > >
> > > In order to simplify the user steps, this patch aligns PR buffer
> > > size per PR data width in driver, to allow user to pass unaligned
> > > size bitstream files to driver.
> > >
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
Thanks,
Alan
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: James Morris @ 2019-03-28 19:23 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andy Lutomirski, Stephen Hemminger, Linux API, LSM List, LKML,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <CACdnJusbi1jB+G0wMcj43wNik9Fiv3QCc8v30ztrFWg5-P-0iQ@mail.gmail.com>
On Thu, 28 Mar 2019, Matthew Garrett wrote:
> On Wed, Mar 27, 2019 at 8:15 PM James Morris <jmorris@namei.org> wrote:
> > OTOH, this seems like a combination of mechanism and policy. The 3 modes
> > are a help here, but I wonder if they may be too coarse grained still,
> > e.g. if someone wants to allow a specific mechanism according to their own
> > threat model and mitigations.
>
> In general the interfaces blocked by these patches could also be
> blocked with an LSM, and I'd guess that people with more fine-grained
> requirements would probably take that approach.
So... I have to ask, why not use LSM for this in the first place?
Either with an existing module or perhaps a lockdown LSM?
>
> > Secure boot gives you some assurance of the static state of the system at
> > boot time, and lockdown is certainly useful (with or without secure boot),
> > but it's not a complete solution to runtime kernel integrity protection by
> > any stretch of the imagination. I'm concerned about it being perceived as
> > such.
>
> What do you think the functionality gaps are in terms of ensuring
> kernel integrity (other than kernel flaws that allow the restrictions
> to be bypassed)?
I don't know of any non-flaw gaps.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: Matthew Garrett @ 2019-03-28 20:08 UTC (permalink / raw)
To: James Morris
Cc: Andy Lutomirski, Stephen Hemminger, Linux API, LSM List, LKML,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <alpine.LRH.2.21.1903290621350.1769@namei.org>
On Thu, Mar 28, 2019 at 12:23 PM James Morris <jmorris@namei.org> wrote:
>
> On Thu, 28 Mar 2019, Matthew Garrett wrote:
>
> > On Wed, Mar 27, 2019 at 8:15 PM James Morris <jmorris@namei.org> wrote:
> > > OTOH, this seems like a combination of mechanism and policy. The 3 modes
> > > are a help here, but I wonder if they may be too coarse grained still,
> > > e.g. if someone wants to allow a specific mechanism according to their own
> > > threat model and mitigations.
> >
> > In general the interfaces blocked by these patches could also be
> > blocked with an LSM, and I'd guess that people with more fine-grained
> > requirements would probably take that approach.
>
> So... I have to ask, why not use LSM for this in the first place?
>
> Either with an existing module or perhaps a lockdown LSM?
Some of it isn't really achievable that way - for instance, enforcing
module or kexec signatures. We have other mechanisms that can be used
to enable that which could be done at the more fine-grained level, but
a design goal was to make it possible to automatically enable a full
set of integrity protections under specified circumstances.
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Dmitry Torokhov @ 2019-03-28 21:33 UTC (permalink / raw)
To: Greg KH
Cc: Lucas De Marchi, Jessica Yu, Alexey Gladkov, Masahiro Yamada,
Michal Marek, Linux Kernel Mailing List,
Linux Kbuild mailing list, open list:ABI/API, Kirill A . Shutemov,
Gleb Fotengauer-Malinovskiy, Dmitry V. Levin, Rusty Russell,
linux-modules
In-Reply-To: <20190328184546.GB23800@kroah.com>
On Thu, Mar 28, 2019 at 11:45 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Mar 28, 2019 at 10:41:59AM -0700, Lucas De Marchi wrote:
> > On Wed, Mar 27, 2019 at 8:40 AM Jessica Yu <jeyu@kernel.org> wrote:
> > > >No. There are definitely not all modules. I have a builtin sha256_generic,
> > > >but I can't find him in the /sys/module.
> > >
> > > Yeah, you'll only find builtin modules under /sys/module/ if it has any module
> > > parameters, otherwise you won't find it there. As Masahiro already mentioned,
> > > if a builtin module has any parameters, they would be accessible under /sys/module/.
> >
> > Could we please change that and add the sysfs entry regardless of
> > what's being discussed here? Not having the entry there simply because
> > we don't have parameters for that module always annoyed me.
What is the benefit compared to wasting some memory for the directory?
>
> What is the sysfs directory going to show? Will it just be empty?
>
> Feel free to send a patch for this, but from what I remember, it wasn't
> the easiest thing to do for some reason. But given that the code was
> implemented before git was, I can't quite remember.
I am pretty sure we allow empty attribute groups, so it is probbaly as
simple as removing "If (!params) return 0;" form
module_param_sysfs_setup() and making sure we always create
"parameters" group instead of doing it on first visible parameter in
add_sysfs_param(). I suppose we no longer want to skip over parameters
that are not readable nor writable either?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Richard Guy Briggs @ 2019-03-28 21:40 UTC (permalink / raw)
To: Paul Moore
Cc: Ondrej Mosnacek, nhorman, linux-api, containers, LKML,
David Howells, Linux-Audit Mailing List, netfilter-devel,
Eric W . Biederman, Simo Sorce, netdev, linux-fsdevel, Eric Paris,
Serge E. Hallyn
In-Reply-To: <CAHC9VhQYVSrX1Ng7hZejkGSq6ZXtqi+SKmpARASJ4P2_2i3-4w@mail.gmail.com>
On 2019-03-28 11:46, Paul Moore wrote:
> On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Audit events could happen in a network namespace outside of a task
> > > > context due to packets received from the net that trigger an auditing
> > > > rule prior to being associated with a running task. The network
> > > > namespace could be in use by multiple containers by association to the
> > > > tasks in that network namespace. We still want a way to attribute
> > > > these events to any potential containers. Keep a list per network
> > > > namespace to track these audit container identifiiers.
> > > >
> > > > Add/increment the audit container identifier on:
> > > > - initial setting of the audit container identifier via /proc
> > > > - clone/fork call that inherits an audit container identifier
> > > > - unshare call that inherits an audit container identifier
> > > > - setns call that inherits an audit container identifier
> > > > Delete/decrement the audit container identifier on:
> > > > - an inherited audit container identifier dropped when child set
> > > > - process exit
> > > > - unshare call that drops a net namespace
> > > > - setns call that drops a net namespace
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > include/linux/audit.h | 19 ++++++++++++
> > > > kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > kernel/nsproxy.c | 4 +++
> > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index fa19fa408931..70255c2dfb9f 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -27,6 +27,7 @@
> > > > #include <linux/ptrace.h>
> > > > #include <linux/namei.h> /* LOOKUP_* */
> > > > #include <uapi/linux/audit.h>
> > > > +#include <linux/refcount.h>
> > > >
> > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > >
> > > > extern struct audit_task_info init_struct_audit;
> > > >
> > > > +struct audit_contid {
> > > > + struct list_head list;
> > > > + u64 id;
> > > > + refcount_t refcount;
> > >
> > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > if we could just make it a regular unsigned int (we don't need the
> > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > checking, so it's probably better to leave it as is...
> >
> > Since the update is done using rcu-safe methods, do we even need the
> > spin_lock? Neil? Paul?
>
> As discussed, the refcount field is protected against simultaneous
> writes by the spinlock that protects additions/removals from the list
> as a whole so I don't believe the refcount_t atomicity is critical in
> this regard.
>
> Where it gets tricky, and I can't say I'm 100% confident on my answer
> here, is if refcount was a regular int and we wanted to access it
> outside of a spinlock (to be clear, it doesn't look like this patch
> currently does this). With RCU, if refcount was a regular int
> (unsigned or otherwise), I believe it would be possible for different
> threads of execution to potentially see different values of refcount
> (assuming one thread was adding/removing from the list). Using a
> refcount_t would protect against this, alternatively, taking the
> spinlock should also protect against this.
Ok, from the above it isn't clear to me if you are happy with the
current code or would prefer any changes, or from below that you still
need to work it through to make a pronouncement. It sounds to me you
would be ok with *either* spinlock *or* refcount_t, but don't see the
need for both.
> As we all know, RCU can be tricky at times, so I may be off on the
> above; if I am, please provide an explanation so I (and likely others
> as well) can learn a little bit more. :)
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Paul Moore @ 2019-03-28 22:00 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Ondrej Mosnacek, nhorman, linux-api, containers, LKML,
David Howells, Linux-Audit Mailing List, netfilter-devel,
Eric W . Biederman, Simo Sorce, netdev, linux-fsdevel, Eric Paris,
Serge E. Hallyn
In-Reply-To: <20190328214023.qpszfvxbrjlldmmt@madcap2.tricolour.ca>
On Thu, Mar 28, 2019 at 5:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-03-28 11:46, Paul Moore wrote:
> > On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > Audit events could happen in a network namespace outside of a task
> > > > > context due to packets received from the net that trigger an auditing
> > > > > rule prior to being associated with a running task. The network
> > > > > namespace could be in use by multiple containers by association to the
> > > > > tasks in that network namespace. We still want a way to attribute
> > > > > these events to any potential containers. Keep a list per network
> > > > > namespace to track these audit container identifiiers.
> > > > >
> > > > > Add/increment the audit container identifier on:
> > > > > - initial setting of the audit container identifier via /proc
> > > > > - clone/fork call that inherits an audit container identifier
> > > > > - unshare call that inherits an audit container identifier
> > > > > - setns call that inherits an audit container identifier
> > > > > Delete/decrement the audit container identifier on:
> > > > > - an inherited audit container identifier dropped when child set
> > > > > - process exit
> > > > > - unshare call that drops a net namespace
> > > > > - setns call that drops a net namespace
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > include/linux/audit.h | 19 ++++++++++++
> > > > > kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > kernel/nsproxy.c | 4 +++
> > > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > > index fa19fa408931..70255c2dfb9f 100644
> > > > > --- a/include/linux/audit.h
> > > > > +++ b/include/linux/audit.h
> > > > > @@ -27,6 +27,7 @@
> > > > > #include <linux/ptrace.h>
> > > > > #include <linux/namei.h> /* LOOKUP_* */
> > > > > #include <uapi/linux/audit.h>
> > > > > +#include <linux/refcount.h>
> > > > >
> > > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > > >
> > > > > extern struct audit_task_info init_struct_audit;
> > > > >
> > > > > +struct audit_contid {
> > > > > + struct list_head list;
> > > > > + u64 id;
> > > > > + refcount_t refcount;
> > > >
> > > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > > if we could just make it a regular unsigned int (we don't need the
> > > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > > checking, so it's probably better to leave it as is...
> > >
> > > Since the update is done using rcu-safe methods, do we even need the
> > > spin_lock? Neil? Paul?
> >
> > As discussed, the refcount field is protected against simultaneous
> > writes by the spinlock that protects additions/removals from the list
> > as a whole so I don't believe the refcount_t atomicity is critical in
> > this regard.
> >
> > Where it gets tricky, and I can't say I'm 100% confident on my answer
> > here, is if refcount was a regular int and we wanted to access it
> > outside of a spinlock (to be clear, it doesn't look like this patch
> > currently does this). With RCU, if refcount was a regular int
> > (unsigned or otherwise), I believe it would be possible for different
> > threads of execution to potentially see different values of refcount
> > (assuming one thread was adding/removing from the list). Using a
> > refcount_t would protect against this, alternatively, taking the
> > spinlock should also protect against this.
>
> Ok, from the above it isn't clear to me if you are happy with the
> current code or would prefer any changes, or from below that you still
> need to work it through to make a pronouncement. It sounds to me you
> would be ok with *either* spinlock *or* refcount_t, but don't see the
> need for both.
To be fair you didn't ask if I was "happy" with the approach above,
you asked if we needed the spinlock/refcount_t. I believe I answered
that question as comprehensively as I could, but perhaps you wanted a
hard yes or no? In that case, since refcount_t is obviously safer, I
would stick with that for now just to limit the number of possible
failures. If someone smarter than you or I comes along and
definitively says you are 100% safe to use an int, then go ahead and
use an int.
Beyond that, I'm still in the process of reviewing your patches, but I
haven't finished yet, so no "pronouncement" or whatever you want to
call it.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 06/17] fpga: dfl: pci: enable SRIOV support.
From: Alan Tull @ 2019-03-28 22:03 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Zhang Yi Z,
Xu Yilun
In-Reply-To: <1553483264-5379-7-git-send-email-hao.wu@intel.com>
On Sun, Mar 24, 2019 at 10:24 PM Wu Hao <hao.wu@intel.com> wrote:
>
> This patch enables the standard sriov support. It allows user to
> enable SRIOV (and VFs), then user could pass through accelerators
> (VFs) into virtual machine or use VFs directly in host.
>
> Signed-off-by: Zhang Yi Z <yi.z.zhang@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
^ permalink raw reply
* Re: [PATCH 05/17] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
From: Alan Tull @ 2019-03-28 22:03 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Zhang Yi Z,
Xu Yilun
In-Reply-To: <1553483264-5379-6-git-send-email-hao.wu@intel.com>
On Sun, Mar 24, 2019 at 10:23 PM Wu Hao <hao.wu@intel.com> wrote:
>
> In order to support virtualization usage via PCIe SRIOV, this patch
> adds two ioctls under FPGA Management Engine (FME) to release and
> assign back the port device. In order to safely turn Port from PF
> into VF and enable PCIe SRIOV, it requires user to invoke this
> PORT_RELEASE ioctl to release port firstly to remove userspace
> interfaces, and then configure the PF/VF access register in FME.
> After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> ioctl to attach the port back to PF.
>
> Ioctl interfaces:
> * DFL_FPGA_FME_PORT_RELEASE
> Release platform device of given port, it deletes port platform
> device to remove related userspace interfaces on PF, then
> configures PF/VF access mode to VF.
>
> * DFL_FPGA_FME_PORT_ASSIGN
> Assign platform device of given port back to PF, it configures
> PF/VF access mode to PF, then adds port platform device back to
> re-enable related userspace interfaces on PF.
>
> Signed-off-by: Zhang Yi Z <yi.z.zhang@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
Acked-by: Alan Tull <atull@kernel.org>
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Neil Horman @ 2019-03-29 14:49 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Ondrej Mosnacek, linux-api, containers, LKML, David Howells,
Linux-Audit Mailing List, netfilter-devel, Eric W . Biederman,
Simo Sorce, netdev, linux-fsdevel, Eric Paris, Serge E. Hallyn
In-Reply-To: <20190328011202.6raixwzdimn5b4zk@madcap2.tricolour.ca>
On Wed, Mar 27, 2019 at 09:12:02PM -0400, Richard Guy Briggs wrote:
> On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Audit events could happen in a network namespace outside of a task
> > > context due to packets received from the net that trigger an auditing
> > > rule prior to being associated with a running task. The network
> > > namespace could be in use by multiple containers by association to the
> > > tasks in that network namespace. We still want a way to attribute
> > > these events to any potential containers. Keep a list per network
> > > namespace to track these audit container identifiiers.
> > >
> > > Add/increment the audit container identifier on:
> > > - initial setting of the audit container identifier via /proc
> > > - clone/fork call that inherits an audit container identifier
> > > - unshare call that inherits an audit container identifier
> > > - setns call that inherits an audit container identifier
> > > Delete/decrement the audit container identifier on:
> > > - an inherited audit container identifier dropped when child set
> > > - process exit
> > > - unshare call that drops a net namespace
> > > - setns call that drops a net namespace
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > include/linux/audit.h | 19 ++++++++++++
> > > kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > kernel/nsproxy.c | 4 +++
> > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index fa19fa408931..70255c2dfb9f 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -27,6 +27,7 @@
> > > #include <linux/ptrace.h>
> > > #include <linux/namei.h> /* LOOKUP_* */
> > > #include <uapi/linux/audit.h>
> > > +#include <linux/refcount.h>
> > >
> > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > >
> > > extern struct audit_task_info init_struct_audit;
> > >
> > > +struct audit_contid {
> > > + struct list_head list;
> > > + u64 id;
> > > + refcount_t refcount;
> >
> > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > if we could just make it a regular unsigned int (we don't need the
> > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > checking, so it's probably better to leave it as is...
>
> Since the update is done using rcu-safe methods, do we even need the
> spin_lock? Neil? Paul?
>
Yes, we do. Rcu-safe methods only apply to read side operations, we still need
traditional mutual exclusion on the write side of the operation. That is to say
we need to protect the list against multiple writers at the same time, and for
that we need a spin lock.
Neil
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Neil Horman @ 2019-03-29 14:50 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, Ondrej Mosnacek, linux-api, containers, LKML,
David Howells, Linux-Audit Mailing List, netfilter-devel,
Eric W . Biederman, Simo Sorce, netdev, linux-fsdevel, Eric Paris,
Serge E. Hallyn
In-Reply-To: <20190328214023.qpszfvxbrjlldmmt@madcap2.tricolour.ca>
On Thu, Mar 28, 2019 at 05:40:23PM -0400, Richard Guy Briggs wrote:
> On 2019-03-28 11:46, Paul Moore wrote:
> > On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > Audit events could happen in a network namespace outside of a task
> > > > > context due to packets received from the net that trigger an auditing
> > > > > rule prior to being associated with a running task. The network
> > > > > namespace could be in use by multiple containers by association to the
> > > > > tasks in that network namespace. We still want a way to attribute
> > > > > these events to any potential containers. Keep a list per network
> > > > > namespace to track these audit container identifiiers.
> > > > >
> > > > > Add/increment the audit container identifier on:
> > > > > - initial setting of the audit container identifier via /proc
> > > > > - clone/fork call that inherits an audit container identifier
> > > > > - unshare call that inherits an audit container identifier
> > > > > - setns call that inherits an audit container identifier
> > > > > Delete/decrement the audit container identifier on:
> > > > > - an inherited audit container identifier dropped when child set
> > > > > - process exit
> > > > > - unshare call that drops a net namespace
> > > > > - setns call that drops a net namespace
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > include/linux/audit.h | 19 ++++++++++++
> > > > > kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > kernel/nsproxy.c | 4 +++
> > > > > 3 files changed, 106 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > > index fa19fa408931..70255c2dfb9f 100644
> > > > > --- a/include/linux/audit.h
> > > > > +++ b/include/linux/audit.h
> > > > > @@ -27,6 +27,7 @@
> > > > > #include <linux/ptrace.h>
> > > > > #include <linux/namei.h> /* LOOKUP_* */
> > > > > #include <uapi/linux/audit.h>
> > > > > +#include <linux/refcount.h>
> > > > >
> > > > > #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > > #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > > >
> > > > > extern struct audit_task_info init_struct_audit;
> > > > >
> > > > > +struct audit_contid {
> > > > > + struct list_head list;
> > > > > + u64 id;
> > > > > + refcount_t refcount;
> > > >
> > > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > > if we could just make it a regular unsigned int (we don't need the
> > > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > > checking, so it's probably better to leave it as is...
> > >
> > > Since the update is done using rcu-safe methods, do we even need the
> > > spin_lock? Neil? Paul?
> >
> > As discussed, the refcount field is protected against simultaneous
> > writes by the spinlock that protects additions/removals from the list
> > as a whole so I don't believe the refcount_t atomicity is critical in
> > this regard.
> >
> > Where it gets tricky, and I can't say I'm 100% confident on my answer
> > here, is if refcount was a regular int and we wanted to access it
> > outside of a spinlock (to be clear, it doesn't look like this patch
> > currently does this). With RCU, if refcount was a regular int
> > (unsigned or otherwise), I believe it would be possible for different
> > threads of execution to potentially see different values of refcount
> > (assuming one thread was adding/removing from the list). Using a
> > refcount_t would protect against this, alternatively, taking the
> > spinlock should also protect against this.
>
> Ok, from the above it isn't clear to me if you are happy with the
> current code or would prefer any changes, or from below that you still
> need to work it through to make a pronouncement. It sounds to me you
> would be ok with *either* spinlock *or* refcount_t, but don't see the
> need for both.
>
I'll reiterate I think we should keep the refcount type just as it is, not for
safetys sake, but for readability and convienience.
Because the refcount currently only is used on add and delete operations
(implying it is only read in paths where its also modified), we need to
guarantee atomicity against multiple parallel writes. We already have that
guarantee because every path in which we call
refcount_set/refcount_inc/refcount_dec_and_test occurs under the protection of
the list spin lock, and so from that standpoint we don't need the additional
guarantees offered by the refcount_t type.
However, if it were to be converted to an int type, we would have to replace the
refcount_dec_and_test call with this:
if (x == 0)
warn_on_underflow
return
x -= 1;
if (x == 0)
preform_operations_to_free_list_entry
I find refcount_dec_and_test to be far easier to read and maintain, and you
still have to do all of this under the protection of a spin lock, to protect
against multiple writes. And if you ever find that you are adding a pure read
side query of the refcount, you would need to hold the spin lock there as well,
instead of just using the available refcount_read api call
Yeah, you would save a few cycles if you didn't use an atomic type here, but
we're only talking about paths from user space making system calls executing
here (no high volume per packet receive paths or anything), and these paths have
already taken a few locks (the list lock, the task lock, etc), so eliminating
this one atomic isn't going to amount to anything. Lets leave it as it is and
buy ourselves the extra code readability.
Neil
> > As we all know, RCU can be tricky at times, so I may be off on the
> > above; if I am, please provide an explanation so I (and likely others
> > as well) can learn a little bit more. :)
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
>
^ permalink raw reply
* [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-29 15:54 UTC (permalink / raw)
To: jannh, luto, dhowells, serge, linux-api, linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol, Christian Brauner
/* Introduction */
This adds the pidfd_open() syscall.
pidfd_open() allows to retrieve file descriptors for a given pid. This
includes both file descriptors for processes and file descriptors for
threads.
With the addition of this syscalls pidfds become independent of procfs just
as pids are. Of course, if CONFIG_PROC_FS is not set then metadata access
for processes will not be possible but everything else will just work fine.
In addition, this allows us to remove the dependency of pidfd_send_signal()
on procfs and enable it unconditionally.
With the ability to call pidfd_open() on tids we can now add a flag to
pidfd_send_signal() to signal to a specific thread capturing the
functionality of tgkill() and related thread-focused signal syscalls.
The desire to lift the restriction for pidfds on procfs has been expressed
by multiple people (cf. the commit message of commit
3eb39f47934f9d5a3027fe00d906a45fe3a15fad and [2]).
/* Signature */
int pidfd_open(pid_t pid, unsigned int flags);
/* pidfds are anon inode file descriptors */
These pidfds are allocated using anon_inode_getfd(), are O_CLOEXEC by
default and can be used with the pidfd_send_signal() syscall. They are not
dirfds and as such have the advantage that we can make them pollable or
readable in the future if we see a need to do so. The pidfds are not
associated with a specific pid namespaces but rather only reference struct
pid of a given process in their private_data member.
Additionally, Andy made an argument that we should go forward with
non-proc-dirfd file descriptors for the sake of security and extensibility
(cf. [3]). This will unblock or help move along work on pidfd_wait which
is currently ongoing.
/* Process Metadata Access */
One of the oustanding issues has been how to get information about a given
process if pidfds are regular file descriptors and do not provide access to
the process /proc/<pid> directory.
Various solutions have been proposed. The one that most people prefer is to
be able to retrieve a file descriptor to /proc/<pid> based on a pidfd
(cf. [5]). The prefered solution for how to do this has been to implement
an ioctl that for pidfds that translates a pidfd into a dirfd for
/proc/<pid>. This has been implemented in this patchset as well. If
PIDFD_GET_PROCFD is passed as a command to an ioctl() taking a pidfd and an
fd referring to a procfs directory as an argument a corresponding dirfd to
/proc/<pid> can be retrieved.
The ioctl() makes very sure that the struct pid associated with the
/proc/<pid> fd is identical to the struct pid stashed in the pidfd. This
ensures that we avoid pid recycling issues.
/* Testing */
The patchset comes with tests (which btw. I consider mandatory with
every feature-patch that intends to go through the pidfd tree):
- test that no invalid flags can be passed to pidfd_open()
- test that no invalid pid can be passed to pidfd_open()
- test that a pidfd can be retrieved with pidfd_open()
- test whether a pidfd can be converted into an fd to /proc/<pid> to get
metadata access
- test that a pidfd retrieved based on a pid that has been recycled cannot
be converted into /proc/<pid> for that recycled pid
/* Example */
int pidfd = pidfd_open(1234, 0);
int procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int procpidfd = ioctl(pidfd, PIDFD_GET_PROCFD, procfd);
int statusfd = openat(procpidfd, "status", O_RDONLY | O_CLOEXEC);
int ret = read(statusfd, buf, sizeof(buf));
ret = pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
/* References */
[1]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/
[2]: https://lore.kernel.org/lkml/20190320203910.GA2842@avx2/
[3]: https://lore.kernel.org/lkml/CALCETrXO=V=+qEdLDVPf8eCgLZiB9bOTrUfe0V-U-tUZoeoRDA@mail.gmail.com
[4]: https://lore.kernel.org/lkml/CAHk-=wgmKZm-fESEiLq_W37sKpqCY89nQkPNfWhvF_CQ1ANgcw@mail.gmail.com
[5]: https://lore.kernel.org/lkml/533075A9-A6CF-4549-AFC8-B90505B198FD@joelfernandes.org
Christian Brauner (4):
pid: add pidfd_open()
signal: support pidfd_open() with pidfd_send_signal()
signal: PIDFD_SIGNAL_TID threads via pidfds
tests: add pidfd_open() tests
David Howells (1):
Make anon_inodes unconditional
arch/arm/kvm/Kconfig | 1 -
arch/arm64/kvm/Kconfig | 1 -
arch/mips/kvm/Kconfig | 1 -
arch/powerpc/kvm/Kconfig | 1 -
arch/s390/kvm/Kconfig | 1 -
arch/x86/Kconfig | 1 -
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/x86/kvm/Kconfig | 1 -
drivers/base/Kconfig | 1 -
drivers/char/tpm/Kconfig | 1 -
drivers/dma-buf/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/iio/Kconfig | 1 -
drivers/infiniband/Kconfig | 1 -
drivers/vfio/Kconfig | 1 -
fs/Makefile | 2 +-
fs/notify/fanotify/Kconfig | 1 -
fs/notify/inotify/Kconfig | 1 -
include/linux/pid.h | 2 +
include/linux/syscalls.h | 1 +
include/uapi/linux/wait.h | 5 +
init/Kconfig | 10 -
kernel/pid.c | 181 +++++++++
kernel/signal.c | 130 +++++--
kernel/sys_ni.c | 3 -
tools/testing/selftests/pidfd/Makefile | 2 +-
tools/testing/selftests/pidfd/pidfd.h | 57 +++
.../testing/selftests/pidfd/pidfd_open_test.c | 361 ++++++++++++++++++
tools/testing/selftests/pidfd/pidfd_test.c | 41 +-
30 files changed, 701 insertions(+), 112 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd.h
create mode 100644 tools/testing/selftests/pidfd/pidfd_open_test.c
--
2.21.0
^ permalink raw reply
* [PATCH v2 1/5] Make anon_inodes unconditional
From: Christian Brauner @ 2019-03-29 15:54 UTC (permalink / raw)
To: jannh, luto, dhowells, serge, linux-api, linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol, Christian Brauner
In-Reply-To: <20190329155425.26059-1-christian@brauner.io>
From: David Howells <dhowells@redhat.com>
Make the anon_inodes facility unconditional so that it can be used by core
VFS code and the pidfd_open() syscall.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
[christian@brauner.io: adapt commit message to mention pidfd_open()]
Signed-off-by: Christian Brauner <christian@brauner.io>
---
arch/arm/kvm/Kconfig | 1 -
arch/arm64/kvm/Kconfig | 1 -
arch/mips/kvm/Kconfig | 1 -
arch/powerpc/kvm/Kconfig | 1 -
arch/s390/kvm/Kconfig | 1 -
arch/x86/Kconfig | 1 -
arch/x86/kvm/Kconfig | 1 -
drivers/base/Kconfig | 1 -
drivers/char/tpm/Kconfig | 1 -
drivers/dma-buf/Kconfig | 1 -
drivers/gpio/Kconfig | 1 -
drivers/iio/Kconfig | 1 -
drivers/infiniband/Kconfig | 1 -
drivers/vfio/Kconfig | 1 -
fs/Makefile | 2 +-
fs/notify/fanotify/Kconfig | 1 -
fs/notify/inotify/Kconfig | 1 -
init/Kconfig | 10 ----------
18 files changed, 1 insertion(+), 27 deletions(-)
diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index 3f5320f46de2..f591026347a5 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -22,7 +22,6 @@ config KVM
bool "Kernel-based Virtual Machine (KVM) support"
depends on MMU && OF
select PREEMPT_NOTIFIERS
- select ANON_INODES
select ARM_GIC
select ARM_GIC_V3
select ARM_GIC_V3_ITS
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index a3f85624313e..a67121d419a2 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -23,7 +23,6 @@ config KVM
depends on OF
select MMU_NOTIFIER
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
select HAVE_KVM_ARCH_TLB_FLUSH_ALL
select KVM_MMIO
diff --git a/arch/mips/kvm/Kconfig b/arch/mips/kvm/Kconfig
index 4528bc9c3cb1..eac25aef21e0 100644
--- a/arch/mips/kvm/Kconfig
+++ b/arch/mips/kvm/Kconfig
@@ -21,7 +21,6 @@ config KVM
depends on MIPS_FP_SUPPORT
select EXPORT_UASM
select PREEMPT_NOTIFIERS
- select ANON_INODES
select KVM_GENERIC_DIRTYLOG_READ_PROTECT
select HAVE_KVM_VCPU_ASYNC_IOCTL
select KVM_MMIO
diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
index bfdde04e4905..f53997a8ca62 100644
--- a/arch/powerpc/kvm/Kconfig
+++ b/arch/powerpc/kvm/Kconfig
@@ -20,7 +20,6 @@ if VIRTUALIZATION
config KVM
bool
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_EVENTFD
select HAVE_KVM_VCPU_ASYNC_IOCTL
select SRCU
diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index 767453faacfc..1816ee48eadd 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -21,7 +21,6 @@ config KVM
prompt "Kernel-based Virtual Machine (KVM) support"
depends on HAVE_KVM
select PREEMPT_NOTIFIERS
- select ANON_INODES
select HAVE_KVM_CPU_RELAX_INTERCEPT
select HAVE_KVM_VCPU_ASYNC_IOCTL
select HAVE_KVM_EVENTFD
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c1f9b3cf437c..18f2c954464e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -44,7 +44,6 @@ config X86
#
select ACPI_LEGACY_TABLES_LOOKUP if ACPI
select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
- select ANON_INODES
select ARCH_32BIT_OFF_T if X86_32
select ARCH_CLOCKSOURCE_DATA
select ARCH_CLOCKSOURCE_INIT
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index 72fa955f4a15..fc042419e670 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -27,7 +27,6 @@ config KVM
depends on X86_LOCAL_APIC
select PREEMPT_NOTIFIERS
select MMU_NOTIFIER
- select ANON_INODES
select HAVE_KVM_IRQCHIP
select HAVE_KVM_IRQFD
select IRQ_BYPASS_MANAGER
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 059700ea3521..03f067da12ee 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -174,7 +174,6 @@ source "drivers/base/regmap/Kconfig"
config DMA_SHARED_BUFFER
bool
default n
- select ANON_INODES
select IRQ_WORK
help
This option enables the framework for buffer-sharing between
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 536e55d3919f..f3e4bc490cf0 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -157,7 +157,6 @@ config TCG_CRB
config TCG_VTPM_PROXY
tristate "VTPM Proxy Interface"
depends on TCG_TPM
- select ANON_INODES
---help---
This driver proxies for an emulated TPM (vTPM) running in userspace.
A device /dev/vtpmx is provided that creates a device pair
diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 2e5a0faa2cb1..3fc9c2efc583 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -3,7 +3,6 @@ menu "DMABUF options"
config SYNC_FILE
bool "Explicit Synchronization Framework"
default n
- select ANON_INODES
select DMA_SHARED_BUFFER
---help---
The Sync File Framework adds explicit syncronization via
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f50526a771f..0f91600c27ae 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -12,7 +12,6 @@ config ARCH_HAVE_CUSTOM_GPIO_H
menuconfig GPIOLIB
bool "GPIO Support"
- select ANON_INODES
help
This enables GPIO support through the generic GPIO library.
You only need to enable this, if you also want to enable
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index d08aeb41cd07..1dec0fecb6ef 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -4,7 +4,6 @@
menuconfig IIO
tristate "Industrial I/O support"
- select ANON_INODES
help
The industrial I/O subsystem provides a unified framework for
drivers for many different types of embedded sensors using a
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index a1fb840de45d..d318bab25860 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -25,7 +25,6 @@ config INFINIBAND_USER_MAD
config INFINIBAND_USER_ACCESS
tristate "InfiniBand userspace access (verbs and CM)"
- select ANON_INODES
depends on MMU
---help---
Userspace InfiniBand access support. This enables the
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index 9de5ed38da83..3798d77d131c 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -22,7 +22,6 @@ menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
depends on IOMMU_API
select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
- select ANON_INODES
help
VFIO provides a framework for secure userspace device drivers.
See Documentation/vfio.txt for more details.
diff --git a/fs/Makefile b/fs/Makefile
index 427fec226fae..35945f8139e6 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -25,7 +25,7 @@ obj-$(CONFIG_PROC_FS) += proc_namespace.o
obj-y += notify/
obj-$(CONFIG_EPOLL) += eventpoll.o
-obj-$(CONFIG_ANON_INODES) += anon_inodes.o
+obj-y += anon_inodes.o
obj-$(CONFIG_SIGNALFD) += signalfd.o
obj-$(CONFIG_TIMERFD) += timerfd.o
obj-$(CONFIG_EVENTFD) += eventfd.o
diff --git a/fs/notify/fanotify/Kconfig b/fs/notify/fanotify/Kconfig
index 735bfb2e9190..521dc91d2cb5 100644
--- a/fs/notify/fanotify/Kconfig
+++ b/fs/notify/fanotify/Kconfig
@@ -1,7 +1,6 @@
config FANOTIFY
bool "Filesystem wide access notification"
select FSNOTIFY
- select ANON_INODES
select EXPORTFS
default n
---help---
diff --git a/fs/notify/inotify/Kconfig b/fs/notify/inotify/Kconfig
index b981fc0c8379..0161c74e76e2 100644
--- a/fs/notify/inotify/Kconfig
+++ b/fs/notify/inotify/Kconfig
@@ -1,6 +1,5 @@
config INOTIFY_USER
bool "Inotify support for userspace"
- select ANON_INODES
select FSNOTIFY
default y
---help---
diff --git a/init/Kconfig b/init/Kconfig
index 4592bf7997c0..be8f97e37a76 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1171,9 +1171,6 @@ config LD_DEAD_CODE_DATA_ELIMINATION
config SYSCTL
bool
-config ANON_INODES
- bool
-
config HAVE_UID16
bool
@@ -1378,14 +1375,12 @@ config HAVE_FUTEX_CMPXCHG
config EPOLL
bool "Enable eventpoll support" if EXPERT
default y
- select ANON_INODES
help
Disabling this option will cause the kernel to be built without
support for epoll family of system calls.
config SIGNALFD
bool "Enable signalfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the signalfd() system call that allows to receive signals
@@ -1395,7 +1390,6 @@ config SIGNALFD
config TIMERFD
bool "Enable timerfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the timerfd() system call that allows to receive timer
@@ -1405,7 +1399,6 @@ config TIMERFD
config EVENTFD
bool "Enable eventfd() system call" if EXPERT
- select ANON_INODES
default y
help
Enable the eventfd() system call that allows to receive both
@@ -1516,7 +1509,6 @@ config KALLSYMS_BASE_RELATIVE
# syscall, maps, verifier
config BPF_SYSCALL
bool "Enable bpf() system call"
- select ANON_INODES
select BPF
select IRQ_WORK
default n
@@ -1533,7 +1525,6 @@ config BPF_JIT_ALWAYS_ON
config USERFAULTFD
bool "Enable userfaultfd() system call"
- select ANON_INODES
depends on MMU
help
Enable the userfaultfd() system call that allows to intercept and
@@ -1600,7 +1591,6 @@ config PERF_EVENTS
bool "Kernel performance events and counters"
default y if PROFILING
depends on HAVE_PERF_EVENTS
- select ANON_INODES
select IRQ_WORK
select SRCU
help
--
2.21.0
^ permalink raw reply related
* [PATCH v2 2/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-29 15:54 UTC (permalink / raw)
To: jannh, luto, dhowells, serge, linux-api, linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol, Christian Brauner
In-Reply-To: <20190329155425.26059-1-christian@brauner.io>
/* Introduction */
This adds the pidfd_open() syscall.
pidfd_open() allows to retrieve file descriptors for a given pid. This
includes both file descriptors for processes and file descriptors for
threads.
With the addition of this syscalls pidfd become independent of procfs just
as pids are. Of course, if CONFIG_PROC_FS is not set then metadata access
for processes will not be possible but everything else will just work fine.
In addition, this allows us to remove the dependency of pidfd_send_signal()
on procfs and enable it unconditionally.
With the ability to call pidfd_open() on tids we can now add a flag to
pidfd_send_signal() to signal to a specific thread capturing the
functionality of tgkill() and related thread-focused signal syscalls.
The desire to lift the restriction for pidfds on procfs has been expressed
by multiple people (cf. the commit message of commit
3eb39f47934f9d5a3027fe00d906a45fe3a15fad and [2]).
/* Signature */
int pidfd_open(pid_t pid, unsigned int flags);
/* pidfds are anon inode file descriptors */
These pidfds are allocated using anon_inode_getfd(), are O_CLOEXEC by
default and can be used with the pidfd_send_signal() syscall. They are not
dirfds and as such have the advantage that we can make them pollable or
readable in the future if we see a need to do so. The pidfds are not
associated with a specific pid namespaces but rather only reference struct
pid of a given process in their private_data member.
Additionally, Andy made an argument that we should go forward with
non-proc-dirfd file descriptors for the sake of security and extensibility
(cf. [3]). This will unblock or help move along work on pidfd_wait which
is currently ongoing.
/* Process Metadata Access */
One of the oustanding issues has been how to get information about a given
process if pidfds are regular file descriptors and do not provide access to
the process /proc/<pid> directory.
Various solutions have been proposed. The one that most people prefer is to
be able to retrieve a file descriptor to /proc/<pid> based on a pidfd
(cf. [5]). The prefered solution for how to do this has been to implement
an ioctl that for pidfds that translates a pidfd into a dirfd for
/proc/<pid>. This has been implemented in this patchset as well. If
PIDFD_GET_PROCFD is passed as a command to an ioctl() taking a pidfd and an
fd referring to a procfs directory as an argument a corresponding dirfd to
/proc/<pid> can be retrieved.
The ioctl() makes very sure that the struct pid associated with the
/proc/<pid> fd is identical to the struct pid stashed in the pidfd. This
ensures that we avoid pid recycling issues.
/* Example */
int pidfd = pidfd_open(1234, 0);
int procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int procpidfd = ioctl(pidfd, PIDFD_GET_PROCFD, procfd);
int statusfd = openat(procpidfd, "status", O_RDONLY | O_CLOEXEC);
int ret = read(statusfd, buf, sizeof(buf));
ret = pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
/* References */
[1]: https://lore.kernel.org/lkml/20181228233725.722tdfgijxcssg76@brauner.io/
[2]: https://lore.kernel.org/lkml/20190320203910.GA2842@avx2/
[3]: https://lore.kernel.org/lkml/CALCETrXO=V=+qEdLDVPf8eCgLZiB9bOTrUfe0V-U-tUZoeoRDA@mail.gmail.com
[4]: https://lore.kernel.org/lkml/CAHk-=wgmKZm-fESEiLq_W37sKpqCY89nQkPNfWhvF_CQ1ANgcw@mail.gmail.com
[5]: https://lore.kernel.org/lkml/533075A9-A6CF-4549-AFC8-B90505B198FD@joelfernandes.org
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jann Horn <jannh@google.com
Cc: David Howells <dhowells@redhat.com>
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
include/linux/pid.h | 2 +
include/linux/syscalls.h | 1 +
include/uapi/linux/wait.h | 2 +
kernel/pid.c | 181 +++++++++++++++++++++++++
6 files changed, 188 insertions(+)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 1f9607ed087c..c8046f261bee 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -433,3 +433,4 @@
425 i386 io_uring_setup sys_io_uring_setup __ia32_sys_io_uring_setup
426 i386 io_uring_enter sys_io_uring_enter __ia32_sys_io_uring_enter
427 i386 io_uring_register sys_io_uring_register __ia32_sys_io_uring_register
+428 i386 pidfd_open sys_pidfd_open __ia32_sys_pidfd_open
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 92ee0b4378d4..f714a3d57b88 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -349,6 +349,7 @@
425 common io_uring_setup __x64_sys_io_uring_setup
426 common io_uring_enter __x64_sys_io_uring_enter
427 common io_uring_register __x64_sys_io_uring_register
+428 common pidfd_open __x64_sys_pidfd_open
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/pid.h b/include/linux/pid.h
index b6f4ba16065a..3c8ef5a199ca 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -66,6 +66,8 @@ struct pid
extern struct pid init_struct_pid;
+extern const struct file_operations pidfd_fops;
+
static inline struct pid *get_pid(struct pid *pid)
{
if (pid)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e446806a561f..117463673fb5 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -929,6 +929,7 @@ asmlinkage long sys_clock_adjtime32(clockid_t which_clock,
struct old_timex32 __user *tx);
asmlinkage long sys_syncfs(int fd);
asmlinkage long sys_setns(int fd, int nstype);
+asmlinkage long sys_pidfd_open(pid_t pid, unsigned int flags);
asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
unsigned int vlen, unsigned flags);
asmlinkage long sys_process_vm_readv(pid_t pid,
diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
index ac49a220cf2a..d6c7c0701997 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h
@@ -18,5 +18,7 @@
#define P_PID 1
#define P_PGID 2
+/* Get a file descriptor for /proc/<pid> of the corresponding pidfd */
+#define PIDFD_GET_PROCFD _IOR('p', 1, int)
#endif /* _UAPI_LINUX_WAIT_H */
diff --git a/kernel/pid.c b/kernel/pid.c
index 20881598bdfa..8c9e15e0e463 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -26,8 +26,10 @@
*
*/
+#include <linux/anon_inodes.h>
#include <linux/mm.h>
#include <linux/export.h>
+#include <linux/fsnotify.h>
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/rculist.h>
@@ -40,6 +42,7 @@
#include <linux/proc_fs.h>
#include <linux/sched/task.h>
#include <linux/idr.h>
+#include <linux/wait.h>
struct pid init_struct_pid = {
.count = ATOMIC_INIT(1),
@@ -451,6 +454,184 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
return idr_get_next(&ns->idr, &nr);
}
+#ifdef CONFIG_PROC_FS
+static struct pid_namespace *pidfd_get_proc_pid_ns(const struct file *file)
+{
+ struct inode *inode;
+ struct super_block *sb;
+
+ inode = file_inode(file);
+ sb = inode->i_sb;
+ if (sb->s_magic != PROC_SUPER_MAGIC)
+ return ERR_PTR(-EINVAL);
+
+ if (inode->i_ino != PROC_ROOT_INO)
+ return ERR_PTR(-EINVAL);
+
+ return get_pid_ns(inode->i_sb->s_fs_info);
+}
+
+static struct pid *pidfd_get_pid(const struct file *file)
+{
+ if (file->f_op != &pidfd_fops)
+ return ERR_PTR(-EINVAL);
+
+ return get_pid(file->private_data);
+}
+
+static struct file *pidfd_open_proc_pid(const struct file *procf, pid_t pid,
+ const struct pid *pidfd_pid)
+{
+ char name[12]; /* int to strlen + \0 but with */
+ struct file *file;
+ struct pid *proc_pid;
+
+ snprintf(name, sizeof(name), "%d", pid);
+ file = file_open_root(procf->f_path.dentry, procf->f_path.mnt, name,
+ O_DIRECTORY | O_RDONLY | O_NOFOLLOW, 0);
+ if (IS_ERR(file))
+ return file;
+
+ proc_pid = tgid_pidfd_to_pid(file);
+ if (IS_ERR(proc_pid)) {
+ filp_close(file, NULL);
+ return ERR_CAST(proc_pid);
+ }
+
+ if (pidfd_pid != proc_pid) {
+ filp_close(file, NULL);
+ return ERR_PTR(-ESRCH);
+ }
+
+ return file;
+}
+
+static inline int pidfd_to_procfd(int procfd, struct file *pidfd_file)
+{
+ long fd;
+ pid_t ns_pid;
+ struct fd fdproc;
+ struct file *file = NULL;
+ struct pid *pidfd_pid = NULL;
+ struct pid_namespace *proc_pid_ns = NULL;
+
+ fdproc = fdget(procfd);
+ if (!fdproc.file)
+ return -EBADF;
+
+ proc_pid_ns = pidfd_get_proc_pid_ns(fdproc.file);
+ if (IS_ERR(proc_pid_ns)) {
+ fd = PTR_ERR(proc_pid_ns);
+ proc_pid_ns = NULL;
+ goto err;
+ }
+
+ pidfd_pid = pidfd_get_pid(pidfd_file);
+ if (IS_ERR(pidfd_pid)) {
+ fd = PTR_ERR(pidfd_pid);
+ pidfd_pid = NULL;
+ goto err;
+ }
+
+ ns_pid = pid_nr_ns(pidfd_pid, proc_pid_ns);
+ if (!ns_pid) {
+ fd = -ESRCH;
+ goto err;
+ }
+
+ file = pidfd_open_proc_pid(fdproc.file, ns_pid, pidfd_pid);
+ if (IS_ERR(file)) {
+ fd = PTR_ERR(file);
+ file = NULL;
+ goto err;
+ }
+
+ fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fd < 0)
+ goto err;
+
+ fsnotify_open(file);
+ fd_install(fd, file);
+ file = NULL;
+
+err:
+ fdput(fdproc);
+ if (proc_pid_ns)
+ put_pid_ns(proc_pid_ns);
+ put_pid(pidfd_pid);
+ if (file)
+ filp_close(file, NULL);
+
+ return fd;
+}
+#else
+static inline int pidfd_to_procfd(int procfd, struct file *pidfd_file)
+{
+ return -EOPNOTSUPP;
+}
+#endif /* CONFIG_PROC_FS */
+
+static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int procfd = arg;
+
+ switch (cmd) {
+ case PIDFD_GET_PROCFD:
+ return pidfd_to_procfd(procfd, file);
+ default:
+ return -ENOTTY;
+ }
+}
+
+static int pidfd_release(struct inode *inode, struct file *file)
+{
+ struct pid *pid = file->private_data;
+
+ if (pid) {
+ file->private_data = NULL;
+ put_pid(pid);
+ }
+
+ return 0;
+}
+
+const struct file_operations pidfd_fops = {
+ .release = pidfd_release,
+ .unlocked_ioctl = pidfd_ioctl,
+};
+
+static int pidfd_create_fd_cloexec(pid_t pid)
+{
+ int fd;
+ struct pid *p;
+
+ p = find_get_pid(pid);
+ if (!p)
+ return -ESRCH;
+
+ fd = anon_inode_getfd("pidfd", &pidfd_fops, p, O_RDWR | O_CLOEXEC);
+ if (fd < 0)
+ put_pid(p);
+
+ return fd;
+}
+
+/*
+ * pidfd_open - open a pidfd
+ * @pid: pid for which to retrieve a pidfd
+ * @flags: flags to pass
+ */
+SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
+{
+ if (flags)
+ return -EINVAL;
+
+ if (pid <= 0)
+ return -EINVAL;
+
+ return pidfd_create_fd_cloexec(pid);
+}
+
void __init pid_idr_init(void)
{
/* Verify no one has done anything silly: */
--
2.21.0
^ permalink raw reply related
* [PATCH v2 3/5] signal: support pidfd_open() with pidfd_send_signal()
From: Christian Brauner @ 2019-03-29 15:54 UTC (permalink / raw)
To: jannh, luto, dhowells, serge, linux-api, linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol, Christian Brauner, Florian Weimer
In-Reply-To: <20190329155425.26059-1-christian@brauner.io>
Let pidfd_send_signal() use pidfds retrieved via pidfd_open(). With this
patch pidfd_send_signal() becomes independent of procfs. This fullfils the
request made when we merged the pidfd_send_signal() patchset. The
pidfd_send_signal() syscall is now always available allowing for it to be
used by users without procfs mounted or even users without procfs support
compiled into the kernel.
Signed-off-by: Christian Brauner <christian@brauner.io>
Reviewed-by: David Howells <dhowells@redhat.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
---
kernel/signal.c | 14 ++++++++++----
kernel/sys_ni.c | 3 ---
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index b7953934aa99..eb97d0cc6ef7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3513,7 +3513,6 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
return kill_something_info(sig, &info, pid);
}
-#ifdef CONFIG_PROC_FS
/*
* Verify that the signaler and signalee either are in the same pid namespace
* or that the signaler's pid namespace is an ancestor of the signalee's pid
@@ -3550,6 +3549,14 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
return copy_siginfo_from_user(kinfo, info);
}
+static struct pid *pidfd_to_pid(const struct file *file)
+{
+ if (file->f_op == &pidfd_fops)
+ return file->private_data;
+
+ return tgid_pidfd_to_pid(file);
+}
+
/**
* sys_pidfd_send_signal - send a signal to a process through a task file
* descriptor
@@ -3581,12 +3588,12 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (flags)
return -EINVAL;
- f = fdget_raw(pidfd);
+ f = fdget(pidfd);
if (!f.file)
return -EBADF;
/* Is this a pidfd? */
- pid = tgid_pidfd_to_pid(f.file);
+ pid = pidfd_to_pid(f.file);
if (IS_ERR(pid)) {
ret = PTR_ERR(pid);
goto err;
@@ -3625,7 +3632,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
fdput(f);
return ret;
}
-#endif /* CONFIG_PROC_FS */
static int
do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index d21f4befaea4..4d9ae5ea6caf 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -167,9 +167,6 @@ COND_SYSCALL(syslog);
/* kernel/sched/core.c */
-/* kernel/signal.c */
-COND_SYSCALL(pidfd_send_signal);
-
/* kernel/sys.c */
COND_SYSCALL(setregid);
COND_SYSCALL(setgid);
--
2.21.0
^ permalink raw reply related
* [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds
From: Christian Brauner @ 2019-03-29 15:54 UTC (permalink / raw)
To: jannh, luto, dhowells, serge, linux-api, linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol, Christian Brauner, Florian Weimer
In-Reply-To: <20190329155425.26059-1-christian@brauner.io>
With the addition of pidfd_open() it is possible for users to reference a
specific thread by doing:
int pidfd = pidfd_open(<tid>, 0);
This means we can extend pidfd_send_signal() to signal a specific thread.
As promised in the commit for pidfd_send_signal() [1] the extension is
based on a flag argument, i.e. the scope of the signal delivery is based on
the flag argument, not on the type of file descriptor.
To this end the flag PIDFD_SIGNAL_TID is added. With this change we now
cover most of the functionality of all the other signal sending functions
combined:
- pidfd_send_signal(<pidfd>, <sig>, NULL, 0);
which is equivalent to
kill(<positive-pid>, <signal>)
- pidfd_send_signal(<pidfd>, <sig>, <info>, 0);
which is equivalent to
rt_sigqueueinfo(<tgid>, <sig>, <uinfo>)
- pidfd_send_signal(<pidfd>, <sig>, NULL, PIDFD_SIGNAL_TID);
which is equivalent to
tgkill(<tgid>, <tid>, <signal)
rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
- pidfd_send_signal(<pidfd>, <sig>, <info>, PIDFD_SIGNAL_TID);
which is equivalent to
rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
/* References */
[1]: commit 3eb39f47934 ("signal: add pidfd_send_signal() syscall")
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
---
include/uapi/linux/wait.h | 3 +
kernel/signal.c | 116 ++++++++++++++++++++++++++------------
2 files changed, 82 insertions(+), 37 deletions(-)
diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
index d6c7c0701997..b72f0ef84fe5 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h
@@ -21,4 +21,7 @@
/* Get a file descriptor for /proc/<pid> of the corresponding pidfd */
#define PIDFD_GET_PROCFD _IOR('p', 1, int)
+/* Flags to pass to pidfd_send_signal */
+#define PIDFD_SIGNAL_TID 1 /* Send signal to specific thread */
+
#endif /* _UAPI_LINUX_WAIT_H */
diff --git a/kernel/signal.c b/kernel/signal.c
index eb97d0cc6ef7..9f93da85b2b9 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3557,17 +3557,86 @@ static struct pid *pidfd_to_pid(const struct file *file)
return tgid_pidfd_to_pid(file);
}
+static int __do_send_specific(struct task_struct *p, int sig,
+ struct kernel_siginfo *info)
+{
+ int error = -ESRCH;
+
+ error = check_kill_permission(sig, info, p);
+ /*
+ * The null signal is a permissions and process existence probe.
+ * No signal is actually delivered.
+ */
+ if (!error && sig) {
+ error = do_send_sig_info(sig, info, p, PIDTYPE_PID);
+ /*
+ * If lock_task_sighand() failed we pretend the task
+ * dies after receiving the signal. The window is tiny,
+ * and the signal is private anyway.
+ */
+ if (unlikely(error == -ESRCH))
+ error = 0;
+ }
+
+ return error;
+}
+
+static int do_send_specific(pid_t tgid, pid_t pid, int sig,
+ struct kernel_siginfo *info)
+{
+ struct task_struct *p;
+ int error = -ESRCH;
+
+ rcu_read_lock();
+ p = find_task_by_vpid(pid);
+ if (p && (tgid <= 0 || task_tgid_vnr(p) == tgid))
+ error = __do_send_specific(p, sig, info);
+ rcu_read_unlock();
+
+ return error;
+}
+
+static int pidfd_send_signal_specific(struct pid *pid, int sig,
+ struct kernel_siginfo *info)
+{
+ struct task_struct *p;
+ int error = -ESRCH;
+
+ rcu_read_lock();
+ p = pid_task(pid, PIDTYPE_PID);
+ if (p)
+ error = __do_send_specific(p, sig, info);
+ rcu_read_unlock();
+
+ return error;
+}
+
/**
- * sys_pidfd_send_signal - send a signal to a process through a task file
- * descriptor
+ * sys_pidfd_send_signal - send a signal to a process through a pidfd
+
* @pidfd: the file descriptor of the process
* @sig: signal to be sent
* @info: the signal info
* @flags: future flags to be passed
*
- * The syscall currently only signals via PIDTYPE_PID which covers
- * kill(<positive-pid>, <signal>. It does not signal threads or process
- * groups.
+ * The syscall currently covers:
+ * - pidfd_send_signal(<pidfd>, <sig>, NULL, 0);
+ * which is equivalent to
+ * kill(<positive-pid>, <signal>)
+ *
+ * - pidfd_send_signal(<pidfd>, <sig>, <info>, 0);
+ * which is equivalent to
+ * rt_sigqueueinfo(<tgid>, <sig>, <uinfo>)
+ *
+ * - pidfd_send_signal(<pidfd>, <sig>, NULL, PIDFD_SIGNAL_TID);
+ * which is equivalent to
+ * tgkill(<tgid>, <tid>, <signal)
+ *
+ * rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
+ * - pidfd_send_signal(<pidfd>, <sig>, <info>, PIDFD_SIGNAL_TID);
+ * which is equivalent to
+ * rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
+ *
* In order to extend the syscall to threads and process groups the @flags
* argument should be used. In essence, the @flags argument will determine
* what is signaled and not the file descriptor itself. Put in other words,
@@ -3585,7 +3654,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
kernel_siginfo_t kinfo;
/* Enforce flags be set to 0 until we add an extension. */
- if (flags)
+ if (flags & ~PIDFD_SIGNAL_TID)
return -EINVAL;
f = fdget(pidfd);
@@ -3626,43 +3695,16 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
prepare_kill_siginfo(sig, &kinfo);
}
- ret = kill_pid_info(sig, &kinfo, pid);
+ if (flags & PIDFD_SIGNAL_TID)
+ ret = pidfd_send_signal_specific(pid, sig, &kinfo);
+ else
+ ret = kill_pid_info(sig, &kinfo, pid);
err:
fdput(f);
return ret;
}
-static int
-do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
-{
- struct task_struct *p;
- int error = -ESRCH;
-
- rcu_read_lock();
- p = find_task_by_vpid(pid);
- if (p && (tgid <= 0 || task_tgid_vnr(p) == tgid)) {
- error = check_kill_permission(sig, info, p);
- /*
- * The null signal is a permissions and process existence
- * probe. No signal is actually delivered.
- */
- if (!error && sig) {
- error = do_send_sig_info(sig, info, p, PIDTYPE_PID);
- /*
- * If lock_task_sighand() failed we pretend the task
- * dies after receiving the signal. The window is tiny,
- * and the signal is private anyway.
- */
- if (unlikely(error == -ESRCH))
- error = 0;
- }
- }
- rcu_read_unlock();
-
- return error;
-}
-
static int do_tkill(pid_t tgid, pid_t pid, int sig)
{
struct kernel_siginfo info;
--
2.21.0
^ permalink raw reply related
* [PATCH v2 5/5] tests: add pidfd_open() tests
From: Christian Brauner @ 2019-03-29 15:54 UTC (permalink / raw)
To: jannh, luto, dhowells, serge, linux-api, linux-kernel
Cc: arnd, ebiederm, khlebnikov, keescook, adobriyan, tglx,
mtk.manpages, bl0pbl33p, ldv, akpm, oleg, nagarathnam.muthusamy,
cyphar, viro, joel, dancol, Christian Brauner
In-Reply-To: <20190329155425.26059-1-christian@brauner.io>
This adds testing for pidfd_open():
- test that no invalid flags can be passed to pidfd_open()
- test that no invalid pid can be passed to pidfd_open()
- test that a pidfd can be retrieved with pidfd_open()
- test whether a pidfd can be converted into an fd to /proc/<pid> to get
metadata access
- test that a pidfd retrieved based on a pid that has been recycled cannot
be converted into /proc/<pid> for that recycled pid
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jann Horn <jannh@google.com
Cc: David Howells <dhowells@redhat.com>
Cc: "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>
Cc: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: "Dmitry V. Levin" <ldv@altlinux.org>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Nagarathnam Muthusamy <nagarathnam.muthusamy@oracle.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
tools/testing/selftests/pidfd/Makefile | 2 +-
tools/testing/selftests/pidfd/pidfd.h | 57 +++
.../testing/selftests/pidfd/pidfd_open_test.c | 361 ++++++++++++++++++
tools/testing/selftests/pidfd/pidfd_test.c | 41 +-
4 files changed, 420 insertions(+), 41 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd.h
create mode 100644 tools/testing/selftests/pidfd/pidfd_open_test.c
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index deaf8073bc06..b36c0be70848 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,6 +1,6 @@
CFLAGS += -g -I../../../../usr/include/
-TEST_GEN_PROGS := pidfd_test
+TEST_GEN_PROGS := pidfd_test pidfd_open_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
new file mode 100644
index 000000000000..8452e910463f
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PIDFD_H
+#define __PIDFD_H
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/mount.h>
+
+#include "../kselftest.h"
+
+/*
+ * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
+ * That means, when it wraps around any pid < 300 will be skipped.
+ * So we need to use a pid > 300 in order to test recycling.
+ */
+#define PID_RECYCLE 1000
+
+/*
+ * Define a few custom error codes for the child process to clearly indicate
+ * what is happening. This way we can tell the difference between a system
+ * error, a test error, etc.
+ */
+#define PIDFD_PASS 0
+#define PIDFD_FAIL 1
+#define PIDFD_ERROR 2
+#define PIDFD_SKIP 3
+#define PIDFD_XFAIL 4
+
+int wait_for_pid(pid_t pid)
+{
+ int status, ret;
+
+again:
+ ret = waitpid(pid, &status, 0);
+ if (ret == -1) {
+ if (errno == EINTR)
+ goto again;
+
+ return -1;
+ }
+
+ if (!WIFEXITED(status))
+ return -1;
+
+ return WEXITSTATUS(status);
+}
+
+
+#endif /* __PIDFD_H */
diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
new file mode 100644
index 000000000000..ebe7a96cc8d6
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
@@ -0,0 +1,361 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/mount.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include "pidfd.h"
+#include "../kselftest.h"
+
+static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
+{
+ return syscall(__NR_pidfd_open, pid, flags);
+}
+
+static int safe_int(const char *numstr, int *converted)
+{
+ char *err = NULL;
+ signed long int sli;
+
+ errno = 0;
+ sli = strtol(numstr, &err, 0);
+ if (errno == ERANGE && (sli == LONG_MAX || sli == LONG_MIN))
+ return -ERANGE;
+
+ if (errno != 0 && sli == 0)
+ return -EINVAL;
+
+ if (err == numstr || *err != '\0')
+ return -EINVAL;
+
+ if (sli > INT_MAX || sli < INT_MIN)
+ return -ERANGE;
+
+ *converted = (int)sli;
+ return 0;
+}
+
+static int char_left_gc(const char *buffer, size_t len)
+{
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ if (buffer[i] == ' ' ||
+ buffer[i] == '\t')
+ continue;
+
+ return i;
+ }
+
+ return 0;
+}
+
+static int char_right_gc(const char *buffer, size_t len)
+{
+ int i;
+
+ for (i = len - 1; i >= 0; i--) {
+ if (buffer[i] == ' ' ||
+ buffer[i] == '\t' ||
+ buffer[i] == '\n' ||
+ buffer[i] == '\0')
+ continue;
+
+ return i + 1;
+ }
+
+ return 0;
+}
+
+static char *trim_whitespace_in_place(char *buffer)
+{
+ buffer += char_left_gc(buffer, strlen(buffer));
+ buffer[char_right_gc(buffer, strlen(buffer))] = '\0';
+ return buffer;
+}
+
+static pid_t get_pid_from_status_file(int *fd, const char *key, size_t keylen)
+{
+ int ret;
+ FILE *f;
+ size_t n = 0;
+ pid_t result = -1;
+ char *line = NULL;
+
+ /* fd now belongs to FILE and will be closed by fclose() */
+ f = fdopen(*fd, "r");
+ if (!f)
+ return -1;
+
+ while (getline(&line, &n, f) != -1) {
+ char *numstr;
+
+ if (strncmp(line, key, keylen))
+ continue;
+
+ numstr = trim_whitespace_in_place(line + 4);
+ ret = safe_int(numstr, &result);
+ if (ret < 0)
+ goto out;
+
+ break;
+ }
+
+out:
+ free(line);
+ fclose(f);
+ *fd = -1;
+ return result;
+}
+
+static int set_ns_last_pid(void)
+{
+ int fd;
+ ssize_t bytes;
+
+ fd = open("/proc/sys/kernel/ns_last_pid", O_WRONLY | O_CLOEXEC);
+ if (fd < 0) {
+ ksft_print_msg("failed to open \"/proc/sys/kernel/ns_last_pid\"n");
+ return -1;
+ }
+
+ bytes = write(fd, "999", sizeof("999") - 1);
+ close(fd);
+ if (bytes < 0 || (size_t)bytes != (sizeof("999") - 1))
+ return -1;
+
+ return 0;
+}
+
+static int test_pidfd_to_procfd_recycled_pid_fail(void)
+{
+ const char *test_name = "pidfd_open pidfd to procfd conversion on pid recycling";
+ int ret;
+ pid_t pid1;
+
+ ret = unshare(CLONE_NEWPID);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s test: Failed to unshare pid namespace\n",
+ test_name);
+
+ ret = unshare(CLONE_NEWNS);
+ if (ret < 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to unshare mount namespace\n",
+ test_name);
+
+ ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s test: Failed to remount / private\n",
+ test_name);
+
+ /* pid 1 in new pid namespace */
+ pid1 = fork();
+ if (pid1 < 0)
+ ksft_exit_fail_msg("%s test: Failed to create new process\n",
+ test_name);
+
+ /*
+ * This is lazy with file descriptor closing in the child process but
+ * it doesn't matter since we are very short-lived anyway and the
+ * cleanup would just be more complexity in this test.
+ */
+ if (pid1 == 0) {
+ int pidfd, procpidfd, procfd, ret;
+ pid_t pid2;
+
+
+ (void)umount2("/proc", MNT_DETACH);
+ ret = mount("proc", "/proc", "proc", 0, NULL);
+ if (ret < 0) {
+ ksft_print_msg("failed to mount a fresh /proc instance\n");
+ _exit(PIDFD_ERROR);
+ }
+
+ /* get pidfd for pid 1000 */
+ if (set_ns_last_pid() < 0) {
+ ksft_print_msg("failed to set ns_last_pid\n");
+ _exit(PIDFD_ERROR);
+ }
+
+ pid2 = fork();
+ if (pid2 < 0) {
+ ksft_print_msg("failed to create new process\n");
+ _exit(PIDFD_ERROR);
+ }
+
+ if (pid2 == 0)
+ _exit(PIDFD_PASS);
+
+ if (pid2 == PID_RECYCLE) {
+ pidfd = sys_pidfd_open(pid2, 0);
+ } else {
+ ksft_print_msg("failed to create process with pid %d\n",
+ PID_RECYCLE);
+ _exit(PIDFD_ERROR);
+ }
+
+ if (wait_for_pid(pid2))
+ _exit(PIDFD_ERROR);
+
+ if (pidfd < 0)
+ _exit(PIDFD_ERROR);
+
+ /* recycle pid 1000 */
+ if (set_ns_last_pid() < 0) {
+ ksft_print_msg("failed to set ns_last_pid\n");
+ _exit(PIDFD_ERROR);
+ }
+
+ pid2 = fork();
+ if (pid2 < 0) {
+ ksft_print_msg("failed to create new process\n");
+ _exit(PIDFD_ERROR);
+ }
+
+ if (pid2 == 0)
+ _exit(PIDFD_PASS);
+
+ if (pid2 != PID_RECYCLE) {
+ ksft_print_msg("failed to recycle pid %d\n",
+ PID_RECYCLE);
+ _exit(PIDFD_ERROR);
+ }
+
+ /*
+ * Now PID_RECYCLE is in zombie state since we have not waited
+ * on it yet, but it might have already exited. This ensures
+ * that the /proc/<pid> directory stays around.
+ */
+ procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+ if (procfd < 0) {
+ ksft_print_msg("%s - failed to open /proc\n",
+ strerror(errno));
+ _exit(PIDFD_ERROR);
+ }
+
+ procpidfd = ioctl(pidfd, PIDFD_GET_PROCFD, procfd);
+ if (procpidfd >= 0) {
+ ksft_print_msg(
+ "managed to get access to /proc/<pid> of recycled pid\n");
+ _exit(PIDFD_ERROR);
+ }
+
+ if (wait_for_pid(pid2))
+ _exit(PIDFD_ERROR);
+
+ _exit(PIDFD_PASS);
+ }
+
+ if (wait_for_pid(pid1) != 0)
+ return -1;
+
+ ksft_test_result_pass("%s test: passed\n", test_name);
+ ksft_inc_pass_cnt();
+ return 0;
+}
+
+int main(int argc, char **argv)
+{
+ int ret = 1;
+ int pidfd = -1, procfd = -1, procpidfd = -1, statusfd = -1;
+ pid_t pid;
+
+ pidfd = sys_pidfd_open(-1, 0);
+ if (pidfd >= 0) {
+ ksft_print_msg(
+ "%s - succeeded to open pidfd for invalid pid -1\n",
+ strerror(errno));
+ goto on_error;
+ }
+ ksft_test_result_pass("do not allow invalid pid test: passed\n");
+ ksft_inc_pass_cnt();
+
+ pidfd = sys_pidfd_open(getpid(), 1);
+ if (pidfd >= 0) {
+ ksft_print_msg(
+ "%s - succeeded to open pidfd with invalid flag value specified\n",
+ strerror(errno));
+ goto on_error;
+ }
+ ksft_test_result_pass("do not allow invalid flag test: passed\n");
+ ksft_inc_pass_cnt();
+
+ pidfd = sys_pidfd_open(getpid(), 0);
+ if (pidfd < 0) {
+ ksft_print_msg("%s - failed to open pidfd\n", strerror(errno));
+ goto on_error;
+ }
+ ksft_test_result_pass("open a new pidfd test: passed\n");
+ ksft_inc_pass_cnt();
+
+ procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+ if (procfd < 0) {
+ ksft_print_msg("%s - failed to open /proc\n", strerror(errno));
+ goto on_error;
+ }
+
+ procpidfd = ioctl(pidfd, PIDFD_GET_PROCFD, procfd);
+ if (procpidfd < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve /proc/<pid> from pidfd\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ statusfd = openat(procpidfd, "status", O_CLOEXEC | O_RDONLY);
+ if (statusfd < 0) {
+ ksft_print_msg("%s - failed to open /proc/<pid>/status\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ pid = get_pid_from_status_file(&statusfd, "Pid:", sizeof("Pid:") - 1);
+ if (pid < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve pid from /proc/<pid>/status\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ if (pid != getpid()) {
+ ksft_print_msg(
+ "%s - actual pid %d does not equal retrieved pid from /proc/<pid>/status\n",
+ strerror(errno), pid, getpid());
+ goto on_error;
+ }
+ ksft_test_result_pass("convert pidfd to /proc/<pid> fd test: passed\n");
+ ksft_inc_pass_cnt();
+
+ ret = test_pidfd_to_procfd_recycled_pid_fail();
+
+on_error:
+ if (pidfd >= 0)
+ close(pidfd);
+
+ if (procfd >= 0)
+ close(procfd);
+
+ if (procpidfd >= 0)
+ close(procpidfd);
+
+ if (statusfd >= 0)
+ close(statusfd);
+
+ return !ret ? ksft_exit_pass() : ksft_exit_fail();
+}
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index d59378a93782..f01de87249c9 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -14,6 +14,7 @@
#include <sys/wait.h>
#include <unistd.h>
+#include "pidfd.h"
#include "../kselftest.h"
static inline int sys_pidfd_send_signal(int pidfd, int sig, siginfo_t *info,
@@ -62,28 +63,6 @@ static int test_pidfd_send_signal_simple_success(void)
return 0;
}
-static int wait_for_pid(pid_t pid)
-{
- int status, ret;
-
-again:
- ret = waitpid(pid, &status, 0);
- if (ret == -1) {
- if (errno == EINTR)
- goto again;
-
- return -1;
- }
-
- if (ret != pid)
- goto again;
-
- if (!WIFEXITED(status))
- return -1;
-
- return WEXITSTATUS(status);
-}
-
static int test_pidfd_send_signal_exited_fail(void)
{
int pidfd, ret, saved_errno;
@@ -128,13 +107,6 @@ static int test_pidfd_send_signal_exited_fail(void)
return 0;
}
-/*
- * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
- * That means, when it wraps around any pid < 300 will be skipped.
- * So we need to use a pid > 300 in order to test recycling.
- */
-#define PID_RECYCLE 1000
-
/*
* Maximum number of cycles we allow. This is equivalent to PID_MAX_DEFAULT.
* If users set a higher limit or we have cycled PIDFD_MAX_DEFAULT number of
@@ -143,17 +115,6 @@ static int test_pidfd_send_signal_exited_fail(void)
*/
#define PIDFD_MAX_DEFAULT 0x8000
-/*
- * Define a few custom error codes for the child process to clearly indicate
- * what is happening. This way we can tell the difference between a system
- * error, a test error, etc.
- */
-#define PIDFD_PASS 0
-#define PIDFD_FAIL 1
-#define PIDFD_ERROR 2
-#define PIDFD_SKIP 3
-#define PIDFD_XFAIL 4
-
static int test_pidfd_send_signal_recycled_pid_fail(void)
{
int i, ret;
--
2.21.0
^ permalink raw reply related
* Re: [PATCHv8 07/10] acpi/hmat: Register processor domain to its memory
From: Dan Williams @ 2019-03-29 21:15 UTC (permalink / raw)
To: Keith Busch
Cc: Linux Kernel Mailing List, Linux ACPI, Linux MM, Linux API,
Greg Kroah-Hartman, Rafael Wysocki, Dave Hansen, Jonathan Cameron,
Brice Goglin
In-Reply-To: <20190311205606.11228-8-keith.busch@intel.com>
On Mon, Mar 11, 2019 at 1:55 PM Keith Busch <keith.busch@intel.com> wrote:
>
> If the HMAT Subsystem Address Range provides a valid processor proximity
> domain for a memory domain, or a processor domain matches the performance
> access of the valid processor proximity domain, register the memory
> target with that initiator so this relationship will be visible under
> the node's sysfs directory.
>
> Since HMAT requires valid address ranges have an equivalent SRAT entry,
> verify each memory target satisfies this requirement.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
> drivers/acpi/hmat/Kconfig | 3 +-
> drivers/acpi/hmat/hmat.c | 392 +++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 393 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/hmat/Kconfig b/drivers/acpi/hmat/Kconfig
> index 2f7111b7af62..13cddd612a52 100644
> --- a/drivers/acpi/hmat/Kconfig
> +++ b/drivers/acpi/hmat/Kconfig
> @@ -4,4 +4,5 @@ config ACPI_HMAT
> depends on ACPI_NUMA
> help
> If set, this option has the kernel parse and report the
> - platform's ACPI HMAT (Heterogeneous Memory Attributes Table).
> + platform's ACPI HMAT (Heterogeneous Memory Attributes Table),
> + and register memory initiators with their targets.
> diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c
> index 4758beb3b2c1..01a6eddac6f7 100644
> --- a/drivers/acpi/hmat/hmat.c
> +++ b/drivers/acpi/hmat/hmat.c
> @@ -13,11 +13,105 @@
> #include <linux/device.h>
> #include <linux/init.h>
> #include <linux/list.h>
> +#include <linux/list_sort.h>
> #include <linux/node.h>
> #include <linux/sysfs.h>
>
> static __initdata u8 hmat_revision;
>
> +static __initdata LIST_HEAD(targets);
> +static __initdata LIST_HEAD(initiators);
> +static __initdata LIST_HEAD(localities);
> +
> +/*
> + * The defined enum order is used to prioritize attributes to break ties when
> + * selecting the best performing node.
> + */
> +enum locality_types {
> + WRITE_LATENCY,
> + READ_LATENCY,
> + WRITE_BANDWIDTH,
> + READ_BANDWIDTH,
> +};
> +
> +static struct memory_locality *localities_types[4];
> +
> +struct memory_target {
> + struct list_head node;
> + unsigned int memory_pxm;
> + unsigned int processor_pxm;
> + struct node_hmem_attrs hmem_attrs;
> +};
> +
> +struct memory_initiator {
> + struct list_head node;
> + unsigned int processor_pxm;
> +};
> +
> +struct memory_locality {
> + struct list_head node;
> + struct acpi_hmat_locality *hmat_loc;
> +};
> +
> +static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
> +{
> + struct memory_initiator *initiator;
> +
> + list_for_each_entry(initiator, &initiators, node)
> + if (initiator->processor_pxm == cpu_pxm)
> + return initiator;
> + return NULL;
> +}
> +
> +static __init struct memory_target *find_mem_target(unsigned int mem_pxm)
> +{
> + struct memory_target *target;
> +
> + list_for_each_entry(target, &targets, node)
> + if (target->memory_pxm == mem_pxm)
> + return target;
> + return NULL;
The above implementation assumes that every SRAT entry has a unique
@mem_pxm. I don't think that's valid if the memory map is sparse,
right?
^ permalink raw reply
* Re: [PATCH v2 2/5] pid: add pidfd_open()
From: Jann Horn @ 2019-03-29 23:45 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190329155425.26059-3-christian@brauner.io>
On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> /* Introduction */
> This adds the pidfd_open() syscall.
> pidfd_open() allows to retrieve file descriptors for a given pid. This
> includes both file descriptors for processes and file descriptors for
> threads.
Looks good to me, overall. Apart from a few nits below:
Reviewed-by: Jann Horn <jannh@google.com>
[...]
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 20881598bdfa..8c9e15e0e463 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
[...]
> +static struct file *pidfd_open_proc_pid(const struct file *procf, pid_t pid,
> + const struct pid *pidfd_pid)
> +{
> + char name[12]; /* int to strlen + \0 but with */
nit: comment suddenly ends at "but with"?
[...]
> +}
> +
> +static inline int pidfd_to_procfd(int procfd, struct file *pidfd_file)
> +{
> + long fd;
nit: This should probably be an int?
[...]
> + return fd;
> +}
[...]
> +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + int procfd = arg;
nit: I think it'd be semantically cleaner to move this assignment into
the switch case, but I don't feel about it strongly.
> + switch (cmd) {
> + case PIDFD_GET_PROCFD:
> + return pidfd_to_procfd(procfd, file);
> + default:
> + return -ENOTTY;
> + }
> +}
^ permalink raw reply
* Re: [PATCH v2 2/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-29 23:55 UTC (permalink / raw)
To: Jann Horn
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <CAG48ez0Arzdtjxj9zVVTaL=T6tX16e6OcR75rtk9mavCbnMK6w@mail.gmail.com>
On Sat, Mar 30, 2019 at 12:45:46AM +0100, Jann Horn wrote:
> On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> > /* Introduction */
> > This adds the pidfd_open() syscall.
> > pidfd_open() allows to retrieve file descriptors for a given pid. This
> > includes both file descriptors for processes and file descriptors for
> > threads.
>
> Looks good to me, overall. Apart from a few nits below:
> Reviewed-by: Jann Horn <jannh@google.com>
Thanks! Will fixup the nits and add your Reviewed-by!
>
> [...]
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 20881598bdfa..8c9e15e0e463 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> [...]
> > +static struct file *pidfd_open_proc_pid(const struct file *procf, pid_t pid,
> > + const struct pid *pidfd_pid)
> > +{
> > + char name[12]; /* int to strlen + \0 but with */
>
> nit: comment suddenly ends at "but with"?
Will fix.
>
> [...]
> > +}
> > +
> > +static inline int pidfd_to_procfd(int procfd, struct file *pidfd_file)
> > +{
> > + long fd;
>
> nit: This should probably be an int?
Yes.
>
> [...]
> > + return fd;
> > +}
> [...]
> > +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > + int procfd = arg;
>
> nit: I think it'd be semantically cleaner to move this assignment into
> the switch case, but I don't feel about it strongly.
Agreed.
>
> > + switch (cmd) {
> > + case PIDFD_GET_PROCFD:
> > + return pidfd_to_procfd(procfd, file);
> > + default:
> > + return -ENOTTY;
> > + }
> > +}
^ permalink raw reply
* Re: [PATCH v2 4/5] signal: PIDFD_SIGNAL_TID threads via pidfds
From: Jann Horn @ 2019-03-30 1:06 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, David Howells, Serge E. Hallyn, Linux API,
kernel list, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton, Oleg Nesterov,
Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190329155425.26059-5-christian@brauner.io>
On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@brauner.io> wrote:
> With the addition of pidfd_open() it is possible for users to reference a
> specific thread by doing:
>
> int pidfd = pidfd_open(<tid>, 0);
>
> This means we can extend pidfd_send_signal() to signal a specific thread.
> As promised in the commit for pidfd_send_signal() [1] the extension is
> based on a flag argument, i.e. the scope of the signal delivery is based on
> the flag argument, not on the type of file descriptor.
> To this end the flag PIDFD_SIGNAL_TID is added. With this change we now
> cover most of the functionality of all the other signal sending functions
> combined:
[...]
> diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h
> index d6c7c0701997..b72f0ef84fe5 100644
> --- a/include/uapi/linux/wait.h
> +++ b/include/uapi/linux/wait.h
[...]
> +/* Flags to pass to pidfd_send_signal */
> +#define PIDFD_SIGNAL_TID 1 /* Send signal to specific thread */
nit: s/1/1U/; the flags argument is an `unsigned int`
> #endif /* _UAPI_LINUX_WAIT_H */
> diff --git a/kernel/signal.c b/kernel/signal.c
> index eb97d0cc6ef7..9f93da85b2b9 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
[...]
> +static int pidfd_send_signal_specific(struct pid *pid, int sig,
> + struct kernel_siginfo *info)
> +{
> + struct task_struct *p;
> + int error = -ESRCH;
> +
> + rcu_read_lock();
> + p = pid_task(pid, PIDTYPE_PID);
> + if (p)
> + error = __do_send_specific(p, sig, info);
> + rcu_read_unlock();
> +
> + return error;
> +}
> +
> /**
> - * sys_pidfd_send_signal - send a signal to a process through a task file
> - * descriptor
> + * sys_pidfd_send_signal - send a signal to a process through a pidfd
> +
> * @pidfd: the file descriptor of the process
> * @sig: signal to be sent
> * @info: the signal info
> * @flags: future flags to be passed
nit: comment is outdated, it isn't "future flags" anymore
[...]
> + * rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
> + * - pidfd_send_signal(<pidfd>, <sig>, <info>, PIDFD_SIGNAL_TID);
> + * which is equivalent to
> + * rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>)
> + *
> * In order to extend the syscall to threads and process groups the @flags
> * argument should be used. In essence, the @flags argument will determine
> * what is signaled and not the file descriptor itself. Put in other words,
nit: again, outdated comment about @flags
[...]
> @@ -3626,43 +3695,16 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> prepare_kill_siginfo(sig, &kinfo);
> }
>
> - ret = kill_pid_info(sig, &kinfo, pid);
> + if (flags & PIDFD_SIGNAL_TID)
> + ret = pidfd_send_signal_specific(pid, sig, &kinfo);
> + else
> + ret = kill_pid_info(sig, &kinfo, pid);
nit: maybe give pidfd_send_signal_specific() and kill_pid_info() the
same signatures, since they perform similar operations with the same
argument types?
Something that was already kinda weird in the existing code, but is
getting worse with TIDs is the handling of SI_USER with siginfo.
Copying context lines from above here:
if (info) {
ret = copy_siginfo_from_user_any(&kinfo, info);
if (unlikely(ret))
goto err;
ret = -EINVAL;
if (unlikely(sig != kinfo.si_signo))
goto err;
if ((task_pid(current) != pid) &&
(kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
/* Only allow sending arbitrary signals to yourself. */
ret = -EPERM;
if (kinfo.si_code != SI_USER)
goto err;
/* Turn this into a regular kill signal. */
prepare_kill_siginfo(sig, &kinfo);
}
} else {
prepare_kill_siginfo(sig, &kinfo);
}
So for signals to PIDs, the rule is that if you send siginfo with
SI_USER to yourself, the siginfo is preserved; otherwise the kernel
silently clobbers it. That's already kind of weird - silent behavior
difference depending on a security check. But now, for signals to
threads, I think the result is going to be that signalling the thread
group leader preserves information, and signalling any other thread
clobbers it? If so, that seems bad.
do_rt_sigqueueinfo() seems to have the same issue, from a glance - but
there, at least the error case is just a -EPERM, not a silent behavior
difference.
Would it make sense to refuse sending siginfo with SI_USER to
non-current? If you actually want to send a normal SI_USER signal, you
can use info==NULL, right? That should create wrongness parity with
do_rt_sigqueueinfo().
To improve things further, I guess you'd have to move the comparison
against current into pidfd_send_signal_specific(), or move the task
lookup out of it, or something like that?
> err:
> fdput(f);
> return ret;
> }
[...]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox