* Re: [PATCH ghak90 V5 04/10] audit: log container info of syscalls
From: Neil Horman @ 2019-03-16 22:44 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <85fcd0a81adef25cb60b2e479bbb380e76dbf999.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:52PM -0400, Richard Guy Briggs 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>
> ---
> 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);
> + 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)
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Deepa Dinamani @ 2019-03-17 18:20 UTC (permalink / raw)
To: Florian Weimer
Cc: Arnd Bergmann, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API
In-Reply-To: <87a7hvded7.fsf@mid.deneb.enyo.de>
On Fri, Mar 15, 2019 at 2:20 PM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Arnd Bergmann:
>
> > On Thu, Mar 14, 2019 at 7:41 PM Florian Weimer <fw@deneb.enyo.de> wrote:
> >>
> >> * Arnd Bergmann:
> >>
> >> > diff --git a/arch/alpha/include/uapi/asm/socket.h
> >> > b/arch/alpha/include/uapi/asm/socket.h
> >> > index 0d0fddb7e738..976e89b116e5 100644
> >> > --- a/arch/alpha/include/uapi/asm/socket.h
> >> > +++ b/arch/alpha/include/uapi/asm/socket.h
> >> > @@ -2,8 +2,8 @@
> >> > #ifndef _UAPI_ASM_SOCKET_H
> >> > #define _UAPI_ASM_SOCKET_H
> >> >
> >> > +#include <linux/posix_types.h>
> >> > #include <asm/sockios.h>
> >> > -#include <asm/bitsperlong.h>
> >>
> >> This breaks POSIX conformance in glibc because the
> >> <linux/posix_types.h> header is not namespace clean. It contains the
> >> identifiers fds_bits and val:
> >>
> >> unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
> >>
> >> int val[2];
> >
> > What is problematic about the struct members here? I had thought that
> > only the struct names have to be in a namespace to be usable here,
> > but not the members.
>
> According POSIX, a user can do this:
>
> #define fds_bits 1024
>
> before including the <sys/socket.h> header file. Similarly for val.
>
> Since glibc pulls in <asm/socket.h> indirectly, the result is a parse
> error, even though the programmer did nothing wrong (fds_bits is not
> an identifier used by POSIX, nor is it in the implementation
> namespace, ans <sys/socket.h> is a POSIX header).
>
> > We could use asm/posix_types.h instead of linux/posix_types.h,
> > would that address your concern?
>
> It should fix the fds_bits case, I think. But
> <asm-generic/posix_types.h> still uses val, so that part of the issue
> remains.
Would moving kernel namespace types(__kernel prefix) to a different
header file(kernel_types.h?) and then including this from
linux/posix_types.h.
And, for socket.h just including kernel_types.h make sense?
-Deepa
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Arnd Bergmann @ 2019-03-18 8:27 UTC (permalink / raw)
To: Deepa Dinamani
Cc: Florian Weimer, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API, Josh Boyer,
Linus Torvalds, Jeff Law
In-Reply-To: <CABeXuvrEecSAkBuN-SmAbGwhLWNCC+bA1=X78fm8xMyw8=hm6Q@mail.gmail.com>
On Sun, Mar 17, 2019 at 7:20 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote:
> On Fri, Mar 15, 2019 at 2:20 PM Florian Weimer <fw@deneb.enyo.de> wrote:
> > > On Thu, Mar 14, 2019 at 7:41 PM Florian Weimer <fw@deneb.enyo.de> wrote:
> > >> > diff --git a/arch/alpha/include/uapi/asm/socket.h
> > >> > b/arch/alpha/include/uapi/asm/socket.h
> > >> > index 0d0fddb7e738..976e89b116e5 100644
> > >> > --- a/arch/alpha/include/uapi/asm/socket.h
> > >> > +++ b/arch/alpha/include/uapi/asm/socket.h
> > >> > @@ -2,8 +2,8 @@
> > >> > #ifndef _UAPI_ASM_SOCKET_H
> > >> > #define _UAPI_ASM_SOCKET_H
> > >> >
> > >> > +#include <linux/posix_types.h>
> > >> > #include <asm/sockios.h>
> > >> > -#include <asm/bitsperlong.h>
> > >>
> > >> This breaks POSIX conformance in glibc because the
> > >> <linux/posix_types.h> header is not namespace clean. It contains the
> > >> identifiers fds_bits and val:
> > >>
> > >> unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
> > >>
> > >> int val[2];
> > >
> > > What is problematic about the struct members here? I had thought that
> > > only the struct names have to be in a namespace to be usable here,
> > > but not the members.
> >
> > According POSIX, a user can do this:
> >
> > #define fds_bits 1024
> >
> > before including the <sys/socket.h> header file. Similarly for val.
> >
> > Since glibc pulls in <asm/socket.h> indirectly, the result is a parse
> > error, even though the programmer did nothing wrong (fds_bits is not
> > an identifier used by POSIX, nor is it in the implementation
> > namespace, ans <sys/socket.h> is a POSIX header).
Ok, I see. Thanks for the explanation!
> > > We could use asm/posix_types.h instead of linux/posix_types.h,
> > > would that address your concern?
> >
> > It should fix the fds_bits case, I think. But
> > <asm-generic/posix_types.h> still uses val, so that part of the issue
> > remains.
>
> Would moving kernel namespace types(__kernel prefix) to a different
> header file(kernel_types.h?) and then including this from
> linux/posix_types.h.
> And, for socket.h just including kernel_types.h make sense?
I fear we have considered linux/posix_types.h to be something that
can be included anywhere for a long time, so it may be better to
ensure that this is actually the case, and avoid the problem with those
two structures but leave the rest untouched.
I think we can move __kernel_fsid_t into include/uapi/asm-generic/statfs.h,
which is the only thing that needs it anyway. We have two definitions of
it today, the non-generic one being for mips32, but incidentally there was
a patch the other day to remove that and use the generic one instead.
With that done, we can change asm/socket.h to just use asm/posix_types.h.
I would still prefer to solve the problem for linux/posix_types.h as well,
but I'm not sure even how __kernel_fd_set is used today in
user space, if at all. Commit 8ded2bbc1845 ("posix_types.h: Cleanup
stale __NFDBITS and related definitions") removed most of the fd_set
definition after a long discussion [1], and since then it has been
basically impossible to use 'struct fd_set' from the kernel in a
meaningful way without including the libc headers or duplicating
them.
Should we just remove __kernel_fd_set from the exported headers and
define the internal fd_set directly in include/linux/types.h? (Adding the
folks from the old thread to Cc).
Arnd
[1] https://lore.kernel.org/lkml/20120724181209.GA10534@zod.bos.redhat.com/t/
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Florian Weimer @ 2019-03-18 9:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Deepa Dinamani, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API, Josh Boyer,
Linus Torvalds, Jeff Law
In-Reply-To: <CAK8P3a22n5L45Gknqd=2zinVGqRovkk0OABmGLJbGcmg8xXd+A@mail.gmail.com>
* Arnd Bergmann:
> Should we just remove __kernel_fd_set from the exported headers and
> define the internal fd_set directly in include/linux/types.h? (Adding the
> folks from the old thread to Cc).
The type is used in the sanitizers, but incorrectly. They assume that
FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is
itself somewhat questionable because it leads to such bugs.) Moving
around the type could cause a build failure in the sanitizers, but I'm
not entirely clear how the UAPI headers are included there.
Otherwise, I couldn't find any uses.
^ permalink raw reply
* Re: [PATCH ghak90 V5 03/10] audit: read container ID of a process
From: Neil Horman @ 2019-03-18 11:10 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <25543e3db4d8f3853119f92c09a60664555249a2.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:51PM -0400, Richard Guy Briggs wrote:
> Add support for reading the audit container identifier from the proc
> filesystem.
>
> This is a read from the proc entry of the form
> /proc/PID/audit_containerid where PID is the process ID of the task
> whose audit container identifier is sought.
>
> The read expects up to a u64 value (unset: 18446744073709551615).
>
> This read requires CAP_AUDIT_CONTROL.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> fs/proc/base.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 2505c46c8701..0b833cbdf5b6 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> .llseek = generic_file_llseek,
> };
>
> +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + struct task_struct *task = get_proc_task(inode);
> + ssize_t length;
> + char tmpbuf[TMPBUFLEN*2];
> +
Sorry, didn't notice this previously, but..
Why *2 here? Its not wrong per-se, but would it be better to just change
TMPBUFLEN to be 22 bytes unilaterally? Its only ever used on stack calls that
arent that deep, and then you won't have to think about adjusting this call site
if you ever change the value of TMPBUFLEN in the future.
I'm fine with doing this in another patch later, but it seems like a worthwhile
cleanup
functionality looks good beyond that nit.
> + if (!task)
> + return -ESRCH;
> + /* if we don't have caps, reject */
> + if (!capable(CAP_AUDIT_CONTROL))
> + return -EPERM;
> + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> + put_task_struct(task);
> + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> +}
> +
> static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> }
>
> static const struct file_operations proc_contid_operations = {
> + .read = proc_contid_read,
> .write = proc_contid_write,
> .llseek = generic_file_llseek,
> };
> @@ -3039,7 +3058,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
> #ifdef CONFIG_AUDIT
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3428,7 +3447,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> #ifdef CONFIG_AUDIT
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Patrick Bellasi @ 2019-03-18 12:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313201821.GW2482@worktop.programming.kicks-ass.net>
On 13-Mar 21:18, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> > +static void uclamp_fork(struct task_struct *p)
> > +{
> > + unsigned int clamp_id;
> > +
> > + if (unlikely(!p->sched_class->uclamp_enabled))
> > + return;
> > +
> > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> > + p->uclamp[clamp_id].active = false;
> > +}
>
> Because in that case .active == false, and copy_process() will have done
> thr right thing?
Don't really get what you mean here? :/
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Arnd Bergmann @ 2019-03-18 12:56 UTC (permalink / raw)
To: Florian Weimer
Cc: Deepa Dinamani, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API, Josh Boyer,
Linus Torvalds, Jeff Law
In-Reply-To: <87o968y1uv.fsf@mid.deneb.enyo.de>
On Mon, Mar 18, 2019 at 10:25 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Arnd Bergmann:
>
> > Should we just remove __kernel_fd_set from the exported headers and
> > define the internal fd_set directly in include/linux/types.h? (Adding the
> > folks from the old thread to Cc).
>
> The type is used in the sanitizers, but incorrectly. They assume that
> FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is
> itself somewhat questionable because it leads to such bugs.)
> Moving around the type could cause a build failure in the sanitizers, but I'm
> not entirely clear how the UAPI headers are included there.
It looks like sanitizer_platform_limits_posix.cc includes
linux/posix_types.h to ensure that __kernel_fd_set is the same
size as __sanitizer___kernel_fd_set, and then it uses the
latter afterwards.
What I don't see here is what kind of operation is actually done
on the data, I only see a cast to void. If libsanitizer actually does
anything interesting here, we should definitely fix it to use the
correct size, especially since this is actually something that
can trigger a buffer overflow in subtle ways when used carelessly.
See for example [1], which we still have not addressed
(I suspect we actually need to have glibc use __kernel_long_t
instead of 'long int' here, but that is a separate issue, and
not overly important given how few users there are on x32).
For this specific use (and probably others like it), renaming the
fds_bits member to __kernel_fds_bits or something like that
would keep user space still compiling. That would only break
if someone was using __kernel_fd_set, and actually doing
bit operations on it. glibc uses '__fds_bits' unless __USE_XOPEN
is set, so maybe we should use use that name unconditionally.
> Otherwise, I couldn't find any uses.
^ permalink raw reply
* Re: [PATCH v7 06/15] sched/core: uclamp: Reset uclamp values on RESET_ON_FORK
From: Patrick Bellasi @ 2019-03-18 12:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190313205235.GX2482@worktop.programming.kicks-ass.net>
On 13-Mar 21:52, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:45AM +0000, Patrick Bellasi wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 070caa1f72eb..8b282616e9c9 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1071,7 +1071,7 @@ static void __setscheduler_uclamp(struct task_struct *p,
> > }
> > }
> >
> > -static void uclamp_fork(struct task_struct *p)
> > +static void uclamp_fork(struct task_struct *p, bool reset)
> > {
> > unsigned int clamp_id;
> >
> > @@ -1080,6 +1080,17 @@ static void uclamp_fork(struct task_struct *p)
>
> IIRC there's an early return here if the class doesn't have uclamp
> support, which I think is wrong now. You want the reset irrespective of
> whether the class supports it, no?
Yep, actually... since in this method we are always and only resetting
certain values, it's probably better to just remove the check on class
support and unconditionally do the required ones.
> > for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> > p->uclamp[clamp_id].active = false;
> > +
> > + if (likely(!reset))
> > + return;
> > +
> > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id) {
> > + unsigned int clamp_value = uclamp_none(clamp_id);
> > +
> > + p->uclamp[clamp_id].user_defined = false;
> > + p->uclamp[clamp_id].value = clamp_value;
> > + p->uclamp[clamp_id].bucket_id = uclamp_bucket_id(clamp_value);
> > + }
> > }
>
>
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-18 13:10 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190318121804.tr3myhtgczvovmkq@e110439-lin>
On Mon, Mar 18, 2019 at 12:18:04PM +0000, Patrick Bellasi wrote:
> On 13-Mar 21:18, Peter Zijlstra wrote:
> > On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> > > +static void uclamp_fork(struct task_struct *p)
> > > +{
> > > + unsigned int clamp_id;
> > > +
> > > + if (unlikely(!p->sched_class->uclamp_enabled))
> > > + return;
> > > +
> > > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> > > + p->uclamp[clamp_id].active = false;
> > > +}
> >
> > Because in that case .active == false, and copy_process() will have done
> > thr right thing?
>
> Don't really get what you mean here? :/
Why don't we have to set .active=false when
!sched_class->uclamp_enabled?
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Florian Weimer @ 2019-03-18 13:12 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Deepa Dinamani, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API, Josh Boyer,
Linus Torvalds, Jeff Law
In-Reply-To: <CAK8P3a1DzRhiuNxBeQaTYSX1NhCaN6+B0Ya7aeGfxUGj35LFhw@mail.gmail.com>
* Arnd Bergmann:
> On Mon, Mar 18, 2019 at 10:25 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>>
>> * Arnd Bergmann:
>>
>> > Should we just remove __kernel_fd_set from the exported headers and
>> > define the internal fd_set directly in include/linux/types.h? (Adding the
>> > folks from the old thread to Cc).
>>
>> The type is used in the sanitizers, but incorrectly. They assume that
>> FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is
>> itself somewhat questionable because it leads to such bugs.)
>> Moving around the type could cause a build failure in the sanitizers, but I'm
>> not entirely clear how the UAPI headers are included there.
>
> It looks like sanitizer_platform_limits_posix.cc includes
> linux/posix_types.h to ensure that __kernel_fd_set is the same
> size as __sanitizer___kernel_fd_set, and then it uses the
> latter afterwards.
>
> What I don't see here is what kind of operation is actually done
> on the data, I only see a cast to void.
I think it is used to assert that the select family of system calls
writes to the 1024 bits for each of the passed pointers. Which is not
actually true—the write size is controlled by the file descriptor
count argument.
> If libsanitizer actually does
> anything interesting here, we should definitely fix it to use the
> correct size, especially since this is actually something that
> can trigger a buffer overflow in subtle ways when used carelessly.
> See for example [1], which we still have not addressed
The footnote is missing.
> For this specific use (and probably others like it), renaming the
> fds_bits member to __kernel_fds_bits or something like that
> would keep user space still compiling. That would only break
> if someone was using __kernel_fd_set, and actually doing
> bit operations on it. glibc uses '__fds_bits' unless __USE_XOPEN
> is set, so maybe we should use use that name unconditionally.
Please use something that is more obviously Linux-specific.
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Patrick Bellasi @ 2019-03-18 14:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190318131050.GQ6058@hirez.programming.kicks-ass.net>
On 18-Mar 14:10, Peter Zijlstra wrote:
> On Mon, Mar 18, 2019 at 12:18:04PM +0000, Patrick Bellasi wrote:
> > On 13-Mar 21:18, Peter Zijlstra wrote:
> > > On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> > > > +static void uclamp_fork(struct task_struct *p)
> > > > +{
> > > > + unsigned int clamp_id;
> > > > +
> > > > + if (unlikely(!p->sched_class->uclamp_enabled))
> > > > + return;
> > > > +
> > > > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> > > > + p->uclamp[clamp_id].active = false;
> > > > +}
> > >
> > > Because in that case .active == false, and copy_process() will have done
> > > thr right thing?
> >
> > Don't really get what you mean here? :/
>
> Why don't we have to set .active=false when
> !sched_class->uclamp_enabled?
Ok, got it.
In principle because:
- FAIR and RT will have uclamp_enabled
- DL cannot fork
... thus, yes, it seems that the check above is not necessary anyway.
Moreover, as per one of your comments in another message, we still need
to cover the "reset on fork" case for FAIR and RT. Thus, I'm going to
completely remove the support check in uclamp_fork and we always reset
active for all classes.
Cheers!
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-18 14:29 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190318142152.44biczhoby44xlfz@e110439-lin>
On Mon, Mar 18, 2019 at 02:21:52PM +0000, Patrick Bellasi wrote:
> On 18-Mar 14:10, Peter Zijlstra wrote:
> > On Mon, Mar 18, 2019 at 12:18:04PM +0000, Patrick Bellasi wrote:
> > > On 13-Mar 21:18, Peter Zijlstra wrote:
> > > > On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> > > > > +static void uclamp_fork(struct task_struct *p)
> > > > > +{
> > > > > + unsigned int clamp_id;
> > > > > +
> > > > > + if (unlikely(!p->sched_class->uclamp_enabled))
> > > > > + return;
> > > > > +
> > > > > + for (clamp_id = 0; clamp_id < UCLAMP_CNT; ++clamp_id)
> > > > > + p->uclamp[clamp_id].active = false;
> > > > > +}
> > > >
> > > > Because in that case .active == false, and copy_process() will have done
> > > > thr right thing?
> > >
> > > Don't really get what you mean here? :/
> >
> > Why don't we have to set .active=false when
> > !sched_class->uclamp_enabled?
>
> Ok, got it.
>
> In principle because:
> - FAIR and RT will have uclamp_enabled
> - DL cannot fork
>
> ... thus, yes, it seems that the check above is not necessary anyway.
>
> Moreover, as per one of your comments in another message, we still need
> to cover the "reset on fork" case for FAIR and RT. Thus, I'm going to
> completely remove the support check in uclamp_fork and we always reset
> active for all classes.
Right, thanks!
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Arnd Bergmann @ 2019-03-18 14:34 UTC (permalink / raw)
To: Florian Weimer
Cc: Deepa Dinamani, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API, Josh Boyer,
Linus Torvalds, Jeff Law
In-Reply-To: <877ecwwckm.fsf@mid.deneb.enyo.de>
On Mon, Mar 18, 2019 at 2:12 PM Florian Weimer <fw@deneb.enyo.de> wrote:
> > On Mon, Mar 18, 2019 at 10:25 AM Florian Weimer <fw@deneb.enyo.de> wrote:
> >>
> >> * Arnd Bergmann:
> >>
> >> > Should we just remove __kernel_fd_set from the exported headers and
> >> > define the internal fd_set directly in include/linux/types.h? (Adding the
> >> > folks from the old thread to Cc).
> >>
> >> The type is used in the sanitizers, but incorrectly. They assume that
> >> FD_SETSIZE is always 1024. (The existence of __kernel_fd_set is
> >> itself somewhat questionable because it leads to such bugs.)
> >> Moving around the type could cause a build failure in the sanitizers, but I'm
> >> not entirely clear how the UAPI headers are included there.
> >
> > It looks like sanitizer_platform_limits_posix.cc includes
> > linux/posix_types.h to ensure that __kernel_fd_set is the same
> > size as __sanitizer___kernel_fd_set, and then it uses the
> > latter afterwards.
> >
> > What I don't see here is what kind of operation is actually done
> > on the data, I only see a cast to void.
>
> I think it is used to assert that the select family of system calls
> writes to the 1024 bits for each of the passed pointers.
Yes, that is what I expected to see in libsanitizer, I just couldn't
find any code that actually does this check.
> Which is not actually true—the write size is controlled by the
> file descriptor count argument.
Yes, of course. In fact, I see multiple possible problems that
- kernel reading uninitialized data if 'FD_ZERO()' was
used with a shorter size than the count argument.
- kernel writing beyond the fd_set data on stack
when the declaration had a shorter size than the count
argument.
Each one could happen either because __FD_SETSIZE
is smaller than 'count', or because kernel and user space
disagree on the element size (32 vs 64 bit on x32).
> > If libsanitizer actually does
> > anything interesting here, we should definitely fix it to use the
> > correct size, especially since this is actually something that
> > can trigger a buffer overflow in subtle ways when used carelessly.
> > See for example [1], which we still have not addressed
>
> The footnote is missing.
Sorry, I meant [1] https://patchwork.kernel.org/patch/10245053/
> > For this specific use (and probably others like it), renaming the
> > fds_bits member to __kernel_fds_bits or something like that
> > would keep user space still compiling. That would only break
> > if someone was using __kernel_fd_set, and actually doing
> > bit operations on it. glibc uses '__fds_bits' unless __USE_XOPEN
> > is set, so maybe we should use use that name unconditionally.
>
> Please use something that is more obviously Linux-specific.
Ok, so not '__fds_bits'.
Is '__kernel_fds_bits' ok? I would prefer to keep at least the
name __kernel_ namespace that we have for typedefs and the
occasional struct tag.
Arnd
^ permalink raw reply
* Re: [PATCH] y2038: fix socket.h header inclusion
From: Florian Weimer @ 2019-03-18 14:37 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Deepa Dinamani, David S . Miller, Willem de Bruijn, alpha,
linux-arch, linux-mips, Parisc List, sparclinux, Laura Abbott,
Networking, Linux Kernel Mailing List, Linux API, Josh Boyer,
Linus Torvalds, Jeff Law
In-Reply-To: <CAK8P3a3X26niT8Y8mWCNXgcRkWhT=ADK-Tt2vjYz6SLj90shCQ@mail.gmail.com>
* Arnd Bergmann:
> Ok, so not '__fds_bits'.
>
> Is '__kernel_fds_bits' ok? I would prefer to keep at least the
> name __kernel_ namespace that we have for typedefs and the
> occasional struct tag.
glibc should be okay with that. We use __kernel_ in the math
libraries for something completely different, but those files do not
(or should not) include UAPI headers, and in any case, the set of such
identifiers is really small.
^ permalink raw reply
* Re: [PATCH v7 10/15] sched/fair: uclamp: Add uclamp support to energy_compute()
From: Patrick Bellasi @ 2019-03-18 15:19 UTC (permalink / raw)
To: Quentin Perret
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190306172124.tpr32k6hawos7a3g@queper01-lin>
On 06-Mar 17:21, Quentin Perret wrote:
[...]
> > Since we are at that:
> > - rename schedutil_freq_util() into schedutil_cpu_util(),
> > since it's not only used for frequency selection.
> > - use "unsigned int" instead of "unsigned long" whenever the tracked
> > utilization value is not expected to overflow 32bit.
>
> We use unsigned long all over the place right ? All the task_util*()
> functions return unsigned long, the capacity-related functions too, and
> util_avg is an unsigned long in sched_avg. So I'm not sure if we want to
> do this TBH.
For utilization we never need more then an "unsigned int" as storage
class. Even at RQ level, 32bits allows +4mln tasks.
However we started with long and keep going on with that, this was
just an attempt to incrementally fix that whenever we do some
changes or we add some new code.
But, perhaps a single whole sale update patch would fit better this
job in case we really wanna do it at some point.
I'll drop this change in v8 and keep this patch focused on functional
bits, don't want to risk to sidetrack the discussion again.
[...]
> > @@ -283,13 +284,14 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> > static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
> > {
> > struct rq *rq = cpu_rq(sg_cpu->cpu);
> > - unsigned long util = cpu_util_cfs(rq);
> > - unsigned long max = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
> > + unsigned int util_cfs = cpu_util_cfs(rq);
> > + unsigned int cpu_cap = arch_scale_cpu_capacity(NULL, sg_cpu->cpu);
>
> Do you really need this one ? What's wrong with 'max' :-) ?
Being a pretty "generic" and thus confusing name is not enough? :)
Anyway, same reasoning as above and same conclusions: I'll drop the
renaming so that we don't sidetrack the discussion on v8.
[...]
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index de181b8a3a2a..b9acef080d99 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2335,6 +2335,7 @@ static inline unsigned long capacity_orig_of(int cpu)
> > #endif
> >
> > #ifdef CONFIG_CPU_FREQ_GOV_SCHEDUTIL
> > +
> > /**
> > * enum schedutil_type - CPU utilization type
>
> Since you're using this enum unconditionally in fair.c, you should to
> move it out of the #ifdef CONFIG_CPU_FREQ_GOV_SCHEDUTIL block, I think.
>
> > * @FREQUENCY_UTIL: Utilization used to select frequency
> > @@ -2350,15 +2351,9 @@ enum schedutil_type {
> > ENERGY_UTIL,
> > };
Good point, will do!
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 12/15] sched/core: uclamp: Propagate parent clamps
From: Patrick Bellasi @ 2019-03-18 16:54 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: LKML, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle
In-Reply-To: <CAJuCfpHTtokPFHu3KHYGj9arbPYn6ZZ=-qNpmVPrDwhPvnA9Ow@mail.gmail.com>
On 14-Mar 09:17, Suren Baghdasaryan wrote:
> On Fri, Feb 8, 2019 at 2:06 AM Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> >
> > In order to properly support hierarchical resources control, the cgroup
> > delegation model requires that attribute writes from a child group never
> > fail but still are (potentially) constrained based on parent's assigned
> > resources. This requires to properly propagate and aggregate parent
> > attributes down to its descendants.
> >
> > Let's implement this mechanism by adding a new "effective" clamp value
> > for each task group. The effective clamp value is defined as the smaller
> > value between the clamp value of a group and the effective clamp value
> > of its parent. This is the actual clamp value enforced on tasks in a
> > task group.
>
> In patch 10 in this series you mentioned "b) do not enforce any
> constraints and/or dependencies between the parent and its child
> nodes"
>
> This patch seems to change that behavior. If so, should it be documented?
Not, I actually have to update the changelog of that patch.
What I mean is that we do not enforce constraints among "requested"
values thus ensuring that each sub-group can always request a clamp
value.
Of course, if it gets that value or not depends on parent constraints,
which are propagated down the hierarchy under the form of "effective"
values by cpu_util_update_heir()
I'll fix the changelog in patch 10 which seems to be confusing for
Tejun too.
[...]
> > @@ -7011,6 +7029,53 @@ static void cpu_cgroup_attach(struct cgroup_taskset *tset)
> > }
> >
> > #ifdef CONFIG_UCLAMP_TASK_GROUP
> > +static void cpu_util_update_hier(struct cgroup_subsys_state *css,
>
> s/cpu_util_update_hier/cpu_util_update_heir ?
Mmm... why?
That "_hier" stands for "hierarchical".
However, since there we update the effective values, maybe I can
better rename it in "_eff" ?
> > + unsigned int clamp_id, unsigned int bucket_id,
> > + unsigned int value)
> > +{
> > + struct cgroup_subsys_state *top_css = css;
> > + struct uclamp_se *uc_se, *uc_parent;
> > +
> > + css_for_each_descendant_pre(css, top_css) {
> > + /*
> > + * The first visited task group is top_css, which clamp value
> > + * is the one passed as parameter. For descendent task
> > + * groups we consider their current value.
> > + */
> > + uc_se = &css_tg(css)->uclamp[clamp_id];
> > + if (css != top_css) {
> > + value = uc_se->value;
> > + bucket_id = uc_se->effective.bucket_id;
> > + }
> > + uc_parent = NULL;
> > + if (css_tg(css)->parent)
> > + uc_parent = &css_tg(css)->parent->uclamp[clamp_id];
> > +
> > + /*
> > + * Skip the whole subtrees if the current effective clamp is
> > + * already matching the TG's clamp value.
> > + * In this case, all the subtrees already have top_value, or a
> > + * more restrictive value, as effective clamp.
> > + */
> > + if (uc_se->effective.value == value &&
> > + uc_parent && uc_parent->effective.value >= value) {
> > + css = css_rightmost_descendant(css);
> > + continue;
> > + }
> > +
> > + /* Propagate the most restrictive effective value */
> > + if (uc_parent && uc_parent->effective.value < value) {
> > + value = uc_parent->effective.value;
> > + bucket_id = uc_parent->effective.bucket_id;
> > + }
> > + if (uc_se->effective.value == value)
> > + continue;
> > +
> > + uc_se->effective.value = value;
> > + uc_se->effective.bucket_id = bucket_id;
> > + }
> > +}
> > +
> > static int cpu_util_min_write_u64(struct cgroup_subsys_state *css,
> > struct cftype *cftype, u64 min_value)
> > {
[...]
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 12/15] sched/core: uclamp: Propagate parent clamps
From: Suren Baghdasaryan @ 2019-03-18 16:58 UTC (permalink / raw)
To: Patrick Bellasi
Cc: LKML, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle
In-Reply-To: <20190318165430.n222vuq4tv3ntbod@e110439-lin>
On Mon, Mar 18, 2019 at 9:54 AM Patrick Bellasi <patrick.bellasi@arm.com> wrote:
>
> On 14-Mar 09:17, Suren Baghdasaryan wrote:
> > On Fri, Feb 8, 2019 at 2:06 AM Patrick Bellasi <patrick.bellasi@arm.com> wrote:
> > >
> > > In order to properly support hierarchical resources control, the cgroup
> > > delegation model requires that attribute writes from a child group never
> > > fail but still are (potentially) constrained based on parent's assigned
> > > resources. This requires to properly propagate and aggregate parent
> > > attributes down to its descendants.
> > >
> > > Let's implement this mechanism by adding a new "effective" clamp value
> > > for each task group. The effective clamp value is defined as the smaller
> > > value between the clamp value of a group and the effective clamp value
> > > of its parent. This is the actual clamp value enforced on tasks in a
> > > task group.
> >
> > In patch 10 in this series you mentioned "b) do not enforce any
> > constraints and/or dependencies between the parent and its child
> > nodes"
> >
> > This patch seems to change that behavior. If so, should it be documented?
>
> Not, I actually have to update the changelog of that patch.
>
> What I mean is that we do not enforce constraints among "requested"
> values thus ensuring that each sub-group can always request a clamp
> value.
> Of course, if it gets that value or not depends on parent constraints,
> which are propagated down the hierarchy under the form of "effective"
> values by cpu_util_update_heir()
>
> I'll fix the changelog in patch 10 which seems to be confusing for
> Tejun too.
>
> [...]
>
> > > @@ -7011,6 +7029,53 @@ static void cpu_cgroup_attach(struct cgroup_taskset *tset)
> > > }
> > >
> > > #ifdef CONFIG_UCLAMP_TASK_GROUP
> > > +static void cpu_util_update_hier(struct cgroup_subsys_state *css,
> >
> > s/cpu_util_update_hier/cpu_util_update_heir ?
>
> Mmm... why?
>
> That "_hier" stands for "hierarchical".
Yeah, I realized that later on but did not want to create more
chatter. _hier seems fine.
> However, since there we update the effective values, maybe I can
> better rename it in "_eff" ?
>
> > > + unsigned int clamp_id, unsigned int bucket_id,
> > > + unsigned int value)
> > > +{
> > > + struct cgroup_subsys_state *top_css = css;
> > > + struct uclamp_se *uc_se, *uc_parent;
> > > +
> > > + css_for_each_descendant_pre(css, top_css) {
> > > + /*
> > > + * The first visited task group is top_css, which clamp value
> > > + * is the one passed as parameter. For descendent task
> > > + * groups we consider their current value.
> > > + */
> > > + uc_se = &css_tg(css)->uclamp[clamp_id];
> > > + if (css != top_css) {
> > > + value = uc_se->value;
> > > + bucket_id = uc_se->effective.bucket_id;
> > > + }
> > > + uc_parent = NULL;
> > > + if (css_tg(css)->parent)
> > > + uc_parent = &css_tg(css)->parent->uclamp[clamp_id];
> > > +
> > > + /*
> > > + * Skip the whole subtrees if the current effective clamp is
> > > + * already matching the TG's clamp value.
> > > + * In this case, all the subtrees already have top_value, or a
> > > + * more restrictive value, as effective clamp.
> > > + */
> > > + if (uc_se->effective.value == value &&
> > > + uc_parent && uc_parent->effective.value >= value) {
> > > + css = css_rightmost_descendant(css);
> > > + continue;
> > > + }
> > > +
> > > + /* Propagate the most restrictive effective value */
> > > + if (uc_parent && uc_parent->effective.value < value) {
> > > + value = uc_parent->effective.value;
> > > + bucket_id = uc_parent->effective.bucket_id;
> > > + }
> > > + if (uc_se->effective.value == value)
> > > + continue;
> > > +
> > > + uc_se->effective.value = value;
> > > + uc_se->effective.bucket_id = bucket_id;
> > > + }
> > > +}
> > > +
> > > static int cpu_util_min_write_u64(struct cgroup_subsys_state *css,
> > > struct cftype *cftype, u64 min_value)
> > > {
>
> [...]
>
> Cheers,
> Patrick
>
> --
> #include <best/regards.h>
>
> Patrick Bellasi
^ permalink raw reply
* Re: [PATCH ghak90 V5 03/10] audit: read container ID of a process
From: Richard Guy Briggs @ 2019-03-18 18:17 UTC (permalink / raw)
To: Neil Horman
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <20190318111003.GA7364@hmswarspite.think-freely.org>
On 2019-03-18 07:10, Neil Horman wrote:
> On Fri, Mar 15, 2019 at 02:29:51PM -0400, Richard Guy Briggs wrote:
> > Add support for reading the audit container identifier from the proc
> > filesystem.
> >
> > This is a read from the proc entry of the form
> > /proc/PID/audit_containerid where PID is the process ID of the task
> > whose audit container identifier is sought.
> >
> > The read expects up to a u64 value (unset: 18446744073709551615).
> >
> > This read requires CAP_AUDIT_CONTROL.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > ---
> > fs/proc/base.c | 23 +++++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 2505c46c8701..0b833cbdf5b6 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> > .llseek = generic_file_llseek,
> > };
> >
> > +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct inode *inode = file_inode(file);
> > + struct task_struct *task = get_proc_task(inode);
> > + ssize_t length;
> > + char tmpbuf[TMPBUFLEN*2];
> > +
> Sorry, didn't notice this previously, but..
> Why *2 here? Its not wrong per-se, but would it be better to just change
> TMPBUFLEN to be 22 bytes unilaterally? Its only ever used on stack calls that
> arent that deep, and then you won't have to think about adjusting this call site
> if you ever change the value of TMPBUFLEN in the future.
TMPBUFLEN is 11 to accomodate a decimal representation of a u32 with
terminating NULL. Since the contid is a u64, it was least disruptive
and made sense to me to just double it. I could define a TMPBUFLEN2 to
be 21 if you prefer?
> I'm fine with doing this in another patch later, but it seems like a worthwhile
> cleanup
>
> functionality looks good beyond that nit.
>
> > + if (!task)
> > + return -ESRCH;
> > + /* if we don't have caps, reject */
> > + if (!capable(CAP_AUDIT_CONTROL))
> > + return -EPERM;
> > + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> > + put_task_struct(task);
> > + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> > +}
> > +
> > static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> > size_t count, loff_t *ppos)
> > {
> > @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> > }
> >
> > static const struct file_operations proc_contid_operations = {
> > + .read = proc_contid_read,
> > .write = proc_contid_write,
> > .llseek = generic_file_llseek,
> > };
> > @@ -3039,7 +3058,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
> > #ifdef CONFIG_AUDIT
> > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3428,7 +3447,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> > #ifdef CONFIG_AUDIT
> > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > --
> > 1.8.3.1
> >
> >
> Acked-by: Neil Horman <nhorman@tuxdriver.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 03/10] audit: read container ID of a process
From: Neil Horman @ 2019-03-18 18:48 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <20190318181721.6e5jxqwvmifduzvq@madcap2.tricolour.ca>
On Mon, Mar 18, 2019 at 02:17:21PM -0400, Richard Guy Briggs wrote:
> On 2019-03-18 07:10, Neil Horman wrote:
> > On Fri, Mar 15, 2019 at 02:29:51PM -0400, Richard Guy Briggs wrote:
> > > Add support for reading the audit container identifier from the proc
> > > filesystem.
> > >
> > > This is a read from the proc entry of the form
> > > /proc/PID/audit_containerid where PID is the process ID of the task
> > > whose audit container identifier is sought.
> > >
> > > The read expects up to a u64 value (unset: 18446744073709551615).
> > >
> > > This read requires CAP_AUDIT_CONTROL.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > ---
> > > fs/proc/base.c | 23 +++++++++++++++++++++--
> > > 1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > index 2505c46c8701..0b833cbdf5b6 100644
> > > --- a/fs/proc/base.c
> > > +++ b/fs/proc/base.c
> > > @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> > > .llseek = generic_file_llseek,
> > > };
> > >
> > > +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + struct inode *inode = file_inode(file);
> > > + struct task_struct *task = get_proc_task(inode);
> > > + ssize_t length;
> > > + char tmpbuf[TMPBUFLEN*2];
> > > +
> > Sorry, didn't notice this previously, but..
> > Why *2 here? Its not wrong per-se, but would it be better to just change
> > TMPBUFLEN to be 22 bytes unilaterally? Its only ever used on stack calls that
> > arent that deep, and then you won't have to think about adjusting this call site
> > if you ever change the value of TMPBUFLEN in the future.
>
> TMPBUFLEN is 11 to accomodate a decimal representation of a u32 with
> terminating NULL. Since the contid is a u64, it was least disruptive
> and made sense to me to just double it. I could define a TMPBUFLEN2 to
> be 21 if you prefer?
>
I'm not adamant on any particular change, just noticing the inconsistency. I
usually write macro buffer sizes to accomodate the largest string I plan to
hold, so it can be used ubiquitously when the overage is small and transiently
allocated, but if you feel like the space would be better conserved a TMPBUFLEN/
TMPBUFLEN2 approach would be fine (or a TMPBUFLENU32 / TMPBUFLENU64 macro set).
Its not anything that needs fixing now, just an observation for clean up at some
future point.
Neil
> > I'm fine with doing this in another patch later, but it seems like a worthwhile
> > cleanup
> >
> > functionality looks good beyond that nit.
> >
> > > + if (!task)
> > > + return -ESRCH;
> > > + /* if we don't have caps, reject */
> > > + if (!capable(CAP_AUDIT_CONTROL))
> > > + return -EPERM;
> > > + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> > > + put_task_struct(task);
> > > + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> > > +}
> > > +
> > > static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> > > size_t count, loff_t *ppos)
> > > {
> > > @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> > > }
> > >
> > > static const struct file_operations proc_contid_operations = {
> > > + .read = proc_contid_read,
> > > .write = proc_contid_write,
> > > .llseek = generic_file_llseek,
> > > };
> > > @@ -3039,7 +3058,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
> > > #ifdef CONFIG_AUDIT
> > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > > #endif
> > > #ifdef CONFIG_FAULT_INJECTION
> > > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > > @@ -3428,7 +3447,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> > > #ifdef CONFIG_AUDIT
> > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > > #endif
> > > #ifdef CONFIG_FAULT_INJECTION
> > > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > > --
> > > 1.8.3.1
> > >
> > >
> > Acked-by: Neil Horman <nhorman@tuxdriver.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 03/10] audit: read container ID of a process
From: Richard Guy Briggs @ 2019-03-18 18:54 UTC (permalink / raw)
To: Neil Horman
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <20190318184856.GA4111@hmswarspite.think-freely.org>
On 2019-03-18 14:48, Neil Horman wrote:
> On Mon, Mar 18, 2019 at 02:17:21PM -0400, Richard Guy Briggs wrote:
> > On 2019-03-18 07:10, Neil Horman wrote:
> > > On Fri, Mar 15, 2019 at 02:29:51PM -0400, Richard Guy Briggs wrote:
> > > > Add support for reading the audit container identifier from the proc
> > > > filesystem.
> > > >
> > > > This is a read from the proc entry of the form
> > > > /proc/PID/audit_containerid where PID is the process ID of the task
> > > > whose audit container identifier is sought.
> > > >
> > > > The read expects up to a u64 value (unset: 18446744073709551615).
> > > >
> > > > This read requires CAP_AUDIT_CONTROL.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > ---
> > > > fs/proc/base.c | 23 +++++++++++++++++++++--
> > > > 1 file changed, 21 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > > > index 2505c46c8701..0b833cbdf5b6 100644
> > > > --- a/fs/proc/base.c
> > > > +++ b/fs/proc/base.c
> > > > @@ -1295,6 +1295,24 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> > > > .llseek = generic_file_llseek,
> > > > };
> > > >
> > > > +static ssize_t proc_contid_read(struct file *file, char __user *buf,
> > > > + size_t count, loff_t *ppos)
> > > > +{
> > > > + struct inode *inode = file_inode(file);
> > > > + struct task_struct *task = get_proc_task(inode);
> > > > + ssize_t length;
> > > > + char tmpbuf[TMPBUFLEN*2];
> > > > +
> > > Sorry, didn't notice this previously, but..
> > > Why *2 here? Its not wrong per-se, but would it be better to just change
> > > TMPBUFLEN to be 22 bytes unilaterally? Its only ever used on stack calls that
> > > arent that deep, and then you won't have to think about adjusting this call site
> > > if you ever change the value of TMPBUFLEN in the future.
> >
> > TMPBUFLEN is 11 to accomodate a decimal representation of a u32 with
> > terminating NULL. Since the contid is a u64, it was least disruptive
> > and made sense to me to just double it. I could define a TMPBUFLEN2 to
> > be 21 if you prefer?
> >
> I'm not adamant on any particular change, just noticing the inconsistency. I
> usually write macro buffer sizes to accomodate the largest string I plan to
> hold, so it can be used ubiquitously when the overage is small and transiently
> allocated, but if you feel like the space would be better conserved a TMPBUFLEN/
> TMPBUFLEN2 approach would be fine (or a TMPBUFLENU32 / TMPBUFLENU64 macro set).
Ok, I see your point about it being transient. I'll fix it up if there
is a respin.
> Its not anything that needs fixing now, just an observation for clean up at some
> future point.
> Neil
>
> > > I'm fine with doing this in another patch later, but it seems like a worthwhile
> > > cleanup
> > >
> > > functionality looks good beyond that nit.
> > >
> > > > + if (!task)
> > > > + return -ESRCH;
> > > > + /* if we don't have caps, reject */
> > > > + if (!capable(CAP_AUDIT_CONTROL))
> > > > + return -EPERM;
> > > > + length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_contid(task));
> > > > + put_task_struct(task);
> > > > + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> > > > +}
> > > > +
> > > > static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> > > > size_t count, loff_t *ppos)
> > > > {
> > > > @@ -1325,6 +1343,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> > > > }
> > > >
> > > > static const struct file_operations proc_contid_operations = {
> > > > + .read = proc_contid_read,
> > > > .write = proc_contid_write,
> > > > .llseek = generic_file_llseek,
> > > > };
> > > > @@ -3039,7 +3058,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
> > > > #ifdef CONFIG_AUDIT
> > > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > > > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > > > #endif
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > > > @@ -3428,7 +3447,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> > > > #ifdef CONFIG_AUDIT
> > > > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > > > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > > > - REG("audit_containerid", S_IWUSR, proc_contid_operations),
> > > > + REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> > > > #endif
> > > > #ifdef CONFIG_FAULT_INJECTION
> > > > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > > > --
> > > > 1.8.3.1
> > > >
> > > Acked-by: Neil Horman <nhorman@tuxdriver.com>
> >
> > - RGB
- RGB
^ permalink raw reply
* Re: [PATCH ghak90 V5 05/10] audit: add containerid support for ptrace and signals
From: Neil Horman @ 2019-03-18 19:04 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <a5a612bff629574cf4b678e7c7a9315e0e60a27c.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:53PM -0400, Richard Guy Briggs 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;
> };
Sorry, just noticed this. How does this work? Given that ctx[] is a variable
length array, one assumes that the receiver of this message (userspace
applications by the looks of it, presume that the ctx data occupies the skb from
the byte following pid to the end of the transmitted buffer. How are they to
know that the last byte is actually the cid value? Wouldn't it be better to
move cid above ctx[0], so that the semantics of the variable length data are
preserved?
Or am I missing something?
otherwise this looks ok to me.
Neil
>
> 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
>
>
^ permalink raw reply
* Re: [PATCH ghak90 V5 05/10] audit: add containerid support for ptrace and signals
From: Richard Guy Briggs @ 2019-03-18 19:29 UTC (permalink / raw)
To: Neil Horman
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <20190318190415.GB4111@hmswarspite.think-freely.org>
On 2019-03-18 15:04, Neil Horman wrote:
> On Fri, Mar 15, 2019 at 02:29:53PM -0400, Richard Guy Briggs 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;
> > };
> Sorry, just noticed this. How does this work? Given that ctx[] is a variable
> length array, one assumes that the receiver of this message (userspace
> applications by the looks of it, presume that the ctx data occupies the skb from
> the byte following pid to the end of the transmitted buffer. How are they to
> know that the last byte is actually the cid value? Wouldn't it be better to
> move cid above ctx[0], so that the semantics of the variable length data are
> preserved?
>
> Or am I missing something?
Nope you're not missing anything, but I am! That's a bug. Thanks for
spotting that!
> otherwise this looks ok to me.
> Neil
>
> >
> > 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
> >
> >
- RGB
^ permalink raw reply
* Re: [PATCH ghak90 V5 06/10] audit: add support for non-syscall auxiliary records
From: Neil Horman @ 2019-03-18 19:34 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <700e4313571f86cc0cdcb7310fbe060f02c7c2db.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:54PM -0400, Richard Guy Briggs wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> include/linux/audit.h | 8 ++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 35 ++++++++++++++++++++++++++++++-----
> 3 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index ebd6625ca80e..6db5aba7cc01 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -285,6 +285,8 @@ static inline void audit_log_contid(struct audit_context *context, u64 contid)
>
> /* These are defined in auditsc.c */
> /* Public API */
> +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> +extern void audit_free_context(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -512,6 +514,12 @@ static inline void audit_fanotify(unsigned int response)
> extern int audit_n_rules;
> extern int audit_signals;
> #else /* CONFIG_AUDITSYSCALL */
> +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> +{
> + return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
> static inline void audit_syscall_entry(int major, unsigned long a0,
> unsigned long a1, unsigned long a2,
> unsigned long a3)
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c5ac6436317e..2a1a8b8a8019 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -111,6 +111,7 @@ struct audit_proctitle {
> struct audit_context {
> int dummy; /* must be the first element */
> int in_syscall; /* 1 if task is in a syscall */
> + bool local; /* local context needed */
> enum audit_state state, current_state;
> unsigned int serial; /* serial number for record */
> int major; /* syscall number */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f04e115df5dc..aa5d13b4fbbb 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -872,11 +872,13 @@ static inline void audit_free_aux(struct audit_context *context)
> }
> }
>
> -static inline struct audit_context *audit_alloc_context(enum audit_state state)
> +static inline struct audit_context *audit_alloc_context(enum audit_state state,
> + gfp_t gfpflags)
> {
> struct audit_context *context;
>
> - context = kzalloc(sizeof(*context), GFP_KERNEL);
> + /* We can be called in atomic context via audit_tg() */
> + context = kzalloc(sizeof(*context), gfpflags);
> if (!context)
> return NULL;
> context->state = state;
> @@ -912,7 +914,8 @@ int audit_alloc_syscall(struct task_struct *tsk)
> return 0;
> }
>
> - if (!(context = audit_alloc_context(state))) {
> + context = audit_alloc_context(state, GFP_KERNEL);
> + if (!context) {
> kfree(key);
> audit_log_lost("out of memory in audit_alloc_syscall");
> return -ENOMEM;
> @@ -924,8 +927,29 @@ int audit_alloc_syscall(struct task_struct *tsk)
> return 0;
> }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(gfp_t gfpflags)
> {
> + struct audit_context *context = NULL;
> +
> + if (!audit_ever_enabled)
> + goto out; /* Return if not auditing. */
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags);
> + if (!context) {
> + audit_log_lost("out of memory in audit_alloc_local");
> + goto out;
> + }
> + context->serial = audit_serial();
> + ktime_get_coarse_real_ts64(&context->ctime);
> + context->local = true;
> +out:
> + return context;
> +}
> +EXPORT_SYMBOL(audit_alloc_local);
> +
> +void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> @@ -935,6 +959,7 @@ static inline void audit_free_context(struct audit_context *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int sessionid,
> @@ -2163,7 +2188,7 @@ void __audit_inode_child(struct inode *parent,
> int auditsc_get_stamp(struct audit_context *ctx,
> struct timespec64 *t, unsigned int *serial)
> {
> - if (!ctx->in_syscall)
> + if (!ctx->in_syscall && !ctx->local)
> return 0;
> if (!ctx->serial)
> ctx->serial = audit_serial();
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH ghak90 V5 07/10] audit: add containerid support for user records
From: Neil Horman @ 2019-03-18 19:41 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <e724de2037db0fd72fef7154c83615a742417d31.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:55PM -0400, Richard Guy Briggs wrote:
> Add audit container identifier auxiliary record to user event standalone
> records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index cfa659b3f6c4..cf448599ef34 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1142,12 +1142,6 @@ static void audit_log_common_recv_msg(struct audit_context *context,
> audit_log_task_context(*ab);
> }
>
> -static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
> - u16 msg_type)
> -{
> - audit_log_common_recv_msg(NULL, ab, msg_type);
> -}
> -
> int is_audit_feature_set(int i)
> {
> return af.features & AUDIT_FEATURE_TO_MASK(i);
> @@ -1409,13 +1403,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>
> err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> + struct audit_context *context;
> +
> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> err = tty_audit_push();
> if (err)
> break;
> }
> - audit_log_user_recv_msg(&ab, msg_type);
> + context = audit_alloc_local(GFP_KERNEL);
> + audit_log_common_recv_msg(context, &ab, msg_type);
> if (msg_type != AUDIT_USER_TTY)
> audit_log_format(ab, " msg='%.*s'",
> AUDIT_MESSAGE_TEXT_MAX,
> @@ -1431,6 +1428,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> audit_log_n_untrustedstring(ab, data, size);
> }
> audit_log_end(ab);
> + audit_log_contid(context, audit_get_contid(current));
> + audit_free_context(context);
> }
> break;
> case AUDIT_ADD_RULE:
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
From: Ondrej Mosnacek @ 2019-03-18 20:02 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, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <3f02f7b56d2fba2918ff6fe90fcfa3ae558faff8.1552665316.git.rgb@redhat.com>
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().
Also, doesn't this loop interfere with the outer loop that also uses
'i' as the control variable?
> + 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.
^ 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