* Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
From: Ondrej Mosnacek @ 2019-03-27 21:41 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, Simo Sorce, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <20190318234716.4moownrpylloiu3g@madcap2.tricolour.ca>
On Tue, Mar 19, 2019 at 12:47 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-03-18 21:02, Ondrej Mosnacek wrote:
> > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > Implement audit container identifier filtering using the AUDIT_CONTID
> > > field name to send an 8-character string representing a u64 since the
> > > value field is only u32.
> > >
> > > Sending it as two u32 was considered, but gathering and comparing two
> > > fields was more complex.
> > >
> > > The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/91
> > > See: https://github.com/linux-audit/audit-userspace/issues/40
> > > 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>
> > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > ---
> > > include/linux/audit.h | 1 +
> > > include/uapi/linux/audit.h | 5 ++++-
> > > kernel/audit.h | 1 +
> > > kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> > > kernel/auditsc.c | 3 +++
> > > 5 files changed, 56 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 6db5aba7cc01..fa19fa408931 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -77,6 +77,7 @@ struct audit_field {
> > > u32 type;
> > > union {
> > > u32 val;
> > > + u64 val64;
> > > kuid_t uid;
> > > kgid_t gid;
> > > struct {
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index a6383e28b2c8..741ab6f38294 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -265,6 +265,7 @@
> > > #define AUDIT_LOGINUID_SET 24
> > > #define AUDIT_SESSIONID 25 /* Session ID */
> > > #define AUDIT_FSTYPE 26 /* FileSystem Type */
> > > +#define AUDIT_CONTID 27 /* Container ID */
> > >
> > > /* These are ONLY useful when checking
> > > * at syscall exit time (AUDIT_AT_EXIT). */
> > > @@ -345,6 +346,7 @@ enum {
> > > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> > > #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> > > +#define AUDIT_FEATURE_BITMAP_CONTAINERID 0x00000080
> > >
> > > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > > @@ -352,7 +354,8 @@ enum {
> > > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> > > AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > > - AUDIT_FEATURE_BITMAP_FILTER_FS)
> > > + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> > > + AUDIT_FEATURE_BITMAP_CONTAINERID)
> > >
> > > /* deprecated: AUDIT_VERSION_* */
> > > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index 2a1a8b8a8019..3a40b608bf8d 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -230,6 +230,7 @@ static inline int audit_hash_ino(u32 ino)
> > >
> > > extern int audit_match_class(int class, unsigned syscall);
> > > extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> > > +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> > > extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> > > extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> > > extern int parent_len(const char *path);
> > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > index add360b46b38..516b8e58959e 100644
> > > --- a/kernel/auditfilter.c
> > > +++ b/kernel/auditfilter.c
> > > @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> > > /* FALL THROUGH */
> > > case AUDIT_ARCH:
> > > case AUDIT_FSTYPE:
> > > + case AUDIT_CONTID:
> > > if (f->op != Audit_not_equal && f->op != Audit_equal)
> > > return -EINVAL;
> > > break;
> > > @@ -582,6 +583,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> > > }
> > > entry->rule.exe = audit_mark;
> > > break;
> > > + case AUDIT_CONTID:
> > > + if (f->val != sizeof(u64))
> > > + goto exit_free;
> > > + str = audit_unpack_string(&bufp, &remain, f->val);
> > > + if (IS_ERR(str))
> > > + goto exit_free;
> > > + f->val64 = ((u64 *)str)[0];
> > > + break;
> > > }
> > > }
> > >
> > > @@ -664,6 +673,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> > > data->buflen += data->values[i] =
> > > audit_pack_string(&bufp, audit_mark_path(krule->exe));
> > > break;
> > > + case AUDIT_CONTID:
> > > + data->buflen += data->values[i] = sizeof(u64);
> > > + for (i = 0; i < sizeof(u64); i++)
> > > + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> >
> > How about just:
> >
> > memcpy(bufp, &f->val64, sizeof(u64));
> >
> > instead of the awkward for loop? It is simpler and also more in line
> > with the code in audit_pack_string().
>
> I'll grant you that. I'll use that instead.
Assuming the above tweak will be done, you can add:
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> > Also, doesn't this loop interfere with the outer loop that also uses
> > 'i' as the control variable?
>
> Yes, it does. Thank you for catching that. Doing a auditctl -l reveals
> this.
>
> > > + break;
> > > case AUDIT_LOGINUID_SET:
> > > if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> > > data->fields[i] = AUDIT_LOGINUID;
> > > @@ -750,6 +764,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> > > if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> > > return 1;
> > > break;
> > > + case AUDIT_CONTID:
> > > + if (a->fields[i].val64 != b->fields[i].val64)
> > > + return 1;
> > > + break;
> > > default:
> > > if (a->fields[i].val != b->fields[i].val)
> > > return 1;
> > > @@ -1206,6 +1224,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> > > }
> > > }
> > >
> > > +int audit_comparator64(u64 left, u32 op, u64 right)
> > > +{
> > > + switch (op) {
> > > + case Audit_equal:
> > > + return (left == right);
> > > + case Audit_not_equal:
> > > + return (left != right);
> > > + case Audit_lt:
> > > + return (left < right);
> > > + case Audit_le:
> > > + return (left <= right);
> > > + case Audit_gt:
> > > + return (left > right);
> > > + case Audit_ge:
> > > + return (left >= right);
> > > + case Audit_bitmask:
> > > + return (left & right);
> > > + case Audit_bittest:
> > > + return ((left & right) == right);
> > > + default:
> > > + BUG();
> > > + return 0;
> > > + }
> > > +}
> > > +
> > > int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> > > {
> > > switch (op) {
> > > @@ -1344,6 +1387,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> > > result = audit_comparator(audit_loginuid_set(current),
> > > f->op, f->val);
> > > break;
> > > + case AUDIT_CONTID:
> > > + result = audit_comparator64(audit_get_contid(current),
> > > + f->op, f->val64);
> > > + break;
> > > case AUDIT_MSGTYPE:
> > > result = audit_comparator(msgtype, f->op, f->val);
> > > break;
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index aa5d13b4fbbb..2d74238e9638 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -616,6 +616,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > > case AUDIT_LOGINUID_SET:
> > > result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> > > break;
> > > + case AUDIT_CONTID:
> > > + result = audit_comparator64(audit_get_contid(tsk), f->op, f->val64);
> > > + break;
> > > case AUDIT_SUBJ_USER:
> > > case AUDIT_SUBJ_ROLE:
> > > case AUDIT_SUBJ_TYPE:
> > > --
> > > 1.8.3.1
> > >
> >
> >
> > --
> > Ondrej Mosnacek <omosnace at redhat dot com>
> > Associate Software Engineer, Security Technologies
> > Red Hat, Inc.
>
> - 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
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
From: Richard Guy Briggs @ 2019-03-27 22:00 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: 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: <CAFqZXNu7d28D=2G0jjAmrYAyKzYgkeGGLwRB1ekvteUbA9G_tw@mail.gmail.com>
On 2019-03-27 22:41, Ondrej Mosnacek wrote:
> On Tue, Mar 19, 2019 at 12:47 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-03-18 21:02, Ondrej Mosnacek wrote:
> > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > Implement audit container identifier filtering using the AUDIT_CONTID
> > > > field name to send an 8-character string representing a u64 since the
> > > > value field is only u32.
> > > >
> > > > Sending it as two u32 was considered, but gathering and comparing two
> > > > fields was more complex.
> > > >
> > > > The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/91
> > > > See: https://github.com/linux-audit/audit-userspace/issues/40
> > > > 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>
> > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > ---
> > > > include/linux/audit.h | 1 +
> > > > include/uapi/linux/audit.h | 5 ++++-
> > > > kernel/audit.h | 1 +
> > > > kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > kernel/auditsc.c | 3 +++
> > > > 5 files changed, 56 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 6db5aba7cc01..fa19fa408931 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -77,6 +77,7 @@ struct audit_field {
> > > > u32 type;
> > > > union {
> > > > u32 val;
> > > > + u64 val64;
> > > > kuid_t uid;
> > > > kgid_t gid;
> > > > struct {
> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index a6383e28b2c8..741ab6f38294 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -265,6 +265,7 @@
> > > > #define AUDIT_LOGINUID_SET 24
> > > > #define AUDIT_SESSIONID 25 /* Session ID */
> > > > #define AUDIT_FSTYPE 26 /* FileSystem Type */
> > > > +#define AUDIT_CONTID 27 /* Container ID */
> > > >
> > > > /* These are ONLY useful when checking
> > > > * at syscall exit time (AUDIT_AT_EXIT). */
> > > > @@ -345,6 +346,7 @@ enum {
> > > > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > > > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> > > > #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> > > > +#define AUDIT_FEATURE_BITMAP_CONTAINERID 0x00000080
> > > >
> > > > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > > > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > > > @@ -352,7 +354,8 @@ enum {
> > > > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > > > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> > > > AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > > > - AUDIT_FEATURE_BITMAP_FILTER_FS)
> > > > + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> > > > + AUDIT_FEATURE_BITMAP_CONTAINERID)
> > > >
> > > > /* deprecated: AUDIT_VERSION_* */
> > > > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > index 2a1a8b8a8019..3a40b608bf8d 100644
> > > > --- a/kernel/audit.h
> > > > +++ b/kernel/audit.h
> > > > @@ -230,6 +230,7 @@ static inline int audit_hash_ino(u32 ino)
> > > >
> > > > extern int audit_match_class(int class, unsigned syscall);
> > > > extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> > > > +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> > > > extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> > > > extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> > > > extern int parent_len(const char *path);
> > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > > > index add360b46b38..516b8e58959e 100644
> > > > --- a/kernel/auditfilter.c
> > > > +++ b/kernel/auditfilter.c
> > > > @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> > > > /* FALL THROUGH */
> > > > case AUDIT_ARCH:
> > > > case AUDIT_FSTYPE:
> > > > + case AUDIT_CONTID:
> > > > if (f->op != Audit_not_equal && f->op != Audit_equal)
> > > > return -EINVAL;
> > > > break;
> > > > @@ -582,6 +583,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> > > > }
> > > > entry->rule.exe = audit_mark;
> > > > break;
> > > > + case AUDIT_CONTID:
> > > > + if (f->val != sizeof(u64))
> > > > + goto exit_free;
> > > > + str = audit_unpack_string(&bufp, &remain, f->val);
> > > > + if (IS_ERR(str))
> > > > + goto exit_free;
> > > > + f->val64 = ((u64 *)str)[0];
> > > > + break;
> > > > }
> > > > }
> > > >
> > > > @@ -664,6 +673,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> > > > data->buflen += data->values[i] =
> > > > audit_pack_string(&bufp, audit_mark_path(krule->exe));
> > > > break;
> > > > + case AUDIT_CONTID:
> > > > + data->buflen += data->values[i] = sizeof(u64);
> > > > + for (i = 0; i < sizeof(u64); i++)
> > > > + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> > >
> > > How about just:
> > >
> > > memcpy(bufp, &f->val64, sizeof(u64));
> > >
> > > instead of the awkward for loop? It is simpler and also more in line
> > > with the code in audit_pack_string().
> >
> > I'll grant you that. I'll use that instead.
>
> Assuming the above tweak will be done, you can add:
>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Thanks. In the process of fixing that, I had also found a buffer
pointer increment bug which has been fixed and a new test added to check
for both.
> > > Also, doesn't this loop interfere with the outer loop that also uses
> > > 'i' as the control variable?
> >
> > Yes, it does. Thank you for catching that. Doing a auditctl -l reveals
> > this.
> >
> > > > + break;
> > > > case AUDIT_LOGINUID_SET:
> > > > if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> > > > data->fields[i] = AUDIT_LOGINUID;
> > > > @@ -750,6 +764,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> > > > if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> > > > return 1;
> > > > break;
> > > > + case AUDIT_CONTID:
> > > > + if (a->fields[i].val64 != b->fields[i].val64)
> > > > + return 1;
> > > > + break;
> > > > default:
> > > > if (a->fields[i].val != b->fields[i].val)
> > > > return 1;
> > > > @@ -1206,6 +1224,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> > > > }
> > > > }
> > > >
> > > > +int audit_comparator64(u64 left, u32 op, u64 right)
> > > > +{
> > > > + switch (op) {
> > > > + case Audit_equal:
> > > > + return (left == right);
> > > > + case Audit_not_equal:
> > > > + return (left != right);
> > > > + case Audit_lt:
> > > > + return (left < right);
> > > > + case Audit_le:
> > > > + return (left <= right);
> > > > + case Audit_gt:
> > > > + return (left > right);
> > > > + case Audit_ge:
> > > > + return (left >= right);
> > > > + case Audit_bitmask:
> > > > + return (left & right);
> > > > + case Audit_bittest:
> > > > + return ((left & right) == right);
> > > > + default:
> > > > + BUG();
> > > > + return 0;
> > > > + }
> > > > +}
> > > > +
> > > > int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> > > > {
> > > > switch (op) {
> > > > @@ -1344,6 +1387,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> > > > result = audit_comparator(audit_loginuid_set(current),
> > > > f->op, f->val);
> > > > break;
> > > > + case AUDIT_CONTID:
> > > > + result = audit_comparator64(audit_get_contid(current),
> > > > + f->op, f->val64);
> > > > + break;
> > > > case AUDIT_MSGTYPE:
> > > > result = audit_comparator(msgtype, f->op, f->val);
> > > > break;
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index aa5d13b4fbbb..2d74238e9638 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -616,6 +616,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > > > case AUDIT_LOGINUID_SET:
> > > > result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> > > > break;
> > > > + case AUDIT_CONTID:
> > > > + result = audit_comparator64(audit_get_contid(tsk), f->op, f->val64);
> > > > + break;
> > > > case AUDIT_SUBJ_USER:
> > > > case AUDIT_SUBJ_ROLE:
> > > > case AUDIT_SUBJ_TYPE:
> > >
> > > Ondrej Mosnacek <omosnace at redhat dot com>
> >
> > - RGB
> >
> > Richard Guy Briggs <rgb@redhat.com>
>
> Ondrej Mosnacek <omosnace at redhat dot 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 04/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2019-03-27 22:10 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: 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: <CAFqZXNvM+d_MKGVykLtcHkwwZhO_UOYACJQu-92tEOvkk7uMrQ@mail.gmail.com>
On 2019-03-27 22:01, Ondrej Mosnacek wrote:
> On Fri, Mar 15, 2019 at 7:34 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Create a new audit record AUDIT_CONTAINER_ID to document the audit
> > container identifier of a process if it is present.
> >
> > Called from audit_log_exit(), syscalls are covered.
> >
> > A sample raw event:
> > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0
> > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0
> > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > type=CONTAINER_ID msg=audit(1519924845.499:257): contid=123458
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/51
> > 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>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> Barring one minor nit below,
>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> > ---
> > include/linux/audit.h | 5 +++++
> > include/uapi/linux/audit.h | 1 +
> > kernel/audit.c | 21 +++++++++++++++++++++
> > kernel/auditsc.c | 2 ++
> > 4 files changed, 29 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 301337776193..43438192ca2a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -199,6 +199,8 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> > return tsk->audit->contid;
> > }
> >
> > +extern void audit_log_contid(struct audit_context *context, u64 contid);
> > +
> > extern u32 audit_enabled;
> > #else /* CONFIG_AUDIT */
> > static inline int audit_alloc(struct task_struct *task)
> > @@ -265,6 +267,9 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> > return AUDIT_CID_UNSET;
> > }
> >
> > +static inline void audit_log_contid(struct audit_context *context, u64 contid)
> > +{ }
> > +
> > #define audit_enabled AUDIT_OFF
> > #endif /* CONFIG_AUDIT */
> >
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index d475cf3b4d7f..a6383e28b2c8 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -115,6 +115,7 @@
> > #define AUDIT_REPLACE 1329 /* Replace auditd if this packet unanswerd */
> > #define AUDIT_KERN_MODULE 1330 /* Kernel Module events */
> > #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */
> > +#define AUDIT_CONTAINER_ID 1332 /* Container ID */
> >
> > #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
> > #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index b5c702abeb42..8cc0e88d7f2a 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2127,6 +2127,27 @@ void audit_log_session_info(struct audit_buffer *ab)
> > audit_log_format(ab, "auid=%u ses=%u", auid, sessionid);
> > }
> >
> > +/*
> > + * audit_log_contid - report container info
> > + * @context: task or local context for record
> > + * @contid: container ID to report
> > + */
> > +void audit_log_contid(struct audit_context *context, u64 contid)
> > +{
> > + struct audit_buffer *ab;
> > +
> > + if (!audit_contid_valid(contid))
> > + return;
> > + /* Generate AUDIT_CONTAINER_ID record with container ID */
> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_ID);
> > + if (!ab)
> > + return;
> > + audit_log_format(ab, "contid=%llu", contid);
>
> Just realized that you *might* get a compiler/static checker warning
> since u64 could technically be something else than unsigned long long
> on some arches... maybe this is not case in the kernel, but might be
> safer to cast it to unsigned long long before passing to
> audit_log_format(). Possibly there are similar occurrences in previous
> (later) patches that I didn't (won't) notice.
Ok, since it is guaranteed to be at least 64 bits, a cast won't truncate
it.
> > + audit_log_end(ab);
> > + return;
> > +}
> > +EXPORT_SYMBOL(audit_log_contid);
> > +
> > void audit_log_key(struct audit_buffer *ab, char *key)
> > {
> > audit_log_format(ab, " key=");
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8090eff7868d..a8c8b44b954d 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1548,6 +1548,8 @@ static void audit_log_exit(void)
> >
> > audit_log_proctitle();
> >
> > + audit_log_contid(context, audit_get_contid(current));
> > +
> > /* Send end of event record to help user space know we are finished */
> > ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
> > if (ab)
>
> Ondrej Mosnacek <omosnace at redhat dot 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 2/4] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-27 22:12 UTC (permalink / raw)
To: Christian Brauner
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190327213404.pv4wqtkjbufkx36u@brauner.io>
On Wed, Mar 27, 2019 at 9:34 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Wed, Mar 27, 2019 at 07:38:13PM +0000, Jonathan Kowalski wrote:
> > Christian,
> >
> > Giving this some thought, it looks fine to me, but I am not convinced
> > this should take a pid argument. I much prefer if obtaining a pidfd
>
> The signatures for pidfd_open() you're suggesting are conflicting. Even
> taking into account that you're referring to Andy's version
My version pidfd_open takes a pid, the pid namespace to search it in,
and has an optional flags value to extend it further in the future to
an immediate need. This is a simpler version of the pidctl's GET_PIDFD
command that you yourself proposed in the last patch, and you don't
need two namespace arguments, just one here (in that case you were
passing -1 in either of those anyway).
so pidctl(PIDCMD_GET_PIDFD, pid, source, -1, 0) becomes
pidfd_open(pid, source, 0);
pidctl(PIDCMD_GET_PIDFD, pid, -1, target or -1, 0) would be
pidfd_open(pid, -1, 0);
The target was just there due to it having to support
comparison/translation commands, but it did not serve much purpose
except the pid was reachable in both. I am not sure that is needed if
you just want a pidfd for pid relative to a namespace.
What I prefer is dropping the pid argument from *your* pidfd_open,
rename it to procfd/procpidfd_open to give it a clearer name, and have
the signature the same as what Andy suggested. This system call is
tied to CONFIG_PROC_FS, all it does is take a pidfd, a proc rootfd,
and return the /proc/<PID> dir fd. It is up to you if you want to
support procpidfd to pidfd conversion, I don't think there was any
usecase mentioned regarding that far, so you may just ignore it.
>
> int procfd = pidfd_open(procrootfd, pidfd, PIDFD_TO_PROCFD); //name
>
> the "not convinced this should take a pid argument" confuses me wrt to
> the proposed second signature:
>
> pidfd_open(pid_t pid, int ns, unsigned int flags);
>
> > goes in the translation stuff, that has very real usecases, and you
> > would end up duplicating the same thing over in two places.
> >
> > If the /proc/<PID> dir fd of a pidfd is relative to its procrootfd, a
> > pid's pidfd is also relative to its namespace. Currently, you change
>
> Pass a /proc/<pid> file descriptor that you have been given access to to
> pidfd_open() and retrieve a pidfd to that process.
It would be nice if it would be free of /proc entirely. The usecase
this system call is after is helping race free translation from pidfd
to procfd. pidfd_open as a separate system call as mentioned above
sounds much cleaner to me, and it works regardless of what /proc is
(compiled in, mounted, not compiled in, no access, etc).
>
> > things which would mean I now have to setns and spawn a child that
> > passes me back the pidfd for a pid in the said namespace.
>
> There are two scenarios:
> - you're in a cousin pid namespace
> - you're in an ancestor pid namespace
>
> If you're in an ancestor pid namespace you can just get a pidfd based on
> the pid of the process in your namespace. If you're in a cousin
> pid namespace it seems reasonable that you have to do a setns to get a
> pidfd. After all, you're not able to see any pids in there by default.
> If you want pidfd to a cousin pid namespace prove that you can access it
> by attaching to the owning user namespace of the pid namespace and get
> your pidfd.
Yes, but you break the part that you had working in the pidctl
patchset, I learn the pid of a process in a child pidns, and I am able
to get a pidfd for it with the pid relative to the passed in namespace
descriptor. You could even get a pidfd for an ancestor process, given
it explicitly gives you its namespace descriptor (or a cousin pidns,
but that still means userspace will pass the namespace descriptor
explicitly), but it is up to you if you want to limit its scope to
down the hierarchy strictly (and it would be nice to know now so that
you don't end up wasting more time later, and the next patchset is
hopefully the last one before merge modulo nits).
>
> >
> > I prefer Andy's version of
> >
> > int procrootfd = open("/proc", O_DIRECTORY | O_RDONLY);
> > int procfd = pidfd_open(procrootfd, pidfd, PIDFD_TO_PROCFD); //name
> > this procfd_open, maybe
> >
> > This is just a nicer race free openat.
> >
> > But, Joel and you agreed that it makes sense to split out the
> > translation stuff out of pidfds.
>
> I don't quite remember that part.
>
> I honestly think that the version proposed here covers for the most part
> what we want and provides a decent compromise by avoiding ioctl()s for
> the translation bits, allows us to not split this into multiple
I was not wanting to suggest an ioctl to burden you further, but it
occured to me that you would have to do less, as it already has
support to determine ownership and is meant for such tasks in
ioctl_ns(2), but I don't care if it's a translate_pid system call.
> syscalls, and also allows retrieving pidfds from other pid namespaces
> provided that one has gotten access to a file descriptor for
> /proc/<pid>. If really needed at some point - I doubt we will - you
> can extend pidfd_open() to take the flag CLONE_NEWPID:
>
> int pidfd = pidfd_open(pid, pid_ns_fd, -1, CLONE_NEWPID);
>
> and get pidfd for another pid namespace back.
>
> Christian
>
> >
> > My suggestion would be to extend ioctl_ns(2) then. It already provides
> > a (rather clumsy) mechanism to determine the relationship by comparing
> > st_dev and st_ino, which userspace can do itself for two namespace
> > descriptors. For translation, you can extend this namespace ioctl
> > (there is one to get a owner UID, you could add one to get a relative
> > PID).
> >
> > Then, your pidfd call will be:
> >
> > pidfd_open(pid_t pid, int ns, unsigned int flags);
> >
> > You would also be able to compile out anything procfs related (include
> > the new API to procfs dir fd translation), otherwise, the way to open
> > pidfds is in this call, and without CONFIG_PROC_FS=Y, this is as good
> > as pidfd_open(pid_t pid) (of which a better version I propose above).
> > The new API should be its own thing, and the procfs translation tuple
> > its own thing, tied to the proc fs option. pidfds need not have any
> > relation to /proc.
>
> They don't have a relation to procfs right now in this syscall. Works
> fine without procfs. That's the point.
Yes, but then you might as well put the translation thing to procfs in
its own system call, and take out pidctl's PIDCMD_GET_PIDFD in its
own, and keep the translate_pid system call as is from Eric's tree.
This gives you three clearly focused APIs:
procfd_open(int procrootfd, int pidfd, unsigned int flags);
pass in a /proc dir fd, and get back the /proc/<PID> dir fd of PID it
maps to, this a race free openat. That's it. flags could tell it to
give a thread's dir fd in the future. It only does translation (and
I'd like to know why the /proc/<PID> to pidfd translation is
necessary, is there a real use case? that will break the namespace
scoping you have in the current iteration of your patch too).
pidfd_open(pid_t pid, int nsfd, unsigned int flags);
Give me a pidfd for the pid relative to the namespace descriptor. It
is up to you if you want to strictly check that the current task has
this pid reachable from jts namespace, or rely on the fact that it
cannot acquire the namespace fd for a cousin or an ancestor without a
cooperating process that passes it in explicitly (again, would be nice
to know what others think about opening it up, i may be missing holes
in the swiss cheese that do allow you get hold of such a namespace
descriptor easily, in which case making it strictly down the hierarchy
is the most sensible).
and translate_pid
translate_pid(pid_t pid, int sourcens, int targetns, ...) as Konstantin had.
>
> >
> > For this procfd conversion system call, I would also lift any
> > namespace related restrictions if you are planning to, given the
> > constraint that I can only open a pidfd from an ancestor namespace
> > with the new pidfd_open system call, or acquire it through a
> > cooperative userspace process by fd passing otherwise, and I need the
> > /proc root fd, having both only permits me to open the said process's
> > /proc/<PID> dir fd subject to normal access restrictions. This means
> > the simplified procfd_open can be used to do metadata access without
> > even talking of PIDs at all, your process could live in its own world
> > and, given it has the authority to open the /proc directory, be able
> > to purely collect metadata based on the two file descriptors it is
> > given.
> >
> > Once you have the restriction in the same call that allows you to open
> > a pidfd for an addressable PID from the given namespace fd, you can
> > finally remove the restriction to signal across namespaces once the
> > siginfo stuff is sorted out, as that will only work when you
> > explicitly push the fd into a sandbox, the process cannot get it out
> > of thin air on its own (and you already mentioned it has nothing to do
> > with security). What I do worry about is one can use NS_GET_PARENT
> > ioctl to get the parent pidns if the owning userns is the same, and
> > just passing that gives me back a pidfd for the task. **So, you might
> > want to add the constraint that the PID is actually reachable by the
> > current task as well, apart from being reachable in the passed in
> > namespace.**
> >
> > Lastly, I also see no need of /proc/<PID> dir fd to pidfd conversion,
> > I would even recommend getting rid of that, so we only have one type
> > of pidfd, the anon inode one. What is the usecase behind that? It
> > would only be needed if you did not have a way to be able to metadata
> > access through a pidfd, which would be the case only prior to this
> > patch.
> >
> > I think this would simplify a lot of things, and ioctl_ns(2) is
> > probably already the place to do comparison operations and query
> > operations on hierarichal namespaces, just adding the relative PID bit
> > will make it gain feature parity with translate_pid.
^ permalink raw reply
* [PATCH v1 0/4] pidfd_open()
From: Christian Brauner @ 2019-03-27 22:19 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
Hey,
This is v1 of this patchset. No major changes. Just fixing nits that
Jann detected.
After the discussion over the last days, this is a fresh approach to
getting pidfds independent of the translate_pid() patchset.
pidfd_open() allows to retrieve pidfds for processes and removes the
dependency of pidfd on procfs.
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. Currently they do not
support any advanced operations. 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.
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 (and
the other way around).
IF PROCFD_TO_PIDFD is passed as a flag together with a file descriptor to a
/proc mount in a given pid namespace and a pidfd pidfd_open() will return a
file descriptor to the corresponding /proc/<pid> directory in procfs
mounts' pid namespace. pidfd_open() is very careful to verify that the pid
hasn't been recycled in between.
IF PIDFD_TO_PROCFD is passed as a flag together with a file descriptor
referencing a /proc/<pid> directory a pidfd referencing the struct pid
stashed in /proc/<pid> of the process will be returned.
The pidfd_open() syscalls in that manner resembles openat() as it uses a
flag argument to modify what type of file descriptor will be returned.
The pidfd_open() implementation together with the flags argument strikes me
as an elegant compromise between splitting this into multiple syscalls and
avoiding ioctls().
Note that this patchset also includes Al's and David's commit to make anon
inodes unconditional. The original intention is to make it possible to use
anon inodes in core vfs functions. pidctl() has the same requirement so
David suggested I sent this in alongside this patch. Both are informed of
this.
The syscall comes with appropriate basic testing.
/* Examples */
// Retrieve pidfd
int pidfd = pidfd_open(1234, -1, -1, 0);
// Retrieve /proc/<pid> handle for pidfd
int procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int procpidfd = pidfd_open(-1, procfd, pidfd, PIDFD_TO_PROCFD);
// Retrieve pidfd for /proc/<pid>
int procpidfd = open("/proc/1234", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int pidfd = pidfd_open(-1, procpidfd, -1, PROCFD_TO_PIDFD);
Thanks!
Christian
Christian Brauner (3):
pid: add pidfd_open()
signal: support pidfd_open() with pidfd_send_signal()
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 | 2 +
include/uapi/linux/wait.h | 3 +
init/Kconfig | 10 -
kernel/pid.c | 242 ++++++++++++++++++
kernel/signal.c | 14 +-
kernel/sys_ni.c | 3 -
tools/testing/selftests/pidfd/Makefile | 2 +-
.../testing/selftests/pidfd/pidfd_open_test.c | 201 +++++++++++++++
28 files changed, 464 insertions(+), 35 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_open_test.c
--
2.21.0
^ permalink raw reply
* [PATCH v1 1/4] Make anon_inodes unconditional
From: Christian Brauner @ 2019-03-27 22:19 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327221910.5897-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 v1 2/4] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-27 22:19 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327221910.5897-1-christian@brauner.io>
pidfd_open() allows to retrieve pidfds for processes and removes the
dependency of pidfd on procfs. Multiple people have expressed a desire to
do this even when pidfd_send_signal() was merged. It is even recorded in
the commit message for pidfd_send_signal() itself
(cf. commit 3eb39f47934f9d5a3027fe00d906a45fe3a15fad):
Q-06: (Andrew Morton [1])
Is there a cleaner way of obtaining the fd? Another syscall perhaps.
A-06: Userspace can already trivially retrieve file descriptors from procfs
so this is something that we will need to support anyway. Hence,
there's no immediate need to add another syscalls just to make
pidfd_send_signal() not dependent on the presence of procfs. However,
adding a syscalls to get such file descriptors is planned for a
future patchset (cf. [1]).
Alexey made a similar request (cf. [2]). 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.
/* 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. Currently they do not
support any advanced operations. 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.
/* 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 (and
the other way around).
IF PROCFD_TO_PIDFD is passed as a flag together with a file descriptor to a
/proc mount in a given pid namespace and a pidfd pidfd_open() will return a
file descriptor to the corresponding /proc/<pid> directory in procfs
mount's pid namespace. pidfd_open() is very careful to verify that the pid
hasn't been recycled in between.
IF PIDFD_TO_PROCFD is passed as a flag together with a file descriptor
referencing a /proc/<pid> directory a pidfd referencing the struct pid
stashed in /proc/<pid> will be returned.
The pidfd_open() syscall in that manner resembles openat() as it uses a
flag argument to modify what type of file descriptor will be returned.
The pidfd_open() implementation together with the flags argument strikes me
as an elegant compromise between splitting this into multiple syscalls and
avoiding ioctls().
/* Examples */
// Retrieve pidfd
int pidfd = pidfd_open(1234, -1, -1, 0);
// Retrieve /proc/<pid> handle for pidfd
int procfd = open("/proc", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int procpidfd = pidfd_open(-1, procfd, pidfd, PIDFD_TO_PROCFD);
// Retrieve pidfd for /proc/<pid>
int procpidfd = open("/proc/1234", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
int pidfd = pidfd_open(-1, procpidfd, -1, PROCFD_TO_PIDFD);
/* 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
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>
---
/* changelog */
v1:
- Jann Horn <jannh@google.com> in [changelog-1]:
- fix grammar in commit message
- ad O_RDONLY explicitly even if 0
- use fdput() on correct struct fd in pidfd_to_procfd()
- avoid passing O_CLOEXEC explicitly, just remove the flags argument
from pidfd_create_fd()
- Christian Brauner <christian@brauner.io>
- s/pidfd_create_fd()/pidfd_create_cloexec()/
- rename procfd argument to fd
- Yann Droneaud <ydroneaud@opteya.com> [changelog-2]:
- use stricter pid != -1 instead of pid >= 0
[changelog-1]: https://lore.kernel.org/lkml/CAG48ez2QgRQKYeNDpacLGCOuNKVM1g=1PK3KzzO2Uoyn2cKXaQ@mail.gmail.com
[changelog-2]: https://lore.kernel.org/lkml/9254286c02dbe883c14e38ed2af0022d36b17355.camel@opteya.com
---
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 | 2 +
include/uapi/linux/wait.h | 3 +
kernel/pid.c | 242 +++++++++++++++++++++++++
6 files changed, 251 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..d8a8ab78f1ff 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -929,6 +929,8 @@ 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, int fd, int pidfd,
+ 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..8282fc19d8f6 100644
--- a/include/uapi/linux/wait.h
+++ b/include/uapi/linux/wait.h
@@ -18,5 +18,8 @@
#define P_PID 1
#define P_PGID 2
+/* Flags for pidfd_open */
+#define PIDFD_TO_PROCFD 1 /* retrieve file descriptor to /proc/<pid> for pidfd */
+#define PROCFD_TO_PIDFD 2 /* retrieve pidfd for /proc/<pid> */
#endif /* _UAPI_LINUX_WAIT_H */
diff --git a/kernel/pid.c b/kernel/pid.c
index 20881598bdfa..22071c76d2e3 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,245 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
return idr_get_next(&ns->idr, &nr);
}
+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,
+};
+
+static int pidfd_create_cloexec(struct pid *pid)
+{
+ int fd;
+
+ fd = anon_inode_getfd("pidfd", &pidfd_fops, get_pid(pid),
+ O_RDWR | O_CLOEXEC);
+ if (fd < 0)
+ put_pid(pid);
+
+ return fd;
+}
+
+#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 int pidfd_to_procfd(int procfd, int pidfd)
+{
+ long fd;
+ pid_t ns_pid;
+ struct fd fdproc, fdpid;
+ struct file *file = NULL;
+ struct pid *pidfd_pid = NULL;
+ struct pid_namespace *proc_pid_ns = NULL;
+
+ fdproc = fdget(procfd);
+ if (!fdproc.file)
+ return -EBADF;
+
+ fdpid = fdget(pidfd);
+ if (!fdpid.file) {
+ fdput(fdproc);
+ 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(fdpid.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);
+ fdput(fdpid);
+ if (proc_pid_ns)
+ put_pid_ns(proc_pid_ns);
+ put_pid(pidfd_pid);
+ if (file)
+ filp_close(file, NULL);
+
+ return fd;
+}
+
+static int procfd_to_pidfd(int procfd)
+{
+ int fd;
+ struct fd fdproc;
+ struct pid *proc_pid;
+
+ fdproc = fdget(procfd);
+ if (!fdproc.file)
+ return -EBADF;
+
+ proc_pid = tgid_pidfd_to_pid(fdproc.file);
+ if (IS_ERR(proc_pid)) {
+ fdput(fdproc);
+ return PTR_ERR(proc_pid);
+ }
+
+ fd = pidfd_create_cloexec(proc_pid);
+ fdput(fdproc);
+ return fd;
+}
+#else
+static inline int pidfd_to_procfd(int procfd, int pidfd)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int procfd_to_pidfd(int procfd)
+{
+ return -EOPNOTSUPP;
+}
+#endif /* CONFIG_PROC_FS */
+
+/*
+ * pidfd_open - open a pidfd
+ * @pid: pid for which to retrieve a pidfd
+ * @procfd: procfd file descriptor
+ * @pidfd: pidfd file descriptor
+ * @flags: flags to pass
+ *
+ * Creates a new pidfd or translates between pidfds and procfds.
+ * If no flag is passed, pidfd_open() will return a new pidfd for @pid. If
+ * PROCFD_TO_PIDFD is in @flags then a pidfd for struct pid referenced by
+ * @procfd is created. If PIDFD_TO_PROCFD is passed then a file descriptor to
+ * the process /proc/<pid> directory relative to the procfs referenced by
+ * @procfd will be returned.
+ */
+SYSCALL_DEFINE4(pidfd_open, pid_t, pid, int, fd, int, pidfd,
+ unsigned int, flags)
+{
+ long fd = -EINVAL;
+
+ if (flags & ~(PIDFD_TO_PROCFD | PROCFD_TO_PIDFD))
+ return -EINVAL;
+
+ if (!flags) {
+ struct pid *pidfd_pid;
+
+ if (pid <= 0)
+ return -EINVAL;
+
+ if (procfd != -1 || pidfd != -1)
+ return -EINVAL;
+
+ pidfd_pid = find_get_pid(pid);
+ fd = pidfd_create_cloexec(pidfd_pid);
+ put_pid(pidfd_pid);
+ } else if (flags & PIDFD_TO_PROCFD) {
+ if (flags & ~PIDFD_TO_PROCFD)
+ return -EINVAL;
+
+ if (pid != -1)
+ return -EINVAL;
+
+ if (procfd < 0 || pidfd < 0)
+ return -EINVAL;
+
+ fd = pidfd_to_procfd(procfd, pidfd);
+ } else if (flags & PROCFD_TO_PIDFD) {
+ if (flags & ~PROCFD_TO_PIDFD)
+ return -EINVAL;
+
+ if (pid != -1 || pidfd != -1)
+ return -EINVAL;
+
+ if (procfd < 0)
+ return -EINVAL;
+
+ fd = procfd_to_pidfd(procfd);
+ }
+
+ return fd;
+}
+
void __init pid_idr_init(void)
{
/* Verify no one has done anything silly: */
--
2.21.0
^ permalink raw reply related
* [PATCH v1 3/4] signal: support pidfd_open() with pidfd_send_signal()
From: Christian Brauner @ 2019-03-27 22:19 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327221910.5897-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>
---
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 v1 4/4] tests: add pidfd_open() tests
From: Christian Brauner @ 2019-03-27 22:19 UTC (permalink / raw)
To: jannh, khlebnikov, luto, dhowells, serge, ebiederm, linux-api,
linux-kernel
Cc: arnd, keescook, adobriyan, tglx, mtk.manpages, bl0pbl33p, ldv,
akpm, oleg, nagarathnam.muthusamy, cyphar, viro, joel, dancol,
Christian Brauner
In-Reply-To: <20190327221910.5897-1-christian@brauner.io>
This adds a simple test case for pidfd_open().
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 +-
.../testing/selftests/pidfd/pidfd_open_test.c | 201 ++++++++++++++++++
2 files changed, 202 insertions(+), 1 deletion(-)
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_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
new file mode 100644
index 000000000000..07a262a9ef2c
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
@@ -0,0 +1,201 @@
+/* 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 "../kselftest.h"
+
+static inline int sys_pidfd_open(pid_t pid, int procfd, int pidfd,
+ unsigned int flags)
+{
+ return syscall(__NR_pidfd_open, pid, procfd, pidfd, 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)
+{
+ 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, "Pid:", 4))
+ 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;
+}
+
+int main(int argc, char **argv)
+{
+ int ret = 1;
+ int pidfd = -1, pidfd2 = -1, procfd = -1, procpidfd = -1, statusfd = -1;
+ pid_t pid;
+
+ pidfd = sys_pidfd_open(getpid(), -1, -1, 0);
+ if (pidfd < 0) {
+ ksft_print_msg("%s - failed to open pidfd\n", strerror(errno));
+ goto on_error;
+ }
+
+ 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 = sys_pidfd_open(-1, procfd, pidfd, PIDFD_TO_PROCFD);
+ if (procpidfd < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve /proc/<pid> from pidfd\n",
+ strerror(errno));
+ goto on_error;
+ }
+
+ pidfd2 = sys_pidfd_open(-1, procpidfd, -1, PROCFD_TO_PIDFD);
+ if (pidfd2 < 0) {
+ ksft_print_msg(
+ "%s - failed to retrieve pidfd from procpidfd\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);
+ 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;
+ }
+
+ ret = 0;
+
+on_error:
+ if (pidfd >= 0)
+ close(pidfd);
+
+ if (pidfd2 >= 0)
+ close(pidfd2);
+
+ if (procfd >= 0)
+ close(procfd);
+
+ if (procpidfd >= 0)
+ close(procpidfd);
+
+ if (statusfd >= 0)
+ close(statusfd);
+
+ return !ret ? ksft_exit_pass() : ksft_exit_fail();
+}
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-27 22:25 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <CAGLj2rHpYAOGz0h0k2pS4JwFSQkMmRSUWxa4BtO+7BUMD21vdQ@mail.gmail.com>
<snip>
> procfd_open(int procrootfd, int pidfd, unsigned int flags);
> pidfd_open(pid_t pid, int nsfd, unsigned int flags);
That honestly just feels like splitting openat into:
openat_dir()
and
opentat_file()
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Ondrej Mosnacek @ 2019-03-27 22:42 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, Simo Sorce, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <27473c84a274c64871cfa8e3636deaf05603c978.1552665316.git.rgb@redhat.com>
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...
> + struct rcu_head rcu;
> +};
> +
> extern int is_audit_feature_set(int which);
>
> extern int __init audit_register_class(int class, unsigned *list);
> @@ -202,6 +210,10 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> }
>
> extern void audit_log_contid(struct audit_context *context, u64 contid);
> +extern void audit_netns_contid_add(struct net *net, u64 contid);
> +extern void audit_netns_contid_del(struct net *net, u64 contid);
> +extern void audit_switch_task_namespaces(struct nsproxy *ns,
> + struct task_struct *p);
>
> extern u32 audit_enabled;
> #else /* CONFIG_AUDIT */
> @@ -271,6 +283,13 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
>
> static inline void audit_log_contid(struct audit_context *context, u64 contid)
> { }
> +static inline void audit_netns_contid_add(struct net *net, u64 contid)
> +{ }
> +static inline void audit_netns_contid_del(struct net *net, u64 contid)
> +{ }
> +static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> + struct task_struct *p)
> +{ }
>
> #define audit_enabled AUDIT_OFF
> #endif /* CONFIG_AUDIT */
> 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;
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
> + 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 (cont) {
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add_rcu(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(&aunet->contid_list_lock);
> +}
> +
> +void audit_netns_contid_del(struct net *net, u64 contid)
> +{
> + struct audit_net *aunet;
> + struct list_head *contid_list;
> + struct audit_contid *cont = NULL;
> +
> + if (!net)
> + return;
> + if (!audit_contid_valid(contid))
> + return;
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + return;
> + contid_list = &aunet->contid_list;
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + if (refcount_dec_and_test(&cont->refcount)) {
> + list_del_rcu(&cont->list);
> + kfree_rcu(cont, rcu);
> + }
> + break;
> + }
> + spin_unlock(&aunet->contid_list_lock);
> +}
> +
> +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{
> + u64 contid = audit_get_contid(p);
> + struct nsproxy *new = p->nsproxy;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + audit_netns_contid_del(ns->net_ns, contid);
> + if (new)
> + audit_netns_contid_add(new->net_ns, contid);
> +}
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -1619,7 +1694,6 @@ static int __net_init audit_net_init(struct net *net)
> .flags = NL_CFG_F_NONROOT_RECV,
> .groups = AUDIT_NLGRP_MAX,
> };
> -
> struct audit_net *aunet = net_generic(net, audit_net_id);
>
> aunet->sk = netlink_kernel_create(net, NETLINK_AUDIT, &cfg);
> @@ -1628,7 +1702,8 @@ static int __net_init audit_net_init(struct net *net)
> return -ENOMEM;
> }
> aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> -
> + INIT_LIST_HEAD(&aunet->contid_list);
> + spin_lock_init(&aunet->contid_list_lock);
> return 0;
> }
>
> @@ -2380,6 +2455,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> uid_t uid;
> struct tty_struct *tty;
> char comm[sizeof(current->comm)];
> + struct net *net = task->nsproxy->net_ns;
>
> task_lock(task);
> /* Can't set if audit disabled */
> @@ -2401,8 +2477,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> else if (!(thread_group_leader(task) && thread_group_empty(task)))
> rc = -EALREADY;
> read_unlock(&tasklist_lock);
> - if (!rc)
> + if (!rc) {
> + if (audit_contid_valid(oldcontid))
> + audit_netns_contid_del(net, oldcontid);
> task->audit->contid = contid;
> + audit_netns_contid_add(net, contid);
> + }
> task_unlock(task);
>
> if (!audit_enabled)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d330059a..718b1201ae70 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
> #include <linux/syscalls.h>
> #include <linux/cgroup.h>
> #include <linux/perf_event.h>
> +#include <linux/audit.h>
>
> static struct kmem_cache *nsproxy_cachep;
>
> @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 contid = audit_get_contid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + audit_netns_contid_add(new_ns->net_ns, contid);
> return 0;
> }
>
> @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + audit_switch_task_namespaces(ns, p);
Since we call audit_switch_task_namespaces() after task_unlock(),
could there be a potential race condition? I'm not going to dive too
much into this now, because it's getting late here, but on first look
it seems like p->nsproxy could change under our hands before we fetch
it in audit_switch_task_namespaces()...
>
> if (ns && atomic_dec_and_test(&ns->count))
> free_nsproxy(ns);
> --
> 1.8.3.1
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH ghak90 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Ondrej Mosnacek @ 2019-03-27 22:52 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, Simo Sorce, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <56127b2a5b82f15cb0d0f040502c2e3bb6945f30.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 7: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>
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
(Just note another "%llu"/u64 occurence, in case you plan to add the casts.)
> ---
> 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/include/linux/audit.h b/include/linux/audit.h
> index 70255c2dfb9f..723e2d020228 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -214,6 +214,8 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> extern void audit_netns_contid_del(struct net *net, u64 contid);
> extern void audit_switch_task_namespaces(struct nsproxy *ns,
> struct task_struct *p);
> +extern void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context);
>
> extern u32 audit_enabled;
> #else /* CONFIG_AUDIT */
> @@ -290,6 +292,9 @@ static inline void audit_netns_contid_del(struct net *net, u64 contid)
> static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> struct task_struct *p)
> { }
> +static inline void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context)
> +{ }
>
> #define audit_enabled AUDIT_OFF
> #endif /* CONFIG_AUDIT */
> 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) {
> + 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);
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
> index 655187bed5d8..bdb1ec2368a7 100644
> --- a/net/netfilter/nft_log.c
> +++ b/net/netfilter/nft_log.c
> @@ -69,13 +69,16 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> struct sk_buff *skb = pkt->skb;
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (!audit_enabled)
> return;
>
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + context = audit_alloc_local(GFP_ATOMIC);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (!ab)
> - return;
> + goto errout;
>
> audit_log_format(ab, "mark=%#x", skb->mark);
>
> @@ -102,6 +105,10 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> audit_log_format(ab, " saddr=? daddr=? proto=-1");
>
> audit_log_end(ab);
> + net = xt_net(&pkt->xt);
> + audit_log_netns_contid_list(net, context);
> +errout:
> + audit_free_context(context);
> }
>
> static void nft_log_eval(const struct nft_expr *expr,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index af883f1b64f9..a3e547435f13 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (audit_enabled == AUDIT_OFF)
> - goto errout;
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + goto out;
> + context = audit_alloc_local(GFP_ATOMIC);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +107,11 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = xt_net(par);
> + audit_log_netns_contid_list(net, context);
> errout:
> + audit_free_context(context);
> +out:
> return XT_CONTINUE;
> }
>
> --
> 1.8.3.1
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Jonathan Kowalski @ 2019-03-28 0:42 UTC (permalink / raw)
To: Christian Brauner
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190327222543.huugotqcew6jyytv@brauner.io>
pidfd_open is open pidfd for pid relative to pidns, so a better
analogy is that it is like openat for a relative pathname wrt dirfd.
O_DIRECTORY is analogous to what type of object, so a TIDFD flag in
the future which interprets pid (pathname) as thread id only and pins
that specific struct pid. That has now limited you to the specific
object and operations you can perform on said object with other pidfd
APIs.
procfd_open in my proposed signature is just a fancy race free openat
on the corresponding pid of the pidfd relative to the procrootfd
(which becomes the dirfd if i were doing it in userspace), which might
as well been implemented in userspace if things were not racy. Andy
suggested something similar.
My point is, when I am talking of the pidfd API, procfs is irrelevant.
You are thinking of it as a process directory and a process file, I am
thinking of it in terms of a process object and the proc dir fd as an
file system based interface to query process state (through read)
which is why I object to them being usable with pidfd_send_signal as
well, in principle. In the future, people may use task_diag for the
same purpose or a different interface, to work around its limitations.
This would just be another interface of the kernel to query process
state, not representative of the process object itself. Hence, keeping
the pidfd to procfd translation entirely separate (as already
suggested) sounds much, much better to me.
The pidfd API and related calls are untouched and unaffected by
presence, absence of procfs or not (they are, but you do unrelated
stuff in the same system call). To me atleast, munging opening (and
then changing what the procfd means to support the flag use case),
having flags like PIDFD_TO_PROCFD that will work only without
CLONE_NEWPID, then having eg. GET_TIDFID that may work with
CLONE_NEWPID, etc.
I find this interface confusing.
I have a few steps when starting to work with a pidfd:
1. Acquire
pidfd_open(pid, ns, flags) or pidfd_clone(...)
2. Operate
pidfd_send_signal(...)
For those who need a race free way to open the correct /proc/<PID> for
a pidfd relative to a /proc dir fd, for the purposes of metadata
access, you will have procfd_open, which is in the kernel because the
same thing is racy to do in userspace.
Otherwise, pidfd_open in this patchset is this and also a polymorphic
system call that can become procfd_open in my example when passed a
flag. It is doing vastly different things given the presence and
absence of options. This is similar to a multiplexor again, but it
looks more confusing. You have to mask options.
pidfd_open currently:
pidfd_open(pid, -1, -1, 0); gets pidfd in current active ns
pidfd_open(-1, procrootfd, pidfd, PIDFD_TO_PROCFD); returns dir fd of
/proc/<PID> it maps to rel. to proc rootfd
pidfd_open(-1, nsfd, pidfd, CLONE_NEWPID); as you propose this
searches pid in pidns pinned by nsfd, and returns a pidfd file
descriptor.
Extend this to threads in the future, and the combination and
permutation starts getting confusing. Based on the flag, it is
entirely changing what will it work upon, and what it will do.
I can reasonably summarise my pidfd_open and procfd_open in their man
page in one line:
pidfd_open(pid, ns, flags) returns pidfd for pid, searching it in the
pidns pinned by ns fd, and flags will determine further if this is
thread local or process local (i.e. tid or tgid, and tgid == tid for
single threaded) (in the future) (so you could do thread directed
signals by passing a flag to pidfd_send_signal and this pidfd).
Your call without CONFIG_PROC_FS will be literally this, but a few
options will have to be set as -1.
procfd_open(procrootfd, pidfd, flags), returns the proc dir fd for the
pid/tid depending on if the pidfd is thread local, process local, hint
it in flags, etc. It is just a race free wrapper around an openat in
userspace, undergoing the same access control checks.
Yes, pidfd_open as it is now works *just fine*, but it is more
confusing to use and discuss. The conclusion from the previous
discussion also seemed to be to split pidctl's PIDCMD_GET_PIDFD into
its own thing, and provide a translation from pidfd to its proc dir fd
on its own. Then, translate_pid can be its own thing, or you could
extend ioctl_ns(2) if you want.
All that said, thanks for the work on this once again. My intention is
just that we don't end up with an API that could have been done better
and be cleaner to use for potential users in the coming years.
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Richard Guy Briggs @ 2019-03-28 1:12 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: 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: <CAFqZXNskwTub5Bm_HZqr02VPB4s79CcgAVX8VE2vQYP07-qssg@mail.gmail.com>
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?
> > + struct rcu_head rcu;
> > +};
> > +
> > extern int is_audit_feature_set(int which);
> >
> > extern int __init audit_register_class(int class, unsigned *list);
> > @@ -202,6 +210,10 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> > }
> >
> > extern void audit_log_contid(struct audit_context *context, u64 contid);
> > +extern void audit_netns_contid_add(struct net *net, u64 contid);
> > +extern void audit_netns_contid_del(struct net *net, u64 contid);
> > +extern void audit_switch_task_namespaces(struct nsproxy *ns,
> > + struct task_struct *p);
> >
> > extern u32 audit_enabled;
> > #else /* CONFIG_AUDIT */
> > @@ -271,6 +283,13 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> >
> > static inline void audit_log_contid(struct audit_context *context, u64 contid)
> > { }
> > +static inline void audit_netns_contid_add(struct net *net, u64 contid)
> > +{ }
> > +static inline void audit_netns_contid_del(struct net *net, u64 contid)
> > +{ }
> > +static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> > + struct task_struct *p)
> > +{ }
> >
> > #define audit_enabled AUDIT_OFF
> > #endif /* CONFIG_AUDIT */
> > 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;
> > + spin_lock(&aunet->contid_list_lock);
> > + if (!list_empty(contid_list))
> > + 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 (cont) {
> > + INIT_LIST_HEAD(&cont->list);
> > + cont->id = contid;
> > + refcount_set(&cont->refcount, 1);
> > + list_add_rcu(&cont->list, contid_list);
> > + }
> > +out:
> > + spin_unlock(&aunet->contid_list_lock);
> > +}
> > +
> > +void audit_netns_contid_del(struct net *net, u64 contid)
> > +{
> > + struct audit_net *aunet;
> > + struct list_head *contid_list;
> > + struct audit_contid *cont = NULL;
> > +
> > + if (!net)
> > + return;
> > + if (!audit_contid_valid(contid))
> > + return;
> > + aunet = net_generic(net, audit_net_id);
> > + if (!aunet)
> > + return;
> > + contid_list = &aunet->contid_list;
> > + spin_lock(&aunet->contid_list_lock);
> > + if (!list_empty(contid_list))
> > + list_for_each_entry_rcu(cont, contid_list, list)
> > + if (cont->id == contid) {
> > + if (refcount_dec_and_test(&cont->refcount)) {
> > + list_del_rcu(&cont->list);
> > + kfree_rcu(cont, rcu);
> > + }
> > + break;
> > + }
> > + spin_unlock(&aunet->contid_list_lock);
> > +}
> > +
> > +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > +{
> > + u64 contid = audit_get_contid(p);
> > + struct nsproxy *new = p->nsproxy;
> > +
> > + if (!audit_contid_valid(contid))
> > + return;
> > + audit_netns_contid_del(ns->net_ns, contid);
> > + if (new)
> > + audit_netns_contid_add(new->net_ns, contid);
> > +}
> > +
> > void audit_panic(const char *message)
> > {
> > switch (audit_failure) {
> > @@ -1619,7 +1694,6 @@ static int __net_init audit_net_init(struct net *net)
> > .flags = NL_CFG_F_NONROOT_RECV,
> > .groups = AUDIT_NLGRP_MAX,
> > };
> > -
> > struct audit_net *aunet = net_generic(net, audit_net_id);
> >
> > aunet->sk = netlink_kernel_create(net, NETLINK_AUDIT, &cfg);
> > @@ -1628,7 +1702,8 @@ static int __net_init audit_net_init(struct net *net)
> > return -ENOMEM;
> > }
> > aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> > -
> > + INIT_LIST_HEAD(&aunet->contid_list);
> > + spin_lock_init(&aunet->contid_list_lock);
> > return 0;
> > }
> >
> > @@ -2380,6 +2455,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > uid_t uid;
> > struct tty_struct *tty;
> > char comm[sizeof(current->comm)];
> > + struct net *net = task->nsproxy->net_ns;
> >
> > task_lock(task);
> > /* Can't set if audit disabled */
> > @@ -2401,8 +2477,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > rc = -EALREADY;
> > read_unlock(&tasklist_lock);
> > - if (!rc)
> > + if (!rc) {
> > + if (audit_contid_valid(oldcontid))
> > + audit_netns_contid_del(net, oldcontid);
> > task->audit->contid = contid;
> > + audit_netns_contid_add(net, contid);
> > + }
> > task_unlock(task);
> >
> > if (!audit_enabled)
> > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> > index f6c5d330059a..718b1201ae70 100644
> > --- a/kernel/nsproxy.c
> > +++ b/kernel/nsproxy.c
> > @@ -27,6 +27,7 @@
> > #include <linux/syscalls.h>
> > #include <linux/cgroup.h>
> > #include <linux/perf_event.h>
> > +#include <linux/audit.h>
> >
> > static struct kmem_cache *nsproxy_cachep;
> >
> > @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> > struct nsproxy *old_ns = tsk->nsproxy;
> > struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> > struct nsproxy *new_ns;
> > + u64 contid = audit_get_contid(tsk);
> >
> > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> > CLONE_NEWPID | CLONE_NEWNET |
> > @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> > return PTR_ERR(new_ns);
> >
> > tsk->nsproxy = new_ns;
> > + audit_netns_contid_add(new_ns->net_ns, contid);
> > return 0;
> > }
> >
> > @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> > ns = p->nsproxy;
> > p->nsproxy = new;
> > task_unlock(p);
> > + audit_switch_task_namespaces(ns, p);
>
> Since we call audit_switch_task_namespaces() after task_unlock(),
> could there be a potential race condition? I'm not going to dive too
> much into this now, because it's getting late here, but on first look
> it seems like p->nsproxy could change under our hands before we fetch
> it in audit_switch_task_namespaces()...
The rules are defined in include/linux/nsproxy.h.
Since the callers (sys_setns, do_exit, copy_process error path) are all
current or handing it a dead task and we are not writing nsproxy or its
pointers, which is only allowed by current anyway, we don't need the
lock.
> >
> > if (ns && atomic_dec_and_test(&ns->count))
> > free_nsproxy(ns);
> > --
> > 1.8.3.1
>
> Ondrej Mosnacek <omosnace at redhat dot 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 05/10] audit: add containerid support for ptrace and signals
From: Richard Guy Briggs @ 2019-03-28 2:04 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, Simo Sorce, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <CAFqZXNuMjvds9OKoj--zgn5j8CGuW55ZGhTPHtKGb+ZOR6B2-A@mail.gmail.com>
On 2019-03-27 22:17, Ondrej Mosnacek wrote:
> On Fri, Mar 15, 2019 at 7:34 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Add audit container identifier support to ptrace and signals. In
> > particular, the "ref" field provides a way to label the auxiliary record
> > to which it is associated.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 1 +
> > kernel/audit.c | 2 ++
> > kernel/audit.h | 2 ++
> > kernel/auditsc.c | 23 +++++++++++++++++------
> > 4 files changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 43438192ca2a..ebd6625ca80e 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -35,6 +35,7 @@ struct audit_sig_info {
> > uid_t uid;
> > pid_t pid;
> > char ctx[0];
> > + u64 cid;
> > };
>
> It seems like this structure implicitly defines the format of some
> message that is sent to userspace... If so, how will userspace detect
> that a new format (including the cid) is being used? Even assuming the
> fixed order as pointed out by Neil, the message still seems to be
> variable-sized so userspace cannot even use the length to infer that.
> Am I missing something here? (I hope I am :)
How humble of you again. No, you're not missing something. This ends
up being an api change... That can be fixed in userspace by checking
for AUDIT_FEATURE_BITMAP_CONTAINERID, but how do we make a newer kernel
not break an older userspace... I think this was the original rationale
for adding it after the ctx but totally missing the fact that the latter
is a variable-length field.
This patch really should be split into audit_sig_cid changes in a patch
by itself and target_cid changes which could go with the second and
fourth patches.
> > struct audit_buffer;
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 8cc0e88d7f2a..cfa659b3f6c4 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -138,6 +138,7 @@ struct audit_net {
> > kuid_t audit_sig_uid = INVALID_UID;
> > pid_t audit_sig_pid = -1;
> > u32 audit_sig_sid = 0;
> > +u64 audit_sig_cid = AUDIT_CID_UNSET;
> >
> > /* Records can be lost in several ways:
> > 0) [suppressed in audit_alloc]
> > @@ -1515,6 +1516,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> > memcpy(sig_data->ctx, ctx, len);
> > security_release_secctx(ctx, len);
> > }
> > + sig_data->cid = audit_sig_cid;
> > audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> > sig_data, sizeof(*sig_data) + len);
> > kfree(sig_data);
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index c00e2ee3c6b3..c5ac6436317e 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -148,6 +148,7 @@ struct audit_context {
> > kuid_t target_uid;
> > unsigned int target_sessionid;
> > u32 target_sid;
> > + u64 target_cid;
> > char target_comm[TASK_COMM_LEN];
> >
> > struct audit_tree_refs *trees, *first_trees;
> > @@ -344,6 +345,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
> > extern pid_t audit_sig_pid;
> > extern kuid_t audit_sig_uid;
> > extern u32 audit_sig_sid;
> > +extern u64 audit_sig_cid;
> >
> > extern int audit_filter(int msgtype, unsigned int listtype);
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index a8c8b44b954d..f04e115df5dc 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -113,6 +113,7 @@ struct audit_aux_data_pids {
> > kuid_t target_uid[AUDIT_AUX_PIDS];
> > unsigned int target_sessionid[AUDIT_AUX_PIDS];
> > u32 target_sid[AUDIT_AUX_PIDS];
> > + u64 target_cid[AUDIT_AUX_PIDS];
> > char target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> > int pid_count;
> > };
> > @@ -1514,7 +1515,7 @@ static void audit_log_exit(void)
> > for (aux = context->aux_pids; aux; aux = aux->next) {
> > struct audit_aux_data_pids *axs = (void *)aux;
> >
> > - for (i = 0; i < axs->pid_count; i++)
> > + for (i = 0; i < axs->pid_count; i++) {
> > if (audit_log_pid_context(context, axs->target_pid[i],
> > axs->target_auid[i],
> > axs->target_uid[i],
> > @@ -1522,14 +1523,20 @@ static void audit_log_exit(void)
> > axs->target_sid[i],
> > axs->target_comm[i]))
> > call_panic = 1;
> > + audit_log_contid(context, axs->target_cid[i]);
> > + }
> > }
> >
> > - if (context->target_pid &&
> > - audit_log_pid_context(context, context->target_pid,
> > - context->target_auid, context->target_uid,
> > - context->target_sessionid,
> > - context->target_sid, context->target_comm))
> > + if (context->target_pid) {
> > + if (audit_log_pid_context(context, context->target_pid,
> > + context->target_auid,
> > + context->target_uid,
> > + context->target_sessionid,
> > + context->target_sid,
> > + context->target_comm))
> > call_panic = 1;
> > + audit_log_contid(context, context->target_cid);
> > + }
> >
> > if (context->pwd.dentry && context->pwd.mnt) {
> > ab = audit_log_start(context, GFP_KERNEL, AUDIT_CWD);
> > @@ -2360,6 +2367,7 @@ void __audit_ptrace(struct task_struct *t)
> > context->target_uid = task_uid(t);
> > context->target_sessionid = audit_get_sessionid(t);
> > security_task_getsecid(t, &context->target_sid);
> > + context->target_cid = audit_get_contid(t);
> > memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> > }
> >
> > @@ -2387,6 +2395,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> > else
> > audit_sig_uid = uid;
> > security_task_getsecid(current, &audit_sig_sid);
> > + audit_sig_cid = audit_get_contid(current);
> > }
> >
> > if (!audit_signals || audit_dummy_context())
> > @@ -2400,6 +2409,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> > ctx->target_uid = t_uid;
> > ctx->target_sessionid = audit_get_sessionid(t);
> > security_task_getsecid(t, &ctx->target_sid);
> > + ctx->target_cid = audit_get_contid(t);
> > memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> > return 0;
> > }
> > @@ -2421,6 +2431,7 @@ int audit_signal_info(int sig, struct task_struct *t)
> > axp->target_uid[axp->pid_count] = t_uid;
> > axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> > security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
> > + axp->target_cid[axp->pid_count] = audit_get_contid(t);
> > memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> > axp->pid_count++;
> >
> > --
> > 1.8.3.1
> >
>
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Software Engineer, Security Technologies
> Red Hat, Inc.
- 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 23/27] bpf: Restrict kernel image access functions when the kernel is locked down
From: James Morris @ 2019-03-28 3:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Stephen Hemminger, Linux API, Matthew Garrett, LSM List, LKML,
David Howells, Alexei Starovoitov, Network Development,
Chun-Yi Lee, Daniel Borkmann, Kees Cook, Will Drewry
In-Reply-To: <CALCETrUZ+oP0xxNt4mwN=AZA+zvkkfPZiAsFmAuqUD48pOHbBg@mail.gmail.com>
On Tue, 26 Mar 2019, Andy Lutomirski wrote:
> >
> > kernel_is_locked_down("ioperm")
> >
> > becomes
> >
> > kernel_is_locked_down(LOCKDOWN_IOPERM)
> >
> > and this function checks e.g.
> >
> > if (lockdown_polcy[id]) {
> > fail or warn;
> > }
> >
> > Thoughts?
>
> I'm concerned that this gives too much useless flexibility to
> administrators and user code in general. If you can break kernel
> integrity, you can break kernel integrity -- it shouldn't really
> matter *how* you break it.
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.
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.
I'm not sure how to think about it architecturally and how it fits as such
in the mainline kernel.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v7)
From: Martin Schwidefsky @ 2019-03-28 7:49 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Mathieu Desnoyers, Paul Burton, Will Deacon, Boqun Feng,
Heiko Carstens, Vasily Gorbik, Russell King,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, carlos,
Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney
In-Reply-To: <4021516e-6a1e-166d-a4f6-e961e6f94cc4@redhat.com>
On Wed, 27 Mar 2019 16:38:32 -0400
"Carlos O'Donell" <codonell@redhat.com> wrote:
> On 3/27/19 5:16 AM, Martin Schwidefsky wrote:
> > On Mon, 25 Mar 2019 11:54:32 -0400 (EDT)
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >
> >>>> +++ b/sysdeps/unix/sysv/linux/s390/bits/rseq.h
> >> [...]
> >>>> +
> >>>> +/* Signature required before each abort handler code. */
> >>>> +#define RSEQ_SIG 0x53053053
> >>>
> >>> Why not a s390 specific value here?
> >>
> >> s390 also has the abort handler in a __rseq_failure section:
> >>
> >> #define RSEQ_ASM_DEFINE_ABORT(label, teardown, abort_label) \
> >> ".pushsection __rseq_failure, \"ax\"\n\t" \
> >> ".long " __rseq_str(RSEQ_SIG) "\n\t" \
> >> __rseq_str(label) ":\n\t" \
> >> teardown \
> >> "j %l[" __rseq_str(abort_label) "]\n\t" \
> >> ".popsection\n\t"
> >>
> >> Same question applies as powerpc: since disassemblers will try to decode
> >> that instruction, would it be better to define it as a valid one ?
> >>
> >> [...]
> >
> > A 4-byte sequence starting with 0x53 is decoded as a "diebr" instruction.
> > And please replace that "j %l[...]" with a "jg %l[...]", the branch target
> > range of the "j" instruction is 64K, not enough for the general case.
>
> Why was this particular operated selected?
>
> So on s390 the RSEQ_SIG will show up as an unexpected "divide to integer"
> instruction that can't be reached by any control flow?
>
> Can we use a NOP with a unique value in an immediate operand?
>
> The goal being to have something that won't confuse during a debug
> session, or that the debugger can ignore (like constant pools on Arm)
I was looking at the wrong table in regard to opcode 0x53. The pattern
0x53...... is not a known instruction as far as the disassembler is
concerned. As Mathieu pointed out "diebr" is actually 0xb353....
Sorry about the confusion.
But why do we need this value in the first place if it can not be reached?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Ondrej Mosnacek @ 2019-03-28 8:01 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: 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: <20190328011202.6raixwzdimn5b4zk@madcap2.tricolour.ca>
On Thu, Mar 28, 2019 at 2:12 AM 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?
>
> > > + struct rcu_head rcu;
> > > +};
> > > +
> > > extern int is_audit_feature_set(int which);
> > >
> > > extern int __init audit_register_class(int class, unsigned *list);
> > > @@ -202,6 +210,10 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> > > }
> > >
> > > extern void audit_log_contid(struct audit_context *context, u64 contid);
> > > +extern void audit_netns_contid_add(struct net *net, u64 contid);
> > > +extern void audit_netns_contid_del(struct net *net, u64 contid);
> > > +extern void audit_switch_task_namespaces(struct nsproxy *ns,
> > > + struct task_struct *p);
> > >
> > > extern u32 audit_enabled;
> > > #else /* CONFIG_AUDIT */
> > > @@ -271,6 +283,13 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> > >
> > > static inline void audit_log_contid(struct audit_context *context, u64 contid)
> > > { }
> > > +static inline void audit_netns_contid_add(struct net *net, u64 contid)
> > > +{ }
> > > +static inline void audit_netns_contid_del(struct net *net, u64 contid)
> > > +{ }
> > > +static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> > > + struct task_struct *p)
> > > +{ }
> > >
> > > #define audit_enabled AUDIT_OFF
> > > #endif /* CONFIG_AUDIT */
> > > 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;
> > > + spin_lock(&aunet->contid_list_lock);
> > > + if (!list_empty(contid_list))
> > > + 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 (cont) {
> > > + INIT_LIST_HEAD(&cont->list);
> > > + cont->id = contid;
> > > + refcount_set(&cont->refcount, 1);
> > > + list_add_rcu(&cont->list, contid_list);
> > > + }
> > > +out:
> > > + spin_unlock(&aunet->contid_list_lock);
> > > +}
> > > +
> > > +void audit_netns_contid_del(struct net *net, u64 contid)
> > > +{
> > > + struct audit_net *aunet;
> > > + struct list_head *contid_list;
> > > + struct audit_contid *cont = NULL;
> > > +
> > > + if (!net)
> > > + return;
> > > + if (!audit_contid_valid(contid))
> > > + return;
> > > + aunet = net_generic(net, audit_net_id);
> > > + if (!aunet)
> > > + return;
> > > + contid_list = &aunet->contid_list;
> > > + spin_lock(&aunet->contid_list_lock);
> > > + if (!list_empty(contid_list))
> > > + list_for_each_entry_rcu(cont, contid_list, list)
> > > + if (cont->id == contid) {
> > > + if (refcount_dec_and_test(&cont->refcount)) {
> > > + list_del_rcu(&cont->list);
> > > + kfree_rcu(cont, rcu);
> > > + }
> > > + break;
> > > + }
> > > + spin_unlock(&aunet->contid_list_lock);
> > > +}
> > > +
> > > +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > > +{
> > > + u64 contid = audit_get_contid(p);
> > > + struct nsproxy *new = p->nsproxy;
> > > +
> > > + if (!audit_contid_valid(contid))
> > > + return;
> > > + audit_netns_contid_del(ns->net_ns, contid);
> > > + if (new)
> > > + audit_netns_contid_add(new->net_ns, contid);
> > > +}
> > > +
> > > void audit_panic(const char *message)
> > > {
> > > switch (audit_failure) {
> > > @@ -1619,7 +1694,6 @@ static int __net_init audit_net_init(struct net *net)
> > > .flags = NL_CFG_F_NONROOT_RECV,
> > > .groups = AUDIT_NLGRP_MAX,
> > > };
> > > -
> > > struct audit_net *aunet = net_generic(net, audit_net_id);
> > >
> > > aunet->sk = netlink_kernel_create(net, NETLINK_AUDIT, &cfg);
> > > @@ -1628,7 +1702,8 @@ static int __net_init audit_net_init(struct net *net)
> > > return -ENOMEM;
> > > }
> > > aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> > > -
> > > + INIT_LIST_HEAD(&aunet->contid_list);
> > > + spin_lock_init(&aunet->contid_list_lock);
> > > return 0;
> > > }
> > >
> > > @@ -2380,6 +2455,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > > uid_t uid;
> > > struct tty_struct *tty;
> > > char comm[sizeof(current->comm)];
> > > + struct net *net = task->nsproxy->net_ns;
> > >
> > > task_lock(task);
> > > /* Can't set if audit disabled */
> > > @@ -2401,8 +2477,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > > else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > > rc = -EALREADY;
> > > read_unlock(&tasklist_lock);
> > > - if (!rc)
> > > + if (!rc) {
> > > + if (audit_contid_valid(oldcontid))
> > > + audit_netns_contid_del(net, oldcontid);
> > > task->audit->contid = contid;
> > > + audit_netns_contid_add(net, contid);
> > > + }
> > > task_unlock(task);
> > >
> > > if (!audit_enabled)
> > > diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> > > index f6c5d330059a..718b1201ae70 100644
> > > --- a/kernel/nsproxy.c
> > > +++ b/kernel/nsproxy.c
> > > @@ -27,6 +27,7 @@
> > > #include <linux/syscalls.h>
> > > #include <linux/cgroup.h>
> > > #include <linux/perf_event.h>
> > > +#include <linux/audit.h>
> > >
> > > static struct kmem_cache *nsproxy_cachep;
> > >
> > > @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> > > struct nsproxy *old_ns = tsk->nsproxy;
> > > struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> > > struct nsproxy *new_ns;
> > > + u64 contid = audit_get_contid(tsk);
> > >
> > > if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> > > CLONE_NEWPID | CLONE_NEWNET |
> > > @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> > > return PTR_ERR(new_ns);
> > >
> > > tsk->nsproxy = new_ns;
> > > + audit_netns_contid_add(new_ns->net_ns, contid);
> > > return 0;
> > > }
> > >
> > > @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> > > ns = p->nsproxy;
> > > p->nsproxy = new;
> > > task_unlock(p);
> > > + audit_switch_task_namespaces(ns, p);
> >
> > Since we call audit_switch_task_namespaces() after task_unlock(),
> > could there be a potential race condition? I'm not going to dive too
> > much into this now, because it's getting late here, but on first look
> > it seems like p->nsproxy could change under our hands before we fetch
> > it in audit_switch_task_namespaces()...
>
> The rules are defined in include/linux/nsproxy.h.
>
> Since the callers (sys_setns, do_exit, copy_process error path) are all
> current or handing it a dead task and we are not writing nsproxy or its
> pointers, which is only allowed by current anyway, we don't need the
> lock.
I see, so the task lock is taken during the swap only to protect
against races with other tasks reading this task's nsproxy... makes
sense. Thanks for clarifying!
The refcount/spinlock issue is not blocking (and could be addressed in
a follow-up patch later), so:
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> > >
> > > if (ns && atomic_dec_and_test(&ns->count))
> > > free_nsproxy(ns);
> > > --
> > > 1.8.3.1
> >
> > Ondrej Mosnacek <omosnace at redhat dot 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: pidfd design
From: Christian Brauner @ 2019-03-28 9:21 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, Joel Fernandes, Suren Baghdasaryan,
Steven Rostedt, Sultan Alsawaf, Tim Murray, Michal Hocko,
Greg Kroah-Hartman, Arve Hjønnevåg, Todd Kjos,
Martijn Coenen, Ingo Molnar, Peter Zijlstra, LKML,
open list:ANDROID DRIVERS, kernel-team, Oleg Nesterov,
Serge E. Hallyn
In-Reply-To: <CALCETrUSaj+iTEOgEvy30eiCi2+FoN8+bq2BjJmamCjJ4yeBjA@mail.gmail.com>
On Mon, Mar 25, 2019 at 05:24:49PM -0700, Andy Lutomirski wrote:
> On Mon, Mar 25, 2019 at 5:12 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Mon, Mar 25, 2019 at 05:00:17PM -0700, Andy Lutomirski wrote:
> > > On Mon, Mar 25, 2019 at 4:45 PM Christian Brauner <christian@brauner.io> wrote:
> > > >
> > > > On Mon, Mar 25, 2019 at 04:42:14PM -0700, Andy Lutomirski wrote:
> > > > > On Mon, Mar 25, 2019 at 1:23 PM Daniel Colascione <dancol@google.com> wrote:
> > > > > >
> > > > > > On Mon, Mar 25, 2019 at 1:14 PM Jann Horn <jannh@google.com> wrote:
> > > > > > >
> > > > > > > On Mon, Mar 25, 2019 at 8:44 PM Andy Lutomirski <luto@kernel.org> wrote:
> > > > >
> > > > > > > One ioctl on procfs roots to translate pidfds into that procfs,
> > > > > > > subject to both the normal lookup permission checks and only working
> > > > > > > if the pidfd has a translation into the procfs:
> > > > > > >
> > > > > > > int proc_root_fd = open("/proc", O_RDONLY);
> > > > > > > int proc_dir_fd = ioctl(proc_root_fd, PROC_PIDFD_TO_PROCFSFD, pidfd);
> > > > > > >
> > > > > > > And one ioctl on procfs directories to translate from PGIDs and PIDs to pidfds:
> > > > > > >
> > > > > > > int proc_pgid_fd = open("/proc/self", O_RDONLY);
> > > > > > > int self_pg_pidfd = ioctl(proc_pgid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > > > > int proc_pid_fd = open("/proc/thread-self", O_RDONLY);
> > > > > > > int self_p_pidfd = ioctl(proc_pid_fd, PROC_PROCFSFD_TO_PIDFD, 0);
> > > > > > >
> > > > >
> > > > > This sounds okay to me. Or we could make it so that a procfs
> > > > > directory fd also works as a pidfd, but that seems more likely to be
> > > > > problematic than just allowing two-way translation like this
> > > > >
> > > > > > >
> > > > > > > And then, as you proposed, the new sys_clone() can just return a
> > > > > > > pidfd, and you can convert it into a procfs fd yourself if you want.
> > > > > >
> > > > > > I think that's the consensus we reached on the other thread. The
> > > > > > O_DIRECTORY open on /proc/self/fd/mypidfd seems like it'd work well
> > > > > > enough.
> > > > >
> > > > > I must have missed this particular email.
> > > > >
> > > > > IMO, if /proc/self/fd/mypidfd allows O_DIRECTORY open to work, then it
> > > > > really ought to do function just like /proc/self/fd/mypidfd/. and
> > > > > /proc/self/fd/mypidfd/status should work. And these latter two
> > > > > options seem nutty.
> > > > >
> > > > > Also, this O_DIRECTORY thing is missing the entire point of the ioctl
> > > > > interface -- it doesn't require procfs access.
> > > >
> > > > The other option was to encode the pid in the callers pid namespace into
> > > > the pidfd's fdinfo so that you can parse it out and open /proc/<pid>.
> > > > You'd just need an event on the pidfd to tell you when the process has
> > > > died. Jonathan and I just discussed this.
> > >
> > > From an application developer's POV, the ioctl interface sounds much,
> > > much nicer.
> >
> > Some people are strongly against ioctl()s some don't. I'm not against
> > them so both options are fine with me if people can agree.
> >
>
> There are certainly non-ioctl equivalents that are functionally
> equivalent. For example, there could be a syscall
> procfs_open_pidfd(procfs_fd, pid_fd). I personally don't really mind
> ioctl() when it's really an operation on an fd.
I totally missed that mail somehow.
Yes, I agree that an ioctl() makes sense for that.
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-28 10:38 UTC (permalink / raw)
To: Jonathan Kowalski
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <CAGLj2rG0Sbho6MnPSmcS73Xj=oG92E4W_QLRu4dD8NfweoUm5Q@mail.gmail.com>
> All that said, thanks for the work on this once again. My intention is
> just that we don't end up with an API that could have been done better
> and be cleaner to use for potential users in the coming years.
Thanks for your input on all of this. I still don't find multiplexers in
the style of seccomp()/fsconfig()/keyctl() to be a problem since they
deal with a specific task. They are very much different from ioctl()s in
that regard. But since Joel, you, and Daniel found the pidctl() approach
not very nice I dropped it. The interface needs to be satisfactory for
all of us especially since Android and other system managers will be the
main consumers.
So let's split this into pidfd_open(pid_t pid, unsigned int flags) which
allows to cleanly get pidfds independent procfs and do the translation
to procpidfds in an ioctl() as we've discussed in prior threads. This
should also accommodate comments and ideas from Andy and Jann.
I'm coding this up now.
Christian
^ permalink raw reply
* Re: [PATCH 1/4] glibc: Perform rseq(2) registration at C startup and thread creation (v7)
From: Mathieu Desnoyers @ 2019-03-28 15:42 UTC (permalink / raw)
To: schwidefsky
Cc: Carlos O'Donell, Paul Burton, Will Deacon, Boqun Feng,
heiko carstens, gor, Russell King, ARM Linux,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, carlos,
Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Dave Watson <davejwatso>
In-Reply-To: <20190328084948.6486fa67@mschwideX1>
----- On Mar 28, 2019, at 3:49 AM, schwidefsky schwidefsky@de.ibm.com wrote:
> On Wed, 27 Mar 2019 16:38:32 -0400
> "Carlos O'Donell" <codonell@redhat.com> wrote:
>
>> On 3/27/19 5:16 AM, Martin Schwidefsky wrote:
>> > On Mon, 25 Mar 2019 11:54:32 -0400 (EDT)
>> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>> >
>> >>>> +++ b/sysdeps/unix/sysv/linux/s390/bits/rseq.h
>> >> [...]
>> >>>> +
>> >>>> +/* Signature required before each abort handler code. */
>> >>>> +#define RSEQ_SIG 0x53053053
>> >>>
>> >>> Why not a s390 specific value here?
>> >>
>> >> s390 also has the abort handler in a __rseq_failure section:
>> >>
>> >> #define RSEQ_ASM_DEFINE_ABORT(label, teardown, abort_label) \
>> >> ".pushsection __rseq_failure, \"ax\"\n\t" \
>> >> ".long " __rseq_str(RSEQ_SIG) "\n\t" \
>> >> __rseq_str(label) ":\n\t" \
>> >> teardown \
>> >> "j %l[" __rseq_str(abort_label) "]\n\t" \
>> >> ".popsection\n\t"
>> >>
>> >> Same question applies as powerpc: since disassemblers will try to decode
>> >> that instruction, would it be better to define it as a valid one ?
>> >>
>> >> [...]
>> >
>> > A 4-byte sequence starting with 0x53 is decoded as a "diebr" instruction.
>> > And please replace that "j %l[...]" with a "jg %l[...]", the branch target
>> > range of the "j" instruction is 64K, not enough for the general case.
>>
>> Why was this particular operated selected?
The 0x53053053 signature was selected by myself on x86, where it is a
4-byte operand to a no-op instruction. That value looks like "SEQSEQSE"
in hexadecimal. The goal was to have an uncommon code signature value.
Then it has been used as-is on arm and mips within literal pools (which seems
fine), and also on s390 and powerpc where those seem to generate invalid
instruction within a separate code section.
I'm mainly concerned about the choice of this value on s390 and powerpc.
>>
>> So on s390 the RSEQ_SIG will show up as an unexpected "divide to integer"
>> instruction that can't be reached by any control flow?
>>
>> Can we use a NOP with a unique value in an immediate operand?
>>
>> The goal being to have something that won't confuse during a debug
>> session, or that the debugger can ignore (like constant pools on Arm)
>
> I was looking at the wrong table in regard to opcode 0x53. The pattern
> 0x53...... is not a known instruction as far as the disassembler is
> concerned. As Mathieu pointed out "diebr" is actually 0xb353....
> Sorry about the confusion.
>
> But why do we need this value in the first place if it can not be reached?
One reason is to help disassemblers in tools like objdump, gdb, and so on.
Another reason for making this a valid instruction is if the CPU speculative
execution can be helped by making this instruction a valid one, even though
it's not reachable through normal execution. (this appears to be important
at least on aarch64)
However, we want that instruction to be an uncommon one, to reduce the chances
that an attacker can use the rseq abort mechanism to redirect execution to
an unrelated code block that would happen to follow that same 4-byte signature.
For instance, I would not use a no-op which is typically generated by compilers
there.
In summary:
* x86: Uses a no-op instruction ending with a 4-byte operand.
* aarch64: Uses a trap instruction which also has a seldom-used operand value.
* arm: The signature is in a literal pool (there is a jump over the signature).
* mips: The signature is in a literal pool (there is a jump over the signature).
* powerpc: the signature is an unreachable invalid(?) instruction within an
executable section.
* s390: the signature is an unreachable invalid instruction within an executable
section.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Paul Moore @ 2019-03-28 15:46 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: <20190328011202.6raixwzdimn5b4zk@madcap2.tricolour.ca>
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.
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
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Joel Fernandes @ 2019-03-28 16:59 UTC (permalink / raw)
To: Christian Brauner, Jonathan Kowalski
Cc: Jann Horn, Konstantin Khlebnikov, Andy Lutomirski, David Howells,
Serge E. Hallyn, Eric W. Biederman, Linux API, linux-kernel,
Arnd Bergmann, Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai, Al Viro
In-Reply-To: <20190328103813.eogszrqbitw3e7k7@brauner.io>
On March 28, 2019 6:38:15 AM EDT, Christian Brauner <christian@brauner.io> wrote:
>> All that said, thanks for the work on this once again. My intention
>is
>> just that we don't end up with an API that could have been done
>better
>> and be cleaner to use for potential users in the coming years.
>
>Thanks for your input on all of this. I still don't find multiplexers
>in
>the style of seccomp()/fsconfig()/keyctl() to be a problem since they
>deal with a specific task. They are very much different from ioctl()s
>in
>that regard. But since Joel, you, and Daniel found the pidctl()
>approach
>not very nice I dropped it. The interface needs to be satisfactory for
>all of us especially since Android and other system managers will be
>the
>main consumers.
>So let's split this into pidfd_open(pid_t pid, unsigned int flags)
>which
>allows to cleanly get pidfds independent procfs and do the translation
>to procpidfds in an ioctl() as we've discussed in prior threads. This
>should also accommodate comments and ideas from Andy and Jann.
>I'm coding this up now.
This sounds quite sensible to me. Thanks!
Joel Fernandes, Android kernel team
Sent from k9-mail on Android
^ permalink raw reply
* Re: [PATCH 2/4] pid: add pidfd_open()
From: Christian Brauner @ 2019-03-28 17:07 UTC (permalink / raw)
To: Joel Fernandes
Cc: Jonathan Kowalski, Jann Horn, Konstantin Khlebnikov,
Andy Lutomirski, David Howells, Serge E. Hallyn,
Eric W. Biederman, Linux API, linux-kernel, Arnd Bergmann,
Kees Cook, Alexey Dobriyan, Thomas Gleixner,
Michael Kerrisk-manpages, Dmitry V. Levin, Andrew Morton,
Oleg Nesterov, Nagarathnam Muthusamy, Aleksa Sarai
In-Reply-To: <533075A9-A6CF-4549-AFC8-B90505B198FD@joelfernandes.org>
On Thu, Mar 28, 2019 at 12:59:46PM -0400, Joel Fernandes wrote:
>
>
> On March 28, 2019 6:38:15 AM EDT, Christian Brauner <christian@brauner.io> wrote:
> >> All that said, thanks for the work on this once again. My intention
> >is
> >> just that we don't end up with an API that could have been done
> >better
> >> and be cleaner to use for potential users in the coming years.
> >
> >Thanks for your input on all of this. I still don't find multiplexers
> >in
> >the style of seccomp()/fsconfig()/keyctl() to be a problem since they
> >deal with a specific task. They are very much different from ioctl()s
> >in
> >that regard. But since Joel, you, and Daniel found the pidctl()
> >approach
> >not very nice I dropped it. The interface needs to be satisfactory for
> >all of us especially since Android and other system managers will be
> >the
> >main consumers.
> >So let's split this into pidfd_open(pid_t pid, unsigned int flags)
> >which
> >allows to cleanly get pidfds independent procfs and do the translation
> >to procpidfds in an ioctl() as we've discussed in prior threads. This
> >should also accommodate comments and ideas from Andy and Jann.
> >I'm coding this up now.
>
> This sounds quite sensible to me. Thanks!
Thanks! I have it ready and hope to send it out later today.
Christian
^ permalink raw reply
* 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
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