* Re: [PATCH 08/17] fpga: dfl: afu: add userclock sysfs interfaces.
From: Alan Tull @ 2019-04-01 21:41 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api,
Ananda Ravuri, Russ Weight, Xu Yilun
In-Reply-To: <1553483264-5379-9-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,
Looks fine.
>
> This patch introduces userclock sysfs interfaces for AFU, user
> could use these interfaces for clock setting to AFU.
>
> Please note that, this is only working for port header feature
> with revision 0, for later revisions, userclock setting is moved
> to a separated private feature, so one revision sysfs interface
> is exposed to userspace application for this purpose too.
>
> Signed-off-by: Ananda Ravuri <ananda.ravuri@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@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 v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-04-01 21:30 UTC (permalink / raw)
To: Christian Brauner
Cc: Jann Horn, Daniel Colascione, Aleksa Sarai, Andy Lutomirski,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <20190401194214.4rbvmwogpke3cjcx@brauner.io>
On Mon, Apr 1, 2019 at 12:42 PM Christian Brauner <christian@brauner.io> wrote:
>
> From what I gather from this thread we are still best of with using fds
> to /proc/<pid> as pidfds. Linus, do you agree or have I misunderstood?
That does seem to be the most flexible option.
> Yes, we can have an internal mount option to restrict access to various
> parts of procfs from such pidfds
I suspect you'd find that other parties might want such a "restrict
proc" mount option too, so I don't think it needs to be anything
internal.
But it would be pretty much independent of the pidfd issue, of course.
> One thing is that we also need something to disable access to the
> "/proc/<pid>/net". One option could be to give the files in "net/" an
> ->open-handler which checks that our file->f_path.mnt is not one of our
> special clone() mounts and if it is refuse the open.
I would expect that that would be part of the "restrict proc" mount options, no?
> Basically, if you have a system without CONFIG_PROC_FS it makes sense
> that clone gives back an anon inode file descriptor as pidfds because
> you can still signal threads in a race-free way. But it doesn't make a
> lot of sense to have pidfd_open() in this scenario because you can't
> really do anything with that pidfd apart from sending signals.
Well, people might want that.
But realistically, everybody enables /proc support anyway. Even if you
don't actually fully *mount* it in some restricted area, the support
is pretty much always there in any kernel config.
But yes, in general I agree that that also most likely means that a
separate system call for "open_pidfd()" isn't worth it.
Because if the main objection to /proc is that it exposes too much,
then I think a much better option is to see how to sanely restrict the
"too much" parts.
Because I think there might be a lot of people who want a restricted
/proc, in various container models etc.
Linus
^ permalink raw reply
* Re: [PATCH ghak90 V5 06/10] audit: add support for non-syscall auxiliary records
From: Richard Guy Briggs @ 2019-04-01 20:43 UTC (permalink / raw)
To: Paul Moore
Cc: nhorman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhTL=zSQ2tO440w+vvX3TPsGtwqL6dWwUHVe0WnHVKqVpA@mail.gmail.com>
On 2019-04-01 14:57, Paul Moore wrote:
> On Mon, Apr 1, 2019 at 1:44 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-04-01 10:49, Paul Moore wrote:
> > > On Fri, Mar 15, 2019 at 2:34 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Standalone audit records have the timestamp and serial number generated
> > > > on the fly and as such are unique, making them standalone. This new
> > > > function audit_alloc_local() generates a local audit context that will
> > > > be used only for a standalone record and its auxiliary record(s). The
> > > > context is discarded immediately after the local associated records are
> > > > produced.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > ---
> > > > include/linux/audit.h | 8 ++++++++
> > > > kernel/audit.h | 1 +
> > > > kernel/auditsc.c | 35 ++++++++++++++++++++++++++++++-----
> > > > 3 files changed, 39 insertions(+), 5 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index ebd6625ca80e..6db5aba7cc01 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -285,6 +285,8 @@ static inline void audit_log_contid(struct audit_context *context, u64 contid)
> > > >
> > > > /* These are defined in auditsc.c */
> > > > /* Public API */
> > > > +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> > > > +extern void audit_free_context(struct audit_context *context);
> > > > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> > > > unsigned long a2, unsigned long a3);
> > > > extern void __audit_syscall_exit(int ret_success, long ret_value);
> > > > @@ -512,6 +514,12 @@ static inline void audit_fanotify(unsigned int response)
> > > > extern int audit_n_rules;
> > > > extern int audit_signals;
> > > > #else /* CONFIG_AUDITSYSCALL */
> > > > +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> > > > +{
> > > > + return NULL;
> > > > +}
> > > > +static inline void audit_free_context(struct audit_context *context)
> > > > +{ }
> > > > static inline void audit_syscall_entry(int major, unsigned long a0,
> > > > unsigned long a1, unsigned long a2,
> > > > unsigned long a3)
> > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > index c5ac6436317e..2a1a8b8a8019 100644
> > > > --- a/kernel/audit.h
> > > > +++ b/kernel/audit.h
> > > > @@ -111,6 +111,7 @@ struct audit_proctitle {
> > > > struct audit_context {
> > > > int dummy; /* must be the first element */
> > > > int in_syscall; /* 1 if task is in a syscall */
> > > > + bool local; /* local context needed */
> > >
> > > It's very possible I've missed it, but "local" never gets used in any
> > > meaningful way in this patchset does it?
> >
> > It is used in audit_alloc_local() to signal to audit_get_stamp() that in
> > absence of a syscall flag, check for a local flag to continue rather
> > than return.
>
> I'm guessing you meant auditsc_get_stamp()? If so, okay.
Yes. (I thought I remembered typing "auditsc...".)
> paul moore
- 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: Richard Guy Briggs @ 2019-04-01 20:41 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <CAHC9VhQpk6cFZckQH+LQmQmirVzLm_GH57ZD3-76y-59G=DocQ@mail.gmail.com>
On 2019-04-01 10:50, Paul Moore wrote:
> On Fri, Mar 15, 2019 at 2: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/kernel/audit.c b/kernel/audit.c
> > index cf448599ef34..7fa3194f5342 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <net/net_namespace.h>
> >
> > #include "audit.h"
> >
> > @@ -99,9 +100,13 @@
> > /**
> > * struct audit_net - audit private network namespace data
> > * @sk: communication socket
> > + * @contid_list: audit container identifier list
> > + * @contid_list_lock audit container identifier list lock
> > */
> > struct audit_net {
> > struct sock *sk;
> > + struct list_head contid_list;
> > + spinlock_t contid_list_lock;
> > };
> >
> > /**
> > @@ -275,8 +280,11 @@ struct audit_task_info init_struct_audit = {
> > void audit_free(struct task_struct *tsk)
> > {
> > struct audit_task_info *info = tsk->audit;
> > + struct nsproxy *ns = tsk->nsproxy;
> >
> > audit_free_syscall(tsk);
> > + if (ns)
> > + audit_netns_contid_del(ns->net_ns, audit_get_contid(tsk));
> > /* Freeing the audit_task_info struct must be performed after
> > * audit_log_exit() due to need for loginuid and sessionid.
> > */
> > @@ -376,6 +384,73 @@ static struct sock *audit_get_sk(const struct net *net)
> > return aunet->sk;
> > }
> >
> > +void audit_netns_contid_add(struct net *net, u64 contid)
> > +{
> > + struct audit_net *aunet = net_generic(net, audit_net_id);
> > + struct list_head *contid_list = &aunet->contid_list;
> > + struct audit_contid *cont;
> > +
> > + if (!audit_contid_valid(contid))
> > + return;
> > + if (!aunet)
> > + return;
>
> We should move the contid_list assignment below this check, or decide
> that aunet is always going to valid (?) and get rid of this check
> completely.
Yes, we should move the contid_list assignment below the aunet check.
At this point, testing has revlealed that audit_netns_contid_del() is
being called very infrequently (very early) with invalid net or aunet
which is why its code has been re-arranged to safeguard against it. So
far, audit_netns_contid_add() has not had this problem, but I'd feel
safer leaving the check in and to make that check relevant (rather than
the contid_list assignment panicing) I'll move that assignment.
> > + spin_lock(&aunet->contid_list_lock);
> > + if (!list_empty(contid_list))
>
> We don't need the list_empty() check here do we? I think we can just
> call list_for_each_entry_rcu(), yes?
Yes, you appear to be correct. The examples in auditfilter.c and
auditsc.c are stale since the original optimizations they were
protecting are no longer present, but the practice was copied by several
maintainers since. I'll fix this and address the others seperately.
> > + list_for_each_entry_rcu(cont, contid_list, list)
> > + if (cont->id == contid) {
> > + refcount_inc(&cont->refcount);
> > + goto out;
> > + }
> > + cont = kmalloc(sizeof(struct audit_contid), GFP_ATOMIC);
>
> If you had to guess, what do you think is going to be more common:
> bumping the refcount of an existing entry in the list, or adding a new
> entry? I'm asking because I always get a little nervous when doing
> allocations while holding a spinlock. Yes, you are doing it with
> GFP_ATOMIC, but it still seems like something to try and avoid if this
> is going to approach 50%. However, if the new entry is rare then the
> extra work of always doing the allocation before taking the lock and
> then freeing it afterwards might be a bad tradeoff.
>
> My gut feeling says we might do about as many allocations as refcount
> bumps, but I could be thinking about this wrong.
I'm sure we could find extreme use cases in both directions. One
extreme is one task per container. The other extreme is a distro
container where all processes for an entire O/S running in a container
just use the refcount. The former seems more likely at this point. We
break even at two tasks per container.
I'm willing to switch this over to alloc/dealloc and use GFP_KERNEL
which will inevitably make some kernel maintainers happier and others
cringe. This could be a good candidate for kmem_cache since it is a
fixed size.
> Moving the allocation outside the spinlock might also open the door to
> doing this as GFP_KERNEL, which is a good thing, but I haven't looked
> at the callers to see if that is possible (it may not be). That's an
> exercise left to the patch author (if he hasn't done that already).
Switching to GFP_KERNEL is certainly preferable. None of the callers
will have a problem with that.
> > + if (cont) {
> > + INIT_LIST_HEAD(&cont->list);
>
> Unless there is some guidance that INIT_LIST_HEAD() should be used
> regardless, you shouldn't need to call this here since list_add_rcu()
> will take care of any list.h related initialization.
I see enough examples without that suggest it isn't necessary. I'll
remove it.
> > + cont->id = contid;
> > + refcount_set(&cont->refcount, 1);
> > + list_add_rcu(&cont->list, contid_list);
> > + }
> > +out:
> > + spin_unlock(&aunet->contid_list_lock);
> > +}
>
> paul moore
- 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 01/17] fpga: dfl-fme-mgr: fix FME_PR_INTFC_ID register address.
From: Moritz Fischer @ 2019-04-01 19:54 UTC (permalink / raw)
To: Wu Hao; +Cc: atull, mdf, linux-fpga, linux-kernel, linux-api
In-Reply-To: <1553483264-5379-2-git-send-email-hao.wu@intel.com>
Hi Wu,
On Mon, Mar 25, 2019 at 11:07:28AM +0800, Wu Hao wrote:
> FME_PR_INTFC_ID is used as compat_id for fpga manager and region,
> but high 64 bits and low 64 bits of the compat_id are swapped by
> mistake. This patch fixes this problem by fixing register address.
>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> ---
> drivers/fpga/dfl-fme-mgr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index 76f3770..b3f7eee 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -30,8 +30,8 @@
> #define FME_PR_STS 0x10
> #define FME_PR_DATA 0x18
> #define FME_PR_ERR 0x20
> -#define FME_PR_INTFC_ID_H 0xA8
> -#define FME_PR_INTFC_ID_L 0xB0
> +#define FME_PR_INTFC_ID_L 0xA8
> +#define FME_PR_INTFC_ID_H 0xB0
Does this handle endianess correct?
>
> /* FME PR Control Register Bitfield */
> #define FME_PR_CTRL_PR_RST BIT_ULL(0) /* Reset PR engine */
> --
> 2.7.4
>
Cheers,
Moritz
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-04-01 19:42 UTC (permalink / raw)
To: Linus Torvalds, Jann Horn
Cc: Daniel Colascione, Aleksa Sarai, Andy Lutomirski,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, 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 <oleg>
In-Reply-To: <CAHk-=wgKOJTdN9TqyjW6FngEZvgUTmCDzJPRrtekW_1SD0gfhw@mail.gmail.com>
On Mon, Apr 01, 2019 at 09:01:29AM -0700, Linus Torvalds wrote:
> On Mon, Apr 1, 2019 at 8:55 AM Daniel Colascione <dancol@google.com> wrote:
> >
> >
> > > I wonder if we really want a fill procfs2, or maybe we could just make
> > > the pidfd readable (yes, it's a directory file descriptor, but we
> > > could allow reading).
> >
> > What would read(2) read?
>
> We could make it read anything, but it would have to be something
> people agree is sufficient (and not so expensive to create that rare
> users of that data would find the overhead excessive).
>
> Eg we could make it return the same thing that /proc/<pid>/status
> reads right now.
>
> But it sounds like you need pretty much all of /proc/<pid>/xyz:
>From what I gather from this thread we are still best of with using fds
to /proc/<pid> as pidfds. Linus, do you agree or have I misunderstood?
Yes, we can have an internal mount option to restrict access to various
parts of procfs from such pidfds or do the parent-less bind-mount trick
but I think this beats having a stunted dummy dirfd that we implement a
read method on.
One thing is that we also need something to disable access to the
"/proc/<pid>/net". One option could be to give the files in "net/" an
->open-handler which checks that our file->f_path.mnt is not one of our
special clone() mounts and if it is refuse the open.
To clarify the way forward:
Jann and I were discussing whether pidfd_open() still makes sense and
whether I shouldn't just jump straight to a first version of
CLONE_PIDFD.
Basically, if you have a system without CONFIG_PROC_FS it makes sense
that clone gives back an anon inode file descriptor as pidfds because
you can still signal threads in a race-free way. But it doesn't make a
lot of sense to have pidfd_open() in this scenario because you can't
really do anything with that pidfd apart from sending signals. And on a
system like that sending a signal is still racy. Since the process can
be recycled between learning the pid number and calling pidfd_open()
[1]. So it only makes sense to have _clone()_ give back anon_inode() fds
on a system without CONFIG_PROC_FS but it doesn't make sense for
pidfd_open() In other news, I think it makes more sense if I jump to the
implementation of CLONE_PIDFD instead of working on pidfd_open().
[1]: The only case - that seems rather far-fetched - where it makes
sense is when the parent wants to create that pidfd and hand it to
someone else.
Christian
^ permalink raw reply
* Re: [PATCH ghak90 V5 06/10] audit: add support for non-syscall auxiliary records
From: Paul Moore @ 2019-04-01 18:57 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <20190401174432.fjnvotpvdrfujoy7@madcap2.tricolour.ca>
On Mon, Apr 1, 2019 at 1:44 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-04-01 10:49, Paul Moore wrote:
> > On Fri, Mar 15, 2019 at 2:34 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Standalone audit records have the timestamp and serial number generated
> > > on the fly and as such are unique, making them standalone. This new
> > > function audit_alloc_local() generates a local audit context that will
> > > be used only for a standalone record and its auxiliary record(s). The
> > > context is discarded immediately after the local associated records are
> > > produced.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > ---
> > > include/linux/audit.h | 8 ++++++++
> > > kernel/audit.h | 1 +
> > > kernel/auditsc.c | 35 ++++++++++++++++++++++++++++++-----
> > > 3 files changed, 39 insertions(+), 5 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index ebd6625ca80e..6db5aba7cc01 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -285,6 +285,8 @@ static inline void audit_log_contid(struct audit_context *context, u64 contid)
> > >
> > > /* These are defined in auditsc.c */
> > > /* Public API */
> > > +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> > > +extern void audit_free_context(struct audit_context *context);
> > > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> > > unsigned long a2, unsigned long a3);
> > > extern void __audit_syscall_exit(int ret_success, long ret_value);
> > > @@ -512,6 +514,12 @@ static inline void audit_fanotify(unsigned int response)
> > > extern int audit_n_rules;
> > > extern int audit_signals;
> > > #else /* CONFIG_AUDITSYSCALL */
> > > +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> > > +{
> > > + return NULL;
> > > +}
> > > +static inline void audit_free_context(struct audit_context *context)
> > > +{ }
> > > static inline void audit_syscall_entry(int major, unsigned long a0,
> > > unsigned long a1, unsigned long a2,
> > > unsigned long a3)
> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index c5ac6436317e..2a1a8b8a8019 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -111,6 +111,7 @@ struct audit_proctitle {
> > > struct audit_context {
> > > int dummy; /* must be the first element */
> > > int in_syscall; /* 1 if task is in a syscall */
> > > + bool local; /* local context needed */
> >
> > It's very possible I've missed it, but "local" never gets used in any
> > meaningful way in this patchset does it?
>
> It is used in audit_alloc_local() to signal to audit_get_stamp() that in
> absence of a syscall flag, check for a local flag to continue rather
> than return.
I'm guessing you meant auditsc_get_stamp()? If so, okay.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Richard Guy Briggs @ 2019-04-01 17:50 UTC (permalink / raw)
To: Paul Moore
Cc: nhorman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhQpNGaWeb7dY4ooPKXRoGBd9Ri_Z_ZPpoKun4gUhXDOGA@mail.gmail.com>
On 2019-04-01 10:50, Paul Moore wrote:
> On Fri, Mar 15, 2019 at 2:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> > event standalone records. Iterate through all potential audit container
> > identifiers associated with a network namespace.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 5 +++++
> > kernel/audit.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > net/netfilter/nft_log.c | 11 +++++++++--
> > net/netfilter/xt_AUDIT.c | 11 +++++++++--
> > 4 files changed, 64 insertions(+), 4 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 7fa3194f5342..80ed323feeb5 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -451,6 +451,47 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > audit_netns_contid_add(new->net_ns, contid);
> > }
> >
> > +/**
> > + * audit_log_netns_contid_list - List contids for the given network namespace
> > + * @net: the network namespace of interest
> > + * @context: the audit context to use
> > + *
> > + * Description:
> > + * Issues a CONTAINER_ID record with a CSV list of contids associated
> > + * with a network namespace to accompany a NETFILTER_PKT record.
> > + */
> > +void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
> > +{
> > + struct audit_buffer *ab = NULL;
> > + struct audit_contid *cont;
> > + bool first = true;
> > + struct audit_net *aunet;
> > +
> > + /* Generate AUDIT_CONTAINER_ID record with container ID CSV list */
> > + rcu_read_lock();
> > + aunet = net_generic(net, audit_net_id);
> > + if (!aunet)
> > + goto out;
> > + list_for_each_entry_rcu(cont, &aunet->contid_list, list) {
> > + if (first) {
>
> This is borderline nit-picky, but it seems like we could get rid of
> "first" and just check to see if "ab" is still NULL.
Yes, this is a better way, thank you.
> > + ab = audit_log_start(context, GFP_ATOMIC,
> > + AUDIT_CONTAINER_ID);
> > + if (!ab) {
> > + audit_log_lost("out of memory in audit_log_netns_contid_list");
> > + goto out;
> > + }
> > + audit_log_format(ab, "contid=");
> > + } else
> > + audit_log_format(ab, ",");
> > + audit_log_format(ab, "%llu", cont->id);
> > + first = false;
> > + }
> > + audit_log_end(ab);
> > +out:
> > + rcu_read_unlock();
> > +}
> > +EXPORT_SYMBOL(audit_log_netns_contid_list);
>
> paul moore
- 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 06/10] audit: add support for non-syscall auxiliary records
From: Richard Guy Briggs @ 2019-04-01 17:44 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <CAHC9VhRWUvG6DwiPn4o+vt=fgGfY+JmG8m78M8CeyGYxF2yRXQ@mail.gmail.com>
On 2019-04-01 10:49, Paul Moore wrote:
> On Fri, Mar 15, 2019 at 2:34 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Standalone audit records have the timestamp and serial number generated
> > on the fly and as such are unique, making them standalone. This new
> > function audit_alloc_local() generates a local audit context that will
> > be used only for a standalone record and its auxiliary record(s). The
> > context is discarded immediately after the local associated records are
> > produced.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > ---
> > include/linux/audit.h | 8 ++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 35 ++++++++++++++++++++++++++++++-----
> > 3 files changed, 39 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index ebd6625ca80e..6db5aba7cc01 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -285,6 +285,8 @@ static inline void audit_log_contid(struct audit_context *context, u64 contid)
> >
> > /* These are defined in auditsc.c */
> > /* Public API */
> > +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> > +extern void audit_free_context(struct audit_context *context);
> > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> > unsigned long a2, unsigned long a3);
> > extern void __audit_syscall_exit(int ret_success, long ret_value);
> > @@ -512,6 +514,12 @@ static inline void audit_fanotify(unsigned int response)
> > extern int audit_n_rules;
> > extern int audit_signals;
> > #else /* CONFIG_AUDITSYSCALL */
> > +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> > +{
> > + return NULL;
> > +}
> > +static inline void audit_free_context(struct audit_context *context)
> > +{ }
> > static inline void audit_syscall_entry(int major, unsigned long a0,
> > unsigned long a1, unsigned long a2,
> > unsigned long a3)
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index c5ac6436317e..2a1a8b8a8019 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -111,6 +111,7 @@ struct audit_proctitle {
> > struct audit_context {
> > int dummy; /* must be the first element */
> > int in_syscall; /* 1 if task is in a syscall */
> > + bool local; /* local context needed */
>
> It's very possible I've missed it, but "local" never gets used in any
> meaningful way in this patchset does it?
It is used in audit_alloc_local() to signal to audit_get_stamp() that in
absence of a syscall flag, check for a local flag to continue rather
than return.
> > enum audit_state state, current_state;
> > unsigned int serial; /* serial number for record */
> > int major; /* syscall number */
>
> paul moore
- RGB
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-04-01 17:32 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jonathan Kowalski, Aleksa Sarai, Andy Lutomirski,
Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAKOZuet2QCoW1XMpTAzXS868PhF-gh14czFn4gwE4A=oX=Yr8w@mail.gmail.com>
On Mon, Apr 1, 2019 at 9:45 AM Daniel Colascione <dancol@google.com> wrote:
>
> But doesn't the CSIGNAL approach still require that libraries somehow
> coordinate which non-SIGCHLD signal they use?
Yes. As mentioned, this was seldom used.
In some cases it's ok, eg aio would just specify the signal in
sigev_signo (except it was never fleshed out to have sival_ptr etc
because nobody ever did it, afaik).
> (Signal coordination a separate problem, unfortunately.)
Yeah, in several cases you would just want to signal a handler
directly, not an index into handlers.
Anyway, I'm just saying that the whole "reap children" part is not
necessarily tied to the pidfd thing. And honestly, nobody ever does
anything like that anyway in practice, because of all the other issues
it causes that you'll hit.
Linus
^ permalink raw reply
* RE: [PATCH v2 0/5] pid: add pidfd_open()
From: David Laight @ 2019-04-01 17:00 UTC (permalink / raw)
To: 'Daniel Colascione', Linus Torvalds
Cc: Jonathan Kowalski, Aleksa Sarai, Andy Lutomirski,
Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAKOZuet2QCoW1XMpTAzXS868PhF-gh14czFn4gwE4A=oX=Yr8w@mail.gmail.com>
From: Daniel Colascione
> Sent: 01 April 2019 17:45
...
> > Now, none of this was ever really used. The people who wanted AIO
> > wanted the brain-damaged POSIX kind of AIO, not something cool and
> > clever. So I suspect the whole exit-signal thing was just used for
> > random small per-project things.
>
> AIO is a whole other ball of wax. :-)
For AIO you need to go back and tell K&R to use the IO_QIO stuff from
RSX/11M rather than the 'IO in process context' they used for Unix :-)
Then you'd end up with the windows kernel.....
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-04-01 16:45 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jonathan Kowalski, Aleksa Sarai, Andy Lutomirski,
Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAHk-=wjgENV0pT8=MjB1UVBeWCrECCNXv15AKKX7Kfg7Eak7qw@mail.gmail.com>
On Mon, Apr 1, 2019 at 9:30 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Apr 1, 2019 at 9:22 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > There's a subtlety: suppose I'm a library and I want to create a
> > private subprocess. I use the new clone facility, whatever it is, and
> > get a pidfd back. I need to be able to read the child's exit status
> > even if the child exits before clone returns in the parent. Doesn't
> > this requirement imply that the pidfd, kernel-side, contain something
> > a bit more than a struct pid?
>
> Note that clone() has always supported this, but it has basically
> never been used.
>
> One of the early thinkings behind clone() was that it could be used
> for basically "AIO in user space", by having exactly these kinds of
> library-private internal subthreads that are hidden as real threads.
>
> It's why we have that special CSIGNAL mask for setting the exit
> signal. It doesn't *just* set the signal to be sent when the thread
> exits - setting the signal to something else than SIGCHLD actually
> changes behavior in other ways too.
>
> In particular a non-SIGCHLD thread won't be found by a regular
> "waitpid()". You have to explicitly wait for it with __WCLONE (which
> in turn is supposed to be used with the explicit pid to be waited
> for).
But doesn't the CSIGNAL approach still require that libraries somehow
coordinate which non-SIGCHLD signal they use? (Signal coordination a
separate problem, unfortunately.) If you don't set a signal, you don't
get any notification at all, and in that case, you have to burn a
thread on a blocking wait, right? And you don't *have* to wait for a
specific PID with __WCLONE. If two libraries each did a separate
__WCLONE or __WALL wait on all subprocesses, they'd still interfere,
AIUI, even if the interface isn't supposed to be used that way.
I like the pidfd way of doing private subprocesses a bit more than the
CSIGNAL approach because it's more "natural" (in that a pidfd is just
a handle like we have handles to tons of other things), integrates
with existing event and wait facilities, doesn't require any global
coordination at all, and works for more than this one use case.
(CSIGNAL is limited to immediate parents. You could, in principle,
SCM_RIGHTS a pidfd to someone else.)
> Now, none of this was ever really used. The people who wanted AIO
> wanted the brain-damaged POSIX kind of AIO, not something cool and
> clever. So I suspect the whole exit-signal thing was just used for
> random small per-project things.
AIO is a whole other ball of wax. :-)
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-04-01 16:29 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jonathan Kowalski, Aleksa Sarai, Andy Lutomirski,
Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAKOZuetovU9NawSjUfGTAwXVfnmKC9GFtmS8FxAE5Hjmzq0HHA@mail.gmail.com>
On Mon, Apr 1, 2019 at 9:22 AM Daniel Colascione <dancol@google.com> wrote:
>
> There's a subtlety: suppose I'm a library and I want to create a
> private subprocess. I use the new clone facility, whatever it is, and
> get a pidfd back. I need to be able to read the child's exit status
> even if the child exits before clone returns in the parent. Doesn't
> this requirement imply that the pidfd, kernel-side, contain something
> a bit more than a struct pid?
Note that clone() has always supported this, but it has basically
never been used.
One of the early thinkings behind clone() was that it could be used
for basically "AIO in user space", by having exactly these kinds of
library-private internal subthreads that are hidden as real threads.
It's why we have that special CSIGNAL mask for setting the exit
signal. It doesn't *just* set the signal to be sent when the thread
exits - setting the signal to something else than SIGCHLD actually
changes behavior in other ways too.
In particular a non-SIGCHLD thread won't be found by a regular
"waitpid()". You have to explicitly wait for it with __WCLONE (which
in turn is supposed to be used with the explicit pid to be waited
for).
Now, none of this was ever really used. The people who wanted AIO
wanted the brain-damaged POSIX kind of AIO, not something cool and
clever. So I suspect the whole exit-signal thing was just used for
random small per-project things.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-04-01 16:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: Daniel Colascione, Aleksa Sarai, Andy Lutomirski,
Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAHk-=wi1UEsMv5wmBZB5S+5YiCbxd-AtVsoz1TfuD-KrdcLQew@mail.gmail.com>
On Mon, Apr 1, 2019 at 5:15 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Apr 1, 2019 at 9:07 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
> >
> > With the POLLHUP model on a simple pidfd, you'd know when the process
> > you were referring to is dead (and one can map POLLPRI to dead and
> > POLLHUP to zombie, etc).
>
> Adding ->poll() to the pidfd should be easy. Again, it would be
> trivially be made to work for the directory fd you get from
> /proc/<pid> too.
>
> Yeah, yeah, pollable directories are odd, but the vfs layer doesn't
> care about things like "is this a directory or not". It will just call
> the f_op->poll() method.
I know, Andy even sent a patch for that long back. The question is,
this sure solves the immediate usecase, but it inhibits some very
powerful (and natural) things from being realised in the future, and
makes some choices harder.
Currently, pidfd_send_signal doesn't work across PID namespaces. It
would be possible to make it work, but some things need to be taken
care of, precisely, that one allows a task to open pidfds for tasks
*it* can see. Why? because you essentially isolate the PID namespace,
so your open() for this namespace suddenly doesn't start opening
things it cannot see through some other namespace (i.e. /proc),
precisely how you cannot open sockets in network namespaces from the
outside, though if you can setns, you should be able to (same with
pidfds). This makes for a nice delegation model, I can essentially put
a task in a namespace with no other tasks, keep pushing pidfds into
the damn thing, and subject to kernel permissions and capabilities it
can yield in the owner userns, signal the said task. You can extend
this to ptrace and other things, by making them accept a pidfd. This
means userspace has to explicitly pass such descriptors around to make
this work, like it does today (and how I can use an open socket and
accept connections whilst living in totally isolated network
namespace).
Besides that, /proc comes with too much stuff, it should be possible
to go from pidfd to /proc/<PID> and do whatever you wish to, but
atleast two things that require varying levels of capabilities of
inspection, the latter of which can be isolated by mount namespaces
even if the process would usially be allowed to peek into it and read
the entire thing, do not end being munged together. I can choose to
pass both, but if /proc dir fds *are* pidfds, you need the entire
complexity of masking and whatnot (which would be usable on its own,
no doubt), making directory descriptors pollable and readable, etc
etc.
>
> Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-04-01 16:21 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Linus Torvalds, Aleksa Sarai, Andy Lutomirski, Christian Brauner,
Jann Horn, Andrew Lutomirski, David Howells, Serge E. Hallyn,
Linux API, Linux List Kernel Mailing, Arnd Bergmann,
Eric W. Biederman, Konstantin Khlebnikov, Kees Cook,
Alexey Dobriyan, Thomas Gleixner, Michael Kerrisk-manpages,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAGLj2rF85hZLAR79oEA1dQARadw1OHr8bxqCLbvO6g9B6ou2Qw@mail.gmail.com>
On Mon, Apr 1, 2019 at 9:07 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Mon, Apr 1, 2019 at 4:55 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Mon, Apr 1, 2019 at 8:36 AM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >
> > > > Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
> > > > an attempt to make it possible one day to mount /proc inside a container
> > > > without adding a bunch of masked paths), though it was just an idea and
> > > > I don't know if he ever had a patch for it.
> >
> > Couldn't this mode just be a relatively simple procfs mount option
> > instead of a whole new filesystem? It'd be a bit like hidepid, right?
> > The internal bind mount option and the no-dotdot-traversal options
> > also look good to me.
> >
> > > I wonder if we really want a fill procfs2, or maybe we could just make
> > > the pidfd readable (yes, it's a directory file descriptor, but we
> > > could allow reading).
> >
> > What would read(2) read?
> >
> > > What are the *actual* use cases for opening /proc files through it? If
> > > it's really just for a small subset that android wants to do this
> > > (getting basic process state like "running" etc), rather than anything
> > > else, then we could skip the whole /proc linking entirely and go the
> > > other way instead (ie open_pidfd() would get that limited IO model,
> > > and we could make the /proc directory node get the same limited IO
> > > model).
> >
> > We do a lot of process state inspection and manipulation, including
> > reading and writing the oom killer adjustment score, reading smaps,
> > and the occasional cgroup manipulation. More generally, I'd also like
> > to be able to write a race-free pkill(1). Doing this work via pidfd
> > would be convenient. More generally, we can't enumerate the specific
> > use cases, because what we want to do with processes isn't bounded in
> > advance, and we regularly find new things in /proc/pid that we want to
> > read and write. I'd rather not prematurely limit the applicability of
> > the pidfd interface, especially when there's a simple option (the
> > procfs directory file descriptor approach) that doesn't require
> > in-advance enumeration of supported process inspection and
> > manipulation actions or a separate per-option pidfd equivalent. I very
> > much want a general-purpose API that reuses the metadata interfaces
> > the kernel already exposes. It's not clear to me how this rich
> > interface could be matched by read(2) on a pidfd.
>
> With the POLLHUP model on a simple pidfd, you'd know when the process
> you were referring to is dead (and one can map POLLPRI to dead and
> POLLHUP to zombie, etc).
I agree that there needs to be *some* kind of pollable file descriptor
that fires on process death. Should it be the pifd itself? I've been
thinking that a pollable directory would be too weird, but if that's
not actually a problem, I'm fine with it.
Mapping different state transitions to different poll bits is an
interesting idea, but I'm also worried about making poll the "source
of truth" with respect to process state as reported by a pidfd.
Consider a socket: for a socket, read(2)/recv(2) is the "source of
truth" and poll is just a hint that says "now is a good time to
attempt a read". Some other systems even allow for spurious wakeups
from poll. It's also worth keeping in mind that some pre-existing
event loops let you provide "is readable" and "is writable" callbacks,
but don't support the more exotic poll notification bits. That's why
I've tried to keep my proposals limited to poll signaling readability.
But I'm not really that picky. I just want something that works.
> This is just an extension of the child process model, since you'd know
> when it's dead, there's no race involved with opening the wrong
> /proc/<PID> entry. The entire thing is already non-racy for direct
> children, the same model can be extended to non-direct ones.
>
> This simplifies a lot of things, now I am essentially just passing a
> file descriptor pinning the struct pid associated with the original
> task, and not process state around to others (I may even want the
> other process to not read that stuff out even if it was allowed to, as
> it wouldn't have been able to otherwise, due to being a in a different
> mount namespace). KISS.
>
> The upshot is this same descriptor can be returned from clone, which
> would allow you to directly register it in your event loop (like
> signalfd, timerfd, file fd, sockets, etc) and POLLIN generated for you
> to read back its exit status (it is arguable if non-parents should be
> returned a readable instance from pidfd_open, but parents sure
> should). You can disable SIGCHLD for the child, and read back exit
> status much later. The entire point of waiting and reaping was that
> it'd be lost, but now you have a descriptor where it is kept for you
> to consume.
There's a subtlety: suppose I'm a library and I want to create a
private subprocess. I use the new clone facility, whatever it is, and
get a pidfd back. I need to be able to read the child's exit status
even if the child exits before clone returns in the parent. Doesn't
this requirement imply that the pidfd, kernel-side, contain something
a bit more than a struct pid?
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-04-01 16:15 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Daniel Colascione, Aleksa Sarai, Andy Lutomirski,
Christian Brauner, Jann Horn, Andrew Lutomirski, David Howells,
Serge E. Hallyn, Linux API, Linux List Kernel Mailing,
Arnd Bergmann, Eric W. Biederman, Konstantin Khlebnikov,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAGLj2rF85hZLAR79oEA1dQARadw1OHr8bxqCLbvO6g9B6ou2Qw@mail.gmail.com>
On Mon, Apr 1, 2019 at 9:07 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> With the POLLHUP model on a simple pidfd, you'd know when the process
> you were referring to is dead (and one can map POLLPRI to dead and
> POLLHUP to zombie, etc).
Adding ->poll() to the pidfd should be easy. Again, it would be
trivially be made to work for the directory fd you get from
/proc/<pid> too.
Yeah, yeah, pollable directories are odd, but the vfs layer doesn't
care about things like "is this a directory or not". It will just call
the f_op->poll() method.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-04-01 16:13 UTC (permalink / raw)
To: Linus Torvalds
Cc: Aleksa Sarai, Andy Lutomirski, Christian Brauner, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAHk-=wgKOJTdN9TqyjW6FngEZvgUTmCDzJPRrtekW_1SD0gfhw@mail.gmail.com>
On Mon, Apr 1, 2019 at 9:01 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Apr 1, 2019 at 8:55 AM Daniel Colascione <dancol@google.com> wrote:
> >
> >
> > > I wonder if we really want a fill procfs2, or maybe we could just make
> > > the pidfd readable (yes, it's a directory file descriptor, but we
> > > could allow reading).
> >
> > What would read(2) read?
>
> We could make it read anything, but it would have to be something
> people agree is sufficient (and not so expensive to create that rare
> users of that data would find the overhead excessive).
In my exithand patch last year, I took a minimal approach and just had
read(2) read EOF once the process exited and blocked until then. Maybe
we could do that? But if we're supported waitid(2) on pidfds instead,
then we don't need read(2) at all on pidfds, right? Right now, I don't
see a situation in which I'd take advantage of a pidfd read(2) that
just gave me the running/sleeping/zombie state.
I'm also a bit worried about the implications of making a directory FD
also readable. Didn't problems with that come up in one of the
multiple-named-streams proposals?
> Eg we could make it return the same thing that /proc/<pid>/status
> reads right now.
>
> But it sounds like you need pretty much all of /proc/<pid>/xyz:
>
> > We do a lot of process state inspection and manipulation, including
> > reading and writing the oom killer adjustment score, reading smaps,
> > and the occasional cgroup manipulation. More generally, I'd also like
> > to be able to write a race-free pkill(1)
>
> I suspect most of what pkill wants is indeed in that 'status' file,
> but other things aren't.
Right. It's hard to predict what we might need. pkill also needs
/proc/pid/cmdline, FWIW.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Andy Lutomirski @ 2019-04-01 16:10 UTC (permalink / raw)
To: Linus Torvalds
Cc: Aleksa Sarai, Christian Brauner, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAHk-=wgKqBQznZdTQaM6yQ+_5dcz-+q8=2sbQsAoDh55hQTLMA@mail.gmail.com>
> On Apr 1, 2019, at 9:36 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>>
>> Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
>> an attempt to make it possible one day to mount /proc inside a container
>> without adding a bunch of masked paths), though it was just an idea and
>> I don't know if he ever had a patch for it.
>
> I wonder if we really want a fill procfs2, or maybe we could just make
> the pidfd readable (yes, it's a directory file descriptor, but we
> could allow reading).
There were patches to make procfs mount options work sensibly, and I think they got merged. So we could probably avoid a whole procfs2 by instead having it be procfs plus a special (maybe purely internal) mount option to restrict it.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-04-01 16:07 UTC (permalink / raw)
To: Daniel Colascione
Cc: Linus Torvalds, Aleksa Sarai, Andy Lutomirski, Christian Brauner,
Jann Horn, Andrew Lutomirski, David Howells, Serge E. Hallyn,
Linux API, Linux List Kernel Mailing, Arnd Bergmann,
Eric W. Biederman, Konstantin Khlebnikov, Kees Cook,
Alexey Dobriyan, Thomas Gleixner, Michael Kerrisk-manpages,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAKOZuev4Q4CY0-2rUpTujSKMVJ9L9Exv=_divFC0G0_OaQHaGw@mail.gmail.com>
On Mon, Apr 1, 2019 at 4:55 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Mon, Apr 1, 2019 at 8:36 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
> > > an attempt to make it possible one day to mount /proc inside a container
> > > without adding a bunch of masked paths), though it was just an idea and
> > > I don't know if he ever had a patch for it.
>
> Couldn't this mode just be a relatively simple procfs mount option
> instead of a whole new filesystem? It'd be a bit like hidepid, right?
> The internal bind mount option and the no-dotdot-traversal options
> also look good to me.
>
> > I wonder if we really want a fill procfs2, or maybe we could just make
> > the pidfd readable (yes, it's a directory file descriptor, but we
> > could allow reading).
>
> What would read(2) read?
>
> > What are the *actual* use cases for opening /proc files through it? If
> > it's really just for a small subset that android wants to do this
> > (getting basic process state like "running" etc), rather than anything
> > else, then we could skip the whole /proc linking entirely and go the
> > other way instead (ie open_pidfd() would get that limited IO model,
> > and we could make the /proc directory node get the same limited IO
> > model).
>
> We do a lot of process state inspection and manipulation, including
> reading and writing the oom killer adjustment score, reading smaps,
> and the occasional cgroup manipulation. More generally, I'd also like
> to be able to write a race-free pkill(1). Doing this work via pidfd
> would be convenient. More generally, we can't enumerate the specific
> use cases, because what we want to do with processes isn't bounded in
> advance, and we regularly find new things in /proc/pid that we want to
> read and write. I'd rather not prematurely limit the applicability of
> the pidfd interface, especially when there's a simple option (the
> procfs directory file descriptor approach) that doesn't require
> in-advance enumeration of supported process inspection and
> manipulation actions or a separate per-option pidfd equivalent. I very
> much want a general-purpose API that reuses the metadata interfaces
> the kernel already exposes. It's not clear to me how this rich
> interface could be matched by read(2) on a pidfd.
With the POLLHUP model on a simple pidfd, you'd know when the process
you were referring to is dead (and one can map POLLPRI to dead and
POLLHUP to zombie, etc).
This is just an extension of the child process model, since you'd know
when it's dead, there's no race involved with opening the wrong
/proc/<PID> entry. The entire thing is already non-racy for direct
children, the same model can be extended to non-direct ones.
This simplifies a lot of things, now I am essentially just passing a
file descriptor pinning the struct pid associated with the original
task, and not process state around to others (I may even want the
other process to not read that stuff out even if it was allowed to, as
it wouldn't have been able to otherwise, due to being a in a different
mount namespace). KISS.
The upshot is this same descriptor can be returned from clone, which
would allow you to directly register it in your event loop (like
signalfd, timerfd, file fd, sockets, etc) and POLLIN generated for you
to read back its exit status (it is arguable if non-parents should be
returned a readable instance from pidfd_open, but parents sure
should). You can disable SIGCHLD for the child, and read back exit
status much later. The entire point of waiting and reaping was that
it'd be lost, but now you have a descriptor where it is kept for you
to consume.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-04-01 16:01 UTC (permalink / raw)
To: Daniel Colascione
Cc: Aleksa Sarai, Andy Lutomirski, Christian Brauner, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAKOZuev4Q4CY0-2rUpTujSKMVJ9L9Exv=_divFC0G0_OaQHaGw@mail.gmail.com>
On Mon, Apr 1, 2019 at 8:55 AM Daniel Colascione <dancol@google.com> wrote:
>
>
> > I wonder if we really want a fill procfs2, or maybe we could just make
> > the pidfd readable (yes, it's a directory file descriptor, but we
> > could allow reading).
>
> What would read(2) read?
We could make it read anything, but it would have to be something
people agree is sufficient (and not so expensive to create that rare
users of that data would find the overhead excessive).
Eg we could make it return the same thing that /proc/<pid>/status
reads right now.
But it sounds like you need pretty much all of /proc/<pid>/xyz:
> We do a lot of process state inspection and manipulation, including
> reading and writing the oom killer adjustment score, reading smaps,
> and the occasional cgroup manipulation. More generally, I'd also like
> to be able to write a race-free pkill(1)
I suspect most of what pkill wants is indeed in that 'status' file,
but other things aren't.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Daniel Colascione @ 2019-04-01 15:55 UTC (permalink / raw)
To: Linus Torvalds
Cc: Aleksa Sarai, Andy Lutomirski, Christian Brauner, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAHk-=wgKqBQznZdTQaM6yQ+_5dcz-+q8=2sbQsAoDh55hQTLMA@mail.gmail.com>
On Mon, Apr 1, 2019 at 8:36 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
> > an attempt to make it possible one day to mount /proc inside a container
> > without adding a bunch of masked paths), though it was just an idea and
> > I don't know if he ever had a patch for it.
Couldn't this mode just be a relatively simple procfs mount option
instead of a whole new filesystem? It'd be a bit like hidepid, right?
The internal bind mount option and the no-dotdot-traversal options
also look good to me.
> I wonder if we really want a fill procfs2, or maybe we could just make
> the pidfd readable (yes, it's a directory file descriptor, but we
> could allow reading).
What would read(2) read?
> What are the *actual* use cases for opening /proc files through it? If
> it's really just for a small subset that android wants to do this
> (getting basic process state like "running" etc), rather than anything
> else, then we could skip the whole /proc linking entirely and go the
> other way instead (ie open_pidfd() would get that limited IO model,
> and we could make the /proc directory node get the same limited IO
> model).
We do a lot of process state inspection and manipulation, including
reading and writing the oom killer adjustment score, reading smaps,
and the occasional cgroup manipulation. More generally, I'd also like
to be able to write a race-free pkill(1). Doing this work via pidfd
would be convenient. More generally, we can't enumerate the specific
use cases, because what we want to do with processes isn't bounded in
advance, and we regularly find new things in /proc/pid that we want to
read and write. I'd rather not prematurely limit the applicability of
the pidfd interface, especially when there's a simple option (the
procfs directory file descriptor approach) that doesn't require
in-advance enumeration of supported process inspection and
manipulation actions or a separate per-option pidfd equivalent. I very
much want a general-purpose API that reuses the metadata interfaces
the kernel already exposes. It's not clear to me how this rich
interface could be matched by read(2) on a pidfd.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Christian Brauner @ 2019-04-01 15:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: Aleksa Sarai, Andy Lutomirski, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <CAHk-=wgKqBQznZdTQaM6yQ+_5dcz-+q8=2sbQsAoDh55hQTLMA@mail.gmail.com>
On Mon, Apr 01, 2019 at 08:36:26AM -0700, Linus Torvalds wrote:
> On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
> > an attempt to make it possible one day to mount /proc inside a container
> > without adding a bunch of masked paths), though it was just an idea and
> > I don't know if he ever had a patch for it.
>
> I wonder if we really want a fill procfs2, or maybe we could just make
No, I don't think we want a full procfs2.
> the pidfd readable (yes, it's a directory file descriptor, but we
> could allow reading).
Hm, if I understand this correctly, then the pidfd we return from
pidfd_open() would still be a dirfd but not tied to procfs? So I would
implement a "dummy" procfs anon_procfs that is a kernel internal mount
from which we allocate inodes, stash struct pid and off to userspace we
go?
>
> What are the *actual* use cases for opening /proc files through it? If
> it's really just for a small subset that android wants to do this
> (getting basic process state like "running" etc), rather than anything
> else, then we could skip the whole /proc linking entirely and go the
> other way instead (ie open_pidfd() would get that limited IO model,
> and we could make the /proc directory node get the same limited IO
> model).
>From the original thread where metadata access was apparently very
important things that were listed:
<quote>
And how do you propose, given one of these handle objects, getting a
process's current priority, or its current oom score, or its list of
memory maps? As I mentioned in my original email, and which nobody has
addressed, if you don't use a dirfd as your process handle or you
don't provide an easy way to get one of these proc directory FDs, you
need to duplicate a lot of metadata access interfaces.
<quote>
( https://lore.kernel.org/lkml/CALCETrUFrFKC2YTLH7ViM_7XPYk3LNmNiaz6s8wtWo1pmJQXzg@mail.gmail.com/ )
Joel can probably speak best to this.
^ permalink raw reply
* Re: [PATCH v2 0/5] pid: add pidfd_open()
From: Linus Torvalds @ 2019-04-01 15:36 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Andy Lutomirski, Christian Brauner, Daniel Colascione, Jann Horn,
Andrew Lutomirski, David Howells, Serge E. Hallyn, Linux API,
Linux List Kernel Mailing, Arnd Bergmann, Eric W. Biederman,
Konstantin Khlebnikov, Kees Cook, Alexey Dobriyan,
Thomas Gleixner, Michael Kerrisk-manpages, Jonathan Kowalski,
Dmitry V. Levin, Andrew Morton
In-Reply-To: <20190401114059.7gdsvcqyoz2o5bbz@yavin>
On Mon, Apr 1, 2019 at 4:41 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Eric pitched a procfs2 which would *just* be the PIDs some time ago (in
> an attempt to make it possible one day to mount /proc inside a container
> without adding a bunch of masked paths), though it was just an idea and
> I don't know if he ever had a patch for it.
I wonder if we really want a fill procfs2, or maybe we could just make
the pidfd readable (yes, it's a directory file descriptor, but we
could allow reading).
What are the *actual* use cases for opening /proc files through it? If
it's really just for a small subset that android wants to do this
(getting basic process state like "running" etc), rather than anything
else, then we could skip the whole /proc linking entirely and go the
other way instead (ie open_pidfd() would get that limited IO model,
and we could make the /proc directory node get the same limited IO
model).
Linus
^ permalink raw reply
* Re: [PATCH ghak90 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Paul Moore @ 2019-04-01 14:50 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <56127b2a5b82f15cb0d0f040502c2e3bb6945f30.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 2:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> event standalone records. Iterate through all potential audit container
> identifiers associated with a network namespace.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 5 +++++
> kernel/audit.c | 41 +++++++++++++++++++++++++++++++++++++++++
> net/netfilter/nft_log.c | 11 +++++++++--
> net/netfilter/xt_AUDIT.c | 11 +++++++++--
> 4 files changed, 64 insertions(+), 4 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7fa3194f5342..80ed323feeb5 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -451,6 +451,47 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> audit_netns_contid_add(new->net_ns, contid);
> }
>
> +/**
> + * audit_log_netns_contid_list - List contids for the given network namespace
> + * @net: the network namespace of interest
> + * @context: the audit context to use
> + *
> + * Description:
> + * Issues a CONTAINER_ID record with a CSV list of contids associated
> + * with a network namespace to accompany a NETFILTER_PKT record.
> + */
> +void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
> +{
> + struct audit_buffer *ab = NULL;
> + struct audit_contid *cont;
> + bool first = true;
> + struct audit_net *aunet;
> +
> + /* Generate AUDIT_CONTAINER_ID record with container ID CSV list */
> + rcu_read_lock();
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + goto out;
> + list_for_each_entry_rcu(cont, &aunet->contid_list, list) {
> + if (first) {
This is borderline nit-picky, but it seems like we could get rid of
"first" and just check to see if "ab" is still NULL.
> + ab = audit_log_start(context, GFP_ATOMIC,
> + AUDIT_CONTAINER_ID);
> + if (!ab) {
> + audit_log_lost("out of memory in audit_log_netns_contid_list");
> + goto out;
> + }
> + audit_log_format(ab, "contid=");
> + } else
> + audit_log_format(ab, ",");
> + audit_log_format(ab, "%llu", cont->id);
> + first = false;
> + }
> + audit_log_end(ab);
> +out:
> + rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(audit_log_netns_contid_list);
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Paul Moore @ 2019-04-01 14:50 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman
In-Reply-To: <27473c84a274c64871cfa8e3636deaf05603c978.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 2: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/kernel/audit.c b/kernel/audit.c
> index cf448599ef34..7fa3194f5342 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -72,6 +72,7 @@
> #include <linux/freezer.h>
> #include <linux/pid_namespace.h>
> #include <net/netns/generic.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -99,9 +100,13 @@
> /**
> * struct audit_net - audit private network namespace data
> * @sk: communication socket
> + * @contid_list: audit container identifier list
> + * @contid_list_lock audit container identifier list lock
> */
> struct audit_net {
> struct sock *sk;
> + struct list_head contid_list;
> + spinlock_t contid_list_lock;
> };
>
> /**
> @@ -275,8 +280,11 @@ struct audit_task_info init_struct_audit = {
> void audit_free(struct task_struct *tsk)
> {
> struct audit_task_info *info = tsk->audit;
> + struct nsproxy *ns = tsk->nsproxy;
>
> audit_free_syscall(tsk);
> + if (ns)
> + audit_netns_contid_del(ns->net_ns, audit_get_contid(tsk));
> /* Freeing the audit_task_info struct must be performed after
> * audit_log_exit() due to need for loginuid and sessionid.
> */
> @@ -376,6 +384,73 @@ static struct sock *audit_get_sk(const struct net *net)
> return aunet->sk;
> }
>
> +void audit_netns_contid_add(struct net *net, u64 contid)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> + struct list_head *contid_list = &aunet->contid_list;
> + struct audit_contid *cont;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + if (!aunet)
> + return;
We should move the contid_list assignment below this check, or decide
that aunet is always going to valid (?) and get rid of this check
completely.
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
We don't need the list_empty() check here do we? I think we can just
call list_for_each_entry_rcu(), yes?
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + refcount_inc(&cont->refcount);
> + goto out;
> + }
> + cont = kmalloc(sizeof(struct audit_contid), GFP_ATOMIC);
If you had to guess, what do you think is going to be more common:
bumping the refcount of an existing entry in the list, or adding a new
entry? I'm asking because I always get a little nervous when doing
allocations while holding a spinlock. Yes, you are doing it with
GFP_ATOMIC, but it still seems like something to try and avoid if this
is going to approach 50%. However, if the new entry is rare then the
extra work of always doing the allocation before taking the lock and
then freeing it afterwards might be a bad tradeoff.
My gut feeling says we might do about as many allocations as refcount
bumps, but I could be thinking about this wrong.
Moving the allocation outside the spinlock might also open the door to
doing this as GFP_KERNEL, which is a good thing, but I haven't looked
at the callers to see if that is possible (it may not be). That's an
exercise left to the patch author (if he hasn't done that already).
> + if (cont) {
> + INIT_LIST_HEAD(&cont->list);
Unless there is some guidance that INIT_LIST_HEAD() should be used
regardless, you shouldn't need to call this here since list_add_rcu()
will take care of any list.h related initialization.
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add_rcu(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(&aunet->contid_list_lock);
> +}
--
paul moore
www.paul-moore.com
^ 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