* Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
From: Paul Moore @ 2020-01-23 17:09 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <20200123162918.b3jbed7tbvr2sf2p@madcap2.tricolour.ca>
On Thu, Jan 23, 2020 at 11:29 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-01-22 16:28, Paul Moore wrote:
> > On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > Add audit container identifier support to the action of signalling the
> > > audit daemon.
> > >
> > > Since this would need to add an element to the audit_sig_info struct,
> > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > audit_sig_info2 struct. Corresponding support is required in the
> > > userspace code to reflect the new record request and reply type.
> > > An older userspace won't break since it won't know to request this
> > > record type.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > include/linux/audit.h | 7 +++++++
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++
> > > kernel/audit.h | 1 +
> > > security/selinux/nlmsgtab.c | 1 +
> > > 5 files changed, 45 insertions(+)
> >
> > ...
> >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 0871c3e5d6df..51159c94041c 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -126,6 +126,14 @@ struct auditd_connection {
> > > kuid_t audit_sig_uid = INVALID_UID;
> > > pid_t audit_sig_pid = -1;
> > > u32 audit_sig_sid = 0;
> > > +/* Since the signal information is stored in the record buffer at the
> > > + * time of the signal, but not retrieved until later, there is a chance
> > > + * that the last process in the container could terminate before the
> > > + * signal record is delivered. In this circumstance, there is a chance
> > > + * the orchestrator could reuse the audit container identifier, causing
> > > + * an overlap of audit records that refer to the same audit container
> > > + * identifier, but a different container instance. */
> > > +u64 audit_sig_cid = AUDIT_CID_UNSET;
> >
> > I believe we could prevent the case mentioned above by taking an
> > additional reference to the audit container ID object when the signal
> > information is collected, dropping it only after the signal
> > information is collected by userspace or another process signals the
> > audit daemon. Yes, it would block that audit container ID from being
> > reused immediately, but since we are talking about one number out of
> > 2^64 that seems like a reasonable tradeoff.
>
> I had thought that through and should have been more explicit about that
> situation when I documented it. We could do that, but then the syscall
> records would be connected with the call from auditd on shutdown to
> request that signal information, rather than the exit of that last
> process that was using that container. This strikes me as misleading.
> Is that really what we want?
???
I think one of us is not understanding the other; maybe it's me, maybe
it's you, maybe it's both of us.
Anyway, here is what I was trying to convey with my original comment
... When we record the audit container ID in audit_signal_info() we
take an extra reference to the audit container ID object so that it
will not disappear (and get reused) until after we respond with an
AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the
AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in
audit_signal_info(). Unless I'm missing some other change you made,
this *shouldn't* affect the syscall records, all it does is preserve
the audit container ID object in the kernel's ACID store so it doesn't
get reused.
(We do need to do some extra housekeeping in audit_signal_info() to
deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 -
basically if audit_sig_cid is not NULL we should drop a reference
before assigning it a new object pointer, and of course we would need
to set audit_sig_cid to NULL in audit_receive_msg() after sending it
up to userspace and dropping the extra ref.)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
From: Richard Guy Briggs @ 2020-01-23 16:29 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <CAHC9VhSuwJGryfrBfzxG01zwb-O_7dbjS0x0a3w-XjcNuYSAcg@mail.gmail.com>
On 2020-01-22 16:28, Paul Moore wrote:
> On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Add audit container identifier support to the action of signalling the
> > audit daemon.
> >
> > Since this would need to add an element to the audit_sig_info struct,
> > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > audit_sig_info2 struct. Corresponding support is required in the
> > userspace code to reflect the new record request and reply type.
> > An older userspace won't break since it won't know to request this
> > record type.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 7 +++++++
> > include/uapi/linux/audit.h | 1 +
> > kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++
> > kernel/audit.h | 1 +
> > security/selinux/nlmsgtab.c | 1 +
> > 5 files changed, 45 insertions(+)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 0871c3e5d6df..51159c94041c 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -126,6 +126,14 @@ struct auditd_connection {
> > kuid_t audit_sig_uid = INVALID_UID;
> > pid_t audit_sig_pid = -1;
> > u32 audit_sig_sid = 0;
> > +/* Since the signal information is stored in the record buffer at the
> > + * time of the signal, but not retrieved until later, there is a chance
> > + * that the last process in the container could terminate before the
> > + * signal record is delivered. In this circumstance, there is a chance
> > + * the orchestrator could reuse the audit container identifier, causing
> > + * an overlap of audit records that refer to the same audit container
> > + * identifier, but a different container instance. */
> > +u64 audit_sig_cid = AUDIT_CID_UNSET;
>
> I believe we could prevent the case mentioned above by taking an
> additional reference to the audit container ID object when the signal
> information is collected, dropping it only after the signal
> information is collected by userspace or another process signals the
> audit daemon. Yes, it would block that audit container ID from being
> reused immediately, but since we are talking about one number out of
> 2^64 that seems like a reasonable tradeoff.
I had thought that through and should have been more explicit about that
situation when I documented it. We could do that, but then the syscall
records would be connected with the call from auditd on shutdown to
request that signal information, rather than the exit of that last
process that was using that container. This strikes me as misleading.
Is that really what we want?
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Michal Hocko @ 2020-01-23 9:13 UTC (permalink / raw)
To: Minchan Kim
Cc: sspatil, kirill, akpm, linux-kernel, linux-mm, linux-api,
oleksandr, surenb, timmurray, dancol, sonnyrao, bgeffon, hannes,
shakeelb, joaodias, ktkhai, christian.brauner, sjpark
In-Reply-To: <20200123014131.GA249784@google.com>
On Wed 22-01-20 17:41:31, Minchan Kim wrote:
[...]
> What do you want to see further?
Either a consensus that it is sufficient to have an inherently racy
interface that requires some form of external sychronization or
a robust interface that can cope with races in a sensible way.
And no, having a flag for future extension is definitely not the
way how a new API should be added. Seriously!
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Florian Weimer @ 2020-01-23 8:19 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Mathieu Desnoyers, Chris Lameter, Jann Horn, Peter Zijlstra,
Thomas Gleixner, linux-kernel, Joel Fernandes, Ingo Molnar,
Catalin Marinas, Dave Watson, Will Deacon, shuah, Andi Kleen,
linux-kselftest, Russell King, Michael Kerrisk, Paul, Paul Turner,
Boqun Feng, Josh Triplett, rostedt <rosted>
In-Reply-To: <ead7a565-9a23-a7d7-904d-c4860f63952a-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
* H. Peter Anvin:
> On 2020-01-21 17:11, Mathieu Desnoyers wrote:
>> ----- On Jan 21, 2020, at 4:44 PM, Chris Lameter cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org wrote:
>>
>>> These scenarios are all pretty complex and will be difficult to understand
>>> for the user of these APIs.
>>>
>>> I think the easiest solution (and most comprehensible) is for the user
>>> space process that does per cpu operations to get some sort of signal. If
>>> its not able to handle that then terminate it. The code makes a basic
>>> assumption after all that the process is running on a specific cpu. If
>>> this is no longer the case then its better to abort if the process cannot
>>> handle moving to a different processor.
>>
>> The point of pin_on_cpu() is to allow threads to access per-cpu data
>> structures belonging to a given CPU even if they cannot run on that
>> CPU (because it is offline).
>>
>> I am not sure what scenario your signal delivery proposal aims to cover.
>>
>> Just to try to put this into the context of a specific scenario to see
>> if I understand your point, is the following what you have in mind ?
>>
>> 1. Thread A issues pin_on_cpu(5),
>> 2. Thread B issues sched_setaffinity removing cpu 5 from thread A's
>> affinity mask,
>> 3. Noticing that it would generate an invalid combination, rather than
>> failing sched_setaffinity, it would send a SIGSEGV (or other) signal
>> to thread A.
>>
>> Or so you have something entirely different in mind ?
>>
>
> I would agree that this seems like the only sane option, or you will
> be in a world of hurt because of conflicting semantics. It is not just
> offlining, but what happens if a policy manager calls
> sched_setaffinity() on another thread -- and now the universe breaks
> because a library is updated to use this new system call which
> collides with the expectations of the policy manager.
Yes, this new interface seems fundamentally incompatible with how
affinity masks are changed today.
Would it be possible to make pin_on_cpu_set to use fallback
synchronization via a futex if the CPU cannot be acquired by running on
it? The rseq section would need to check the futex as well, but would
not have to acquire it.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: H. Peter Anvin @ 2020-01-23 7:53 UTC (permalink / raw)
To: Mathieu Desnoyers, Chris Lameter
Cc: Jann Horn, Peter Zijlstra, Thomas Gleixner, linux-kernel,
Joel Fernandes, Ingo Molnar, Catalin Marinas, Dave Watson,
Will Deacon, shuah, Andi Kleen, linux-kselftest, Russell King,
Michael Kerrisk, Paul, Paul Turner, Boqun Feng, Josh Triplett,
rostedt, Ben Maurer, linux-api
In-Reply-To: <1648013936.596672.1579655468604.JavaMail.zimbra-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org>
On 2020-01-21 17:11, Mathieu Desnoyers wrote:
> ----- On Jan 21, 2020, at 4:44 PM, Chris Lameter cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org wrote:
>
>> These scenarios are all pretty complex and will be difficult to understand
>> for the user of these APIs.
>>
>> I think the easiest solution (and most comprehensible) is for the user
>> space process that does per cpu operations to get some sort of signal. If
>> its not able to handle that then terminate it. The code makes a basic
>> assumption after all that the process is running on a specific cpu. If
>> this is no longer the case then its better to abort if the process cannot
>> handle moving to a different processor.
>
> The point of pin_on_cpu() is to allow threads to access per-cpu data
> structures belonging to a given CPU even if they cannot run on that
> CPU (because it is offline).
>
> I am not sure what scenario your signal delivery proposal aims to cover.
>
> Just to try to put this into the context of a specific scenario to see
> if I understand your point, is the following what you have in mind ?
>
> 1. Thread A issues pin_on_cpu(5),
> 2. Thread B issues sched_setaffinity removing cpu 5 from thread A's
> affinity mask,
> 3. Noticing that it would generate an invalid combination, rather than
> failing sched_setaffinity, it would send a SIGSEGV (or other) signal
> to thread A.
>
> Or so you have something entirely different in mind ?
>
I would agree that this seems like the only sane option, or you will be in a
world of hurt because of conflicting semantics. It is not just offlining, but
what happens if a policy manager calls sched_setaffinity() on another thread
-- and now the universe breaks because a library is updated to use this new
system call which collides with the expectations of the policy manager.
There doesn't seem to be any way to get this to be a local event which doesn't
break assumptions elsewhere in the system without making this an abort event
of some type. However, signals are painful in their own right, mostly because
of the lack of any infrastructure for allocating signals to libraries in user
space. I was actually thinking about exactly that issue just this weekend.
-hpa
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Oleksandr Natalenko @ 2020-01-23 7:29 UTC (permalink / raw)
To: Minchan Kim
Cc: Kirill A. Shutemov, Michal Hocko, Andrew Morton, LKML, linux-mm,
linux-api, Suren Baghdasaryan, Tim Murray, Daniel Colascione,
Sandeep Patil, Sonny Rao, Brian Geffon, Johannes Weiner,
Shakeel Butt, John Dias, ktkhai, christian.brauner, sjpark
In-Reply-To: <20200123014316.GB249784@google.com>
On Wed, Jan 22, 2020 at 05:43:16PM -0800, Minchan Kim wrote:
> > It seems I've overlooked an important piece of this submission: one
> > cannot apply the hint to all the anonymous mapping regardless of address
> > range. For KSM I'd rather either have a possibility to hint all the
> > anonymous mappings, or, as it was suggested previously, be able to iterate
> > over existing mappings using some (fd-based?) API.
>
> Thing is how you could identify a certan range is better for KSM than
> others from external process?
I think the info like this is kinda available via /proc/pid/smaps. It
lists the ranges and the vmflags. But using it raises 2 concerns: one is
the absence of guarantee the mappings won't change after smaps is read
and the second one is that there's no separate vmflag for marking a vma
as non-meregable (and IIRC from previous attempts on addressing this,
we've already exhausted all the flags on 32-bit arches, so it is not
something that can be trivially addressed).
--
Best regards,
Oleksandr Natalenko (post-factum)
Senior Software Maintenance Engineer
^ permalink raw reply
* Re: [PATCH] mm: Add MREMAP_DONTUNMAP to mremap().
From: Andy Lutomirski @ 2020-01-23 3:02 UTC (permalink / raw)
To: Brian Geffon
Cc: linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Andrew Morton,
Michael S . Tsirkin, Arnd Bergmann, Sonny Rao, Minchan Kim,
Joel Fernandes, Lokesh Gidra, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Yu Zhao, Jesse Barnes
In-Reply-To: <20200123014627.71720-1-bgeffon-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> On Jan 22, 2020, at 5:46 PM, Brian Geffon <bgeffon-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> MREMAP_DONTUNMAP is an additional flag that can be used with
> MREMAP_FIXED to move a mapping to a new address. Normally, mremap(2)
> would then tear down the old vma so subsequent accesses to the vma
> cause a segfault. However, with this new flag it will keep the old
> vma with zapping PTEs so any access to the old VMA after that point
> will result in a pagefault.
This needs a vastly better description. Perhaps:
When remapping an anonymous, private mapping, if MREMAP_DONTUNMAP is set, the source mapping will not be removed. Instead it will be cleared as if a brand new anonymous, private mapping had been created atomically as part of the mremap() call. If a userfaultfd was watching the source, it will continue to watch the new mapping. For a mapping that is shared or not anonymous, MREMAP_DONTUNMAP will cause the mremap() call to fail.
Or is it something else?
>
> This feature will find a use in ChromeOS along with userfaultfd.
> Specifically we will want to register a VMA with userfaultfd and then
> pull it out from under a running process. By using MREMAP_DONTUNMAP we
> don't have to worry about mprotecting and then potentially racing with
> VMA permission changes from a running process.
Does this mean you yank it out but you want to replace it simultaneously?
>
> This feature also has a use case in Android, Lokesh Gidra has said
> that "As part of using userfaultfd for GC, We'll have to move the physical
> pages of the java heap to a separate location. For this purpose mremap
> will be used. Without the MREMAP_DONTUNMAP flag, when I mremap the java
> heap, its virtual mapping will be removed as well. Therefore, we'll
> require performing mmap immediately after. This is not only time consuming
> but also opens a time window where a native thread may call mmap and
> reserve the java heap's address range for its own usage. This flag
> solves the problem."
Cute.
^ permalink raw reply
* Re: [RESEND Patch v6 2/2] fpga: dfl: fme: add performance reporting support
From: Wu Hao @ 2020-01-23 2:05 UTC (permalink / raw)
To: Greg KH
Cc: will-DgEjT+Ai2ygdnm+yROfE0A, mdf-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, linux-fpga-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, atull-DgEjT+Ai2ygdnm+yROfE0A,
Luwei Kang, Xu Yilun
In-Reply-To: <20200122141026.GA10399-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Wed, Jan 22, 2020 at 03:10:26PM +0100, Greg KH wrote:
> On Fri, Jan 17, 2020 at 11:10:28AM +0800, Wu Hao wrote:
> > +static const struct attribute_group fme_perf_fabric_events_group = {
> > + .name = "events",
> > + .attrs = fme_perf_fabric_events_attrs,
> > + .is_visible = fme_perf_fabric_events_visible,
> > +};
> > +
>
> I don't see any Documentation/ABI/ entries for all of these new files.
> Please properly document them so we know how to review this.
Hi Greg,
Thanks a lot for the review. Actually all the sysfs entries under events
are perf monitoring events following the same format documented in
"Documentation/ABI/testing/sysfs-bus-event_source-devices-events" and
we added some description in our fpga documentation in patch #1.
Yes, I can add something in ABI directly as well in the next version.
I pasted related descriptions below. I hope this won't block review.
Thank you very much!
Hao
>From Documentation/ABI/testing/sysfs-bus-event_source-devices-events
What: /sys/bus/event_source/devices/<pmu>/events/<event>
Date: 2014/02/24
Contact: Linux kernel mailing list <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Description: Per-pmu performance monitoring events specific to the running system
Each file (except for some of those with a '.' in them, '.unit'
and '.scale') in the 'events' directory describes a single
performance monitoring event supported by the <pmu>. The name
of the file is the name of the event.
File contents:
<term>[=<value>][,<term>[=<value>]]...
Where <term> is one of the terms listed under
/sys/bus/event_source/devices/<pmu>/format/ and <value> is
a number is base-16 format with a '0x' prefix (lowercase only).
If a <term> is specified alone (without an assigned value), it
is implied that 0x1 is assigned to that <term>.
Examples (each of these lines would be in a seperate file):
event=0x2abc
event=0x423,inv,cmask=0x3
domain=0x1,offset=0x8,starting_index=0xffff
domain=0x1,offset=0x8,core=?
Each of the assignments indicates a value to be assigned to a
particular set of bits (as defined by the format file
corresponding to the <term>) in the perf_event structure passed
to the perf_open syscall.
In the case of the last example, a value replacing "?" would
need to be provided by the user selecting the particular event.
This is referred to as "event parameterization". Event
parameters have the format 'param=?'.
>From Documentation/fpga/dfl.rst added by patch #1.
"
The "events" directory describes the configuration templates for all available
events which can be used with perf tool directly. For example, fab_mmio_read
has the configuration "event=0x06,evtype=0x02,portid=0xff", which shows this
event belongs to fabric type (0x02), the local event id is 0x06 and it is for
overall monitoring (portid=0xff).
"
>
> thanks,
>
> greg k-h
^ permalink raw reply
* [PATCH] mm: Add MREMAP_DONTUNMAP to mremap().
From: Brian Geffon @ 2020-01-23 1:46 UTC (permalink / raw)
To: linux-mm-Bw31MaZKKs3YtjvyW6yDsg
Cc: Andrew Morton, Michael S . Tsirkin, Arnd Bergmann, Brian Geffon,
Sonny Rao, Minchan Kim, Joel Fernandes, Lokesh Gidra,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Yu Zhao, Jesse Barnes
MREMAP_DONTUNMAP is an additional flag that can be used with
MREMAP_FIXED to move a mapping to a new address. Normally, mremap(2)
would then tear down the old vma so subsequent accesses to the vma
cause a segfault. However, with this new flag it will keep the old
vma with zapping PTEs so any access to the old VMA after that point
will result in a pagefault.
This feature will find a use in ChromeOS along with userfaultfd.
Specifically we will want to register a VMA with userfaultfd and then
pull it out from under a running process. By using MREMAP_DONTUNMAP we
don't have to worry about mprotecting and then potentially racing with
VMA permission changes from a running process.
This feature also has a use case in Android, Lokesh Gidra has said
that "As part of using userfaultfd for GC, We'll have to move the physical
pages of the java heap to a separate location. For this purpose mremap
will be used. Without the MREMAP_DONTUNMAP flag, when I mremap the java
heap, its virtual mapping will be removed as well. Therefore, we'll
require performing mmap immediately after. This is not only time consuming
but also opens a time window where a native thread may call mmap and
reserve the java heap's address range for its own usage. This flag
solves the problem."
Signed-off-by: Brian Geffon <bgeffon-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
include/uapi/linux/mman.h | 5 +++--
mm/mremap.c | 37 ++++++++++++++++++++++++++++++-------
2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
index fc1a64c3447b..923cc162609c 100644
--- a/include/uapi/linux/mman.h
+++ b/include/uapi/linux/mman.h
@@ -5,8 +5,9 @@
#include <asm/mman.h>
#include <asm-generic/hugetlb_encode.h>
-#define MREMAP_MAYMOVE 1
-#define MREMAP_FIXED 2
+#define MREMAP_MAYMOVE 1
+#define MREMAP_FIXED 2
+#define MREMAP_DONTUNMAP 4
#define OVERCOMMIT_GUESS 0
#define OVERCOMMIT_ALWAYS 1
diff --git a/mm/mremap.c b/mm/mremap.c
index 122938dcec15..bf97c3eb538b 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -318,8 +318,8 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
static unsigned long move_vma(struct vm_area_struct *vma,
unsigned long old_addr, unsigned long old_len,
unsigned long new_len, unsigned long new_addr,
- bool *locked, struct vm_userfaultfd_ctx *uf,
- struct list_head *uf_unmap)
+ bool *locked, unsigned long flags,
+ struct vm_userfaultfd_ctx *uf, struct list_head *uf_unmap)
{
struct mm_struct *mm = vma->vm_mm;
struct vm_area_struct *new_vma;
@@ -408,6 +408,13 @@ static unsigned long move_vma(struct vm_area_struct *vma,
if (unlikely(vma->vm_flags & VM_PFNMAP))
untrack_pfn_moved(vma);
+ if (unlikely(!err && (flags & MREMAP_DONTUNMAP))) {
+ if (vm_flags & VM_ACCOUNT)
+ vma->vm_flags |= VM_ACCOUNT;
+
+ goto out;
+ }
+
if (do_munmap(mm, old_addr, old_len, uf_unmap) < 0) {
/* OOM: unable to split vma, just get accounts right */
vm_unacct_memory(excess >> PAGE_SHIFT);
@@ -422,6 +429,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
vma->vm_next->vm_flags |= VM_ACCOUNT;
}
+out:
if (vm_flags & VM_LOCKED) {
mm->locked_vm += new_len >> PAGE_SHIFT;
*locked = true;
@@ -497,7 +505,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
unsigned long new_addr, unsigned long new_len, bool *locked,
- struct vm_userfaultfd_ctx *uf,
+ unsigned long flags, struct vm_userfaultfd_ctx *uf,
struct list_head *uf_unmap_early,
struct list_head *uf_unmap)
{
@@ -545,6 +553,17 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
old_len = new_len;
}
+ /*
+ * MREMAP_DONTUNMAP expands by old_len + (new_len - old_len), we will
+ * check that we can expand by old_len and vma_to_resize will handle
+ * the vma growing.
+ */
+ if (unlikely(flags & MREMAP_DONTUNMAP && !may_expand_vm(mm,
+ vma->vm_flags, old_len >> PAGE_SHIFT))) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
vma = vma_to_resize(addr, old_len, new_len, &charged);
if (IS_ERR(vma)) {
ret = PTR_ERR(vma);
@@ -561,7 +580,7 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
if (IS_ERR_VALUE(ret))
goto out1;
- ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, uf,
+ ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, flags, uf,
uf_unmap);
if (!(offset_in_page(ret)))
goto out;
@@ -609,12 +628,15 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
addr = untagged_addr(addr);
new_addr = untagged_addr(new_addr);
- if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
+ if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE | MREMAP_DONTUNMAP))
return ret;
if (flags & MREMAP_FIXED && !(flags & MREMAP_MAYMOVE))
return ret;
+ if (flags & MREMAP_DONTUNMAP && !(flags & MREMAP_FIXED))
+ return ret;
+
if (offset_in_page(addr))
return ret;
@@ -634,7 +656,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
if (flags & MREMAP_FIXED) {
ret = mremap_to(addr, old_len, new_addr, new_len,
- &locked, &uf, &uf_unmap_early, &uf_unmap);
+ &locked, flags, &uf, &uf_unmap_early,
+ &uf_unmap);
goto out;
}
@@ -712,7 +735,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
}
ret = move_vma(vma, addr, old_len, new_len, new_addr,
- &locked, &uf, &uf_unmap);
+ &locked, flags, &uf, &uf_unmap);
}
out:
if (offset_in_page(ret)) {
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-23 1:43 UTC (permalink / raw)
To: Oleksandr Natalenko
Cc: Kirill A. Shutemov, Michal Hocko, Andrew Morton, LKML, linux-mm,
linux-api, Suren Baghdasaryan, Tim Murray, Daniel Colascione,
Sandeep Patil, Sonny Rao, Brian Geffon, Johannes Weiner,
Shakeel Butt, John Dias, ktkhai, christian.brauner, sjpark
In-Reply-To: <20200122104424.7gvrfivymjvdous4@butterfly.localdomain>
On Wed, Jan 22, 2020 at 11:44:24AM +0100, Oleksandr Natalenko wrote:
> Hello.
>
> On Tue, Jan 21, 2020 at 10:11:13AM -0800, Minchan Kim wrote:
> > > > I also think people don't want to give an KSM hint to non-mergeable area.
> > >
> > > And how the manager knows which data is mergable?
> >
> > Oleksandr, could you say your thought why you need address range based
> > API?
>
> It seems I've overlooked an important piece of this submission: one
> cannot apply the hint to all the anonymous mapping regardless of address
> range. For KSM I'd rather either have a possibility to hint all the
> anonymous mappings, or, as it was suggested previously, be able to iterate
> over existing mappings using some (fd-based?) API.
Thing is how you could identify a certan range is better for KSM than
others from external process?
^ permalink raw reply
* Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-23 1:41 UTC (permalink / raw)
To: Michal Hocko
Cc: sspatil-hpIqsD4AKlfQT0dZR+AlfA, kirill-oKw7cIdHH8eLwutG50LtGA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
timmurray-hpIqsD4AKlfQT0dZR+AlfA, dancol-hpIqsD4AKlfQT0dZR+AlfA,
sonnyrao-hpIqsD4AKlfQT0dZR+AlfA, bgeffon-hpIqsD4AKlfQT0dZR+AlfA,
hannes-druUgvl0LCNAfugRpC6u6w, shakeelb-hpIqsD4AKlfQT0dZR+AlfA,
joaodias-hpIqsD4AKlfQT0dZR+AlfA, ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200122082853.GS29276-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org>
On Wed, Jan 22, 2020 at 09:28:53AM +0100, Michal Hocko wrote:
> On Tue 21-01-20 10:32:12, Minchan Kim wrote:
> > On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
> [...]
> > > The interface really has to be robust to future potential usecases.
> >
> > I do understand your concern but for me, it's chicken and egg problem.
> > We usually do best effort to make something perfect as far as possible
> > but we also don't do over-engineering without real usecase from the
> > beginning.
> >
> > I already told you how we could synchronize among processes and potential
> > way to be extended Daniel suggested(That's why current API has extra field
> > for the cookie) even though we don't need it right now.
>
> If you can synchronize with the target task then you do not need a
> remote interface. Just use ptrace and you are done with it.
As I mentioned in other reply, we want to do in caller's context, not
callee's one because target processes stay in little cores, which are
much slower than the core the manager lives in.
The other reason is the apps are already freezed so they couldn't response
by ptrace.
>
> > If you want to suggest the other way, please explain why your idea is
> > better and why we need it at this moment.
>
> I believe I have explained my concerns and why they matter. All you are
> saying is that you do not care because your particular usecase doesn't
> care. And that is a first signal of a future disaster when we end up
> with a broken and unfixable interface we have to maintain for ever.
We already had suggested cookie and fd based approaches so I reserved a
argument for that to make the API extendable.
Thing is currently it's a just optimization idea since we have several
ways to sychronize processes(e.g., signal, cgroup freezer, userfaultfd
and so). It's a just matter of granularity, not necessary one we should
introduce it from the beginnig.
If someone needs that kinds of fine-grained consistency, we could extend
it then. And that's the usual way we make progress when we couldn't
know the future.
What do you want to see further?
^ permalink raw reply
* Re: [PATCH ghak90 V8 16/16] audit: add capcontid to set contid outside init_user_ns
From: Paul Moore @ 2020-01-22 21:29 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <5941671b6b6b5de28ab2cc80e72f288cf83291d5.1577736799.git.rgb@redhat.com>
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a
> process in a non-init user namespace the capability to set audit
> container identifiers.
>
> Provide /proc/$PID/audit_capcontid interface to capcontid.
> Valid values are: 1==enabled, 0==disabled
It would be good to be more explicit about "enabled" and "disabled" in
the commit description. For example, which setting allows the target
task to set audit container IDs of it's children processes?
> Report this action in message type AUDIT_SET_CAPCONTID 1022 with fields
> opid= capcontid= old-capcontid=
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/proc/base.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/audit.h | 14 ++++++++++++
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 35 +++++++++++++++++++++++++++++
> 4 files changed, 105 insertions(+)
...
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 26091800180c..283ef8e006e7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1360,6 +1360,59 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
> .write = proc_contid_write,
> .llseek = generic_file_llseek,
> };
> +
> +static ssize_t proc_capcontid_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];
> +
> + if (!task)
> + return -ESRCH;
> + /* if we don't have caps, reject */
> + if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
> + return -EPERM;
> + length = scnprintf(tmpbuf, TMPBUFLEN, "%u", audit_get_capcontid(task));
> + put_task_struct(task);
> + return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> +}
> +
> +static ssize_t proc_capcontid_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + u32 capcontid;
> + int rv;
> + struct task_struct *task = get_proc_task(inode);
> +
> + if (!task)
> + return -ESRCH;
> + if (*ppos != 0) {
> + /* No partial writes. */
> + put_task_struct(task);
> + return -EINVAL;
> + }
> +
> + rv = kstrtou32_from_user(buf, count, 10, &capcontid);
> + if (rv < 0) {
> + put_task_struct(task);
> + return rv;
> + }
> +
> + rv = audit_set_capcontid(task, capcontid);
> + put_task_struct(task);
> + if (rv < 0)
> + return rv;
> + return count;
> +}
> +
> +static const struct file_operations proc_capcontid_operations = {
> + .read = proc_capcontid_read,
> + .write = proc_capcontid_write,
> + .llseek = generic_file_llseek,
> +};
> #endif
>
> #ifdef CONFIG_FAULT_INJECTION
> @@ -3121,6 +3174,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> + REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3522,6 +3576,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
> + REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 28b9c7cd86a6..62c453306c2a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -116,6 +116,7 @@ struct audit_task_info {
> kuid_t loginuid;
> unsigned int sessionid;
> struct audit_contobj *cont;
> + u32 capcontid;
Where is the code change that actually uses this to enforce the
described policy on setting an audit container ID?
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 2844d78cd7af..01251e6dcec0 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -73,6 +73,7 @@
> #define AUDIT_GET_FEATURE 1019 /* Get which features are enabled */
> #define AUDIT_CONTAINER_OP 1020 /* Define the container id and info */
> #define AUDIT_SIGNAL_INFO2 1021 /* Get info auditd signal sender */
> +#define AUDIT_SET_CAPCONTID 1022 /* Set cap_contid of a task */
>
> #define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly uninteresting to kernel */
> #define AUDIT_USER_AVC 1107 /* We filter this differently */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 1287f0b63757..1c22dd084ae8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2698,6 +2698,41 @@ static bool audit_contid_isowner(struct task_struct *tsk)
> return false;
> }
>
> +int audit_set_capcontid(struct task_struct *task, u32 enable)
> +{
> + u32 oldcapcontid;
> + int rc = 0;
> + struct audit_buffer *ab;
> +
> + if (!task->audit)
> + return -ENOPROTOOPT;
> + oldcapcontid = audit_get_capcontid(task);
> + /* if task is not descendant, block */
> + if (task == current)
> + rc = -EBADSLT;
> + else if (!task_is_descendant(current, task))
> + rc = -EXDEV;
See my previous comments about error code sanity.
> + else if (current_user_ns() == &init_user_ns) {
> + if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
> + rc = -EPERM;
I think we just want to use ns_capable() in the context of the current
userns to check CAP_AUDIT_CONTROL, yes? Something like this ...
if (current_user_ns() != &init_user_ns) {
if (!ns_capable(CAP_AUDIT_CONTROL) || !audit_get_capcontid())
rc = -EPERM;
} else if (!capable(CAP_AUDIT_CONTROL))
rc = -EPERM;
> + }
> + if (!rc)
> + task->audit->capcontid = enable;
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_SET_CAPCONTID);
> + if (!ab)
> + return rc;
> +
> + audit_log_format(ab,
> + "opid=%d capcontid=%u old-capcontid=%u",
> + task_tgid_nr(task), enable, oldcapcontid);
> + audit_log_end(ab);
My prior comments about recording the success/failure, or not emitting
the record on failure, seem relevant here too.
> + return rc;
> +}
^ permalink raw reply
* Re: [PATCH ghak90 V8 15/16] audit: check contid count per netns and add config param limit
From: Paul Moore @ 2020-01-22 21:29 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <229a7be1f1136906a360a46e2cf8cdcd4c7c4b1b.1577736799.git.rgb@redhat.com>
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Clamp the number of audit container identifiers associated with a
> network namespace to limit the netlink and disk bandwidth used and to
> prevent losing information from record text size overflow in the contid
> field.
>
> Add a configuration parameter AUDIT_STATUS_CONTID_NETNS_LIMIT (0x100)
> to set the audit container identifier netns limit. This is used to
> prevent overflow of the contid field in CONTAINER_OP and CONTAINER_ID
> messages, losing information, and to limit bandwidth used by these
> messages.
>
> This value must be balanced with the audit container identifier nesting
> depth limit to multiply out to no more than 400. This is determined by
> the total audit message length less message overhead divided by the
> length of the text representation of an audit container identifier.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 16 +++++++----
> include/linux/nsproxy.h | 2 +-
> include/uapi/linux/audit.h | 2 ++
> kernel/audit.c | 68 ++++++++++++++++++++++++++++++++++++++--------
> kernel/audit.h | 7 +++++
> kernel/fork.c | 10 +++++--
> kernel/nsproxy.c | 27 +++++++++++++++---
> 7 files changed, 107 insertions(+), 25 deletions(-)
Similar to my comments in patch 14, let's defer this to a later time
if we need to do this at all.
^ permalink raw reply
* Re: [PATCH ghak90 V8 14/16] audit: check contid depth and add limit config param
From: Paul Moore @ 2020-01-22 21:29 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <28cf3e16f8440bcb852767d3ae13e1a56c19569c.1577736799.git.rgb@redhat.com>
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Clamp the depth of audit container identifier nesting to limit the
> netlink and disk bandwidth used and to prevent losing information from
> record text size overflow in the contid field.
>
> Add a configuration parameter AUDIT_STATUS_CONTID_DEPTH_LIMIT (0x80) to
> set the audit container identifier depth limit. This can be used to
> prevent overflow of the contid field in CONTAINER_OP and CONTAINER_ID
> messages, losing information, and to limit bandwidth used by these
> messages.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/uapi/linux/audit.h | 2 ++
> kernel/audit.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/audit.h | 2 ++
> 3 files changed, 50 insertions(+)
Since setting an audit container ID, and hence acting as an
orchestrator and creating a new nested level of audit container IDs,
is a privileged operation I think we can equate this to the infamous
"shooting oneself in the foot" problem. Let's leave this limitation
out of the patchset for now, if it becomes a problem in the future we
can consider restricting the nesting depth.
^ permalink raw reply
* Re: [PATCH ghak90 V8 13/16] audit: track container nesting
From: Paul Moore @ 2020-01-22 21:29 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <6452955c1e038227a5cd169f689f3fd3db27513f.1577736799.git.rgb@redhat.com>
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Track the parent container of a container to be able to filter and
> report nesting.
>
> Now that we have a way to track and check the parent container of a
> container, modify the contid field format to be able to report that
> nesting using a carrat ("^") separator to indicate nesting. The
> original field format was "contid=<contid>" for task-associated records
> and "contid=<contid>[,<contid>[...]]" for network-namespace-associated
> records. The new field format is
> "contid=<contid>[^<contid>[...]][,<contid>[...]]".
Let's make sure we always use a comma as a separator, even when
recording the parent information, for example:
"contid=<contid>[,^<contid>[...]][,<contid>[...]]"
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 1 +
> kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++--------
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 17 ++++++++++++++++-
> kernel/auditsc.c | 2 +-
> 5 files changed, 64 insertions(+), 10 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ef8e07524c46..68be59d1a89b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> audit_netns_contid_add(new->net_ns, contid);
> }
>
> +void audit_log_contid(struct audit_buffer *ab, u64 contid);
If we need a forward declaration, might as well just move it up near
the top of the file with the rest of the declarations.
> +void audit_log_contid(struct audit_buffer *ab, u64 contid)
> +{
> + struct audit_contobj *cont = NULL, *prcont = NULL;
> + int h;
It seems safer to pass the audit container ID object and not the u64.
> + if (!audit_contid_valid(contid)) {
> + audit_log_format(ab, "%llu", contid);
Do we really want to print (u64)-1 here? Since this is a known
invalid number, would "?" be a better choice?
> + return;
> + }
> + h = audit_hash_contid(contid);
> + rcu_read_lock();
> + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
> + if (cont->id == contid) {
> + prcont = cont;
Why not just pull the code below into the body of this if statement?
It all needs to be done under the RCU read lock anyway and the code
would read much better this way.
> + break;
> + }
> + if (!prcont) {
> + audit_log_format(ab, "%llu", contid);
> + goto out;
> + }
> + while (prcont) {
> + audit_log_format(ab, "%llu", prcont->id);
> + prcont = prcont->parent;
> + if (prcont)
> + audit_log_format(ab, "^");
In the interest of limiting the number of calls to audit_log_format(),
how about something like the following:
audit_log_format("%llu", cont);
iter = cont->parent;
while (iter) {
if (iter->parent)
audit_log_format("^%llu,", iter);
else
audit_log_format("^%llu", iter);
iter = iter->parent;
}
> + }
> +out:
> + rcu_read_unlock();
> +}
> +
> /*
> * audit_log_container_id - report container info
> * @context: task or local context for record
...
> @@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> if (!ab)
> return rc;
>
> - audit_log_format(ab,
> - "op=set opid=%d contid=%llu old-contid=%llu",
> - task_tgid_nr(task), contid, oldcontid);
> + audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task));
> + audit_log_contid(ab, contid);
> + audit_log_format(ab, " old-contid=");
> + audit_log_contid(ab, oldcontid);
This is an interesting case where contid and old-contid are going to
be largely the same, only the first (current) ID is going to be
different; do we want to duplicate all of those IDs?
> audit_log_end(ab);
> return rc;
> }
> @@ -2723,9 +2760,9 @@ void audit_log_container_drop(void)
^ permalink raw reply
* Re: [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting
From: Paul Moore @ 2020-01-22 21:29 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux-Audit Mailing List,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, LKML,
netdev-u79uwXL29TY76Z2rM5mHXA,
netfilter-devel-u79uwXL29TY76Z2rM5mHXA,
sgrubb-H+wXaHxf7aLQT0dZR+AlfA, omosnace-H+wXaHxf7aLQT0dZR+AlfA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA, simo-H+wXaHxf7aLQT0dZR+AlfA,
Eric Paris, Serge Hallyn, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, Dan Walsh,
mpatel-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <cfbb80a08fc770dd0dcf6dac6ff307a80d877c3f.1577736799.git.rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Require the target task to be a descendant of the container
> orchestrator/engine.
>
> You would only change the audit container ID from one set or inherited
> value to another if you were nesting containers.
>
> If changing the contid, the container orchestrator/engine must be a
> descendant and not same orchestrator as the one that set it so it is not
> possible to change the contid of another orchestrator's container.
>
> Since the task_is_descendant() function is used in YAMA and in audit,
> remove the duplication and pull the function into kernel/core/sched.c
>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> include/linux/sched.h | 3 +++
> kernel/audit.c | 44 ++++++++++++++++++++++++++++++++++++--------
> kernel/sched/core.c | 33 +++++++++++++++++++++++++++++++++
> security/yama/yama_lsm.c | 33 ---------------------------------
> 4 files changed, 72 insertions(+), 41 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f7a8d3288ca0..ef8e07524c46 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2603,22 +2610,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> oldcontid = audit_get_contid(task);
> read_lock(&tasklist_lock);
> /* Don't allow the contid to be unset */
> - if (!audit_contid_valid(contid))
> + if (!audit_contid_valid(contid)) {
> rc = -EINVAL;
> + goto unlock;
> + }
> /* Don't allow the contid to be set to the same value again */
> - else if (contid == oldcontid) {
> + if (contid == oldcontid) {
> rc = -EADDRINUSE;
> + goto unlock;
> + }
> /* if we don't have caps, reject */
> - else if (!capable(CAP_AUDIT_CONTROL))
> + if (!capable(CAP_AUDIT_CONTROL)) {
> rc = -EPERM;
> - /* if task has children or is not single-threaded, deny */
> - else if (!list_empty(&task->children))
> + goto unlock;
> + }
> + /* if task has children, deny */
> + if (!list_empty(&task->children)) {
> rc = -EBUSY;
> - else if (!(thread_group_leader(task) && thread_group_empty(task)))
> + goto unlock;
> + }
> + /* if task is not single-threaded, deny */
> + if (!(thread_group_leader(task) && thread_group_empty(task))) {
> rc = -EALREADY;
> - /* if contid is already set, deny */
> - else if (audit_contid_set(task))
> + goto unlock;
> + }
It seems like the if/else-if conversion above should be part of an
earlier patchset.
> + /* if task is not descendant, block */
> + if (task == current) {
> + rc = -EBADSLT;
> + goto unlock;
> + }
> + if (!task_is_descendant(current, task)) {
> + rc = -EXDEV;
> + goto unlock;
> + }
I understand you are trying to provide a unique error code for each
failure case, but this is getting silly. Let's group the descendent
checks under the same error code.
> + /* only allow contid setting again if nesting */
> + if (audit_contid_set(task) && audit_contid_isowner(task))
> rc = -ECHILD;
Should that be "!audit_contid_isowner()"?
^ permalink raw reply
* Re: [PATCH ghak90 V8 11/16] audit: add support for containerid to network namespaces
From: Paul Moore @ 2020-01-22 21:28 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <2954ed671a7622ddf3abdb8854dbba2ad13e9f33.1577736799.git.rgb@redhat.com>
On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> This also adds support to qualify NETFILTER_PKT records.
>
> 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
>
> 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.
>
> Please see the github audit kernel issue for contid net support:
> https://github.com/linux-audit/audit-kernel/issues/92
> Please see the github audit testsuiite issue for the test case:
> https://github.com/linux-audit/audit-testsuite/issues/64
> Please see the github audit wiki for the feature overview:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> include/linux/audit.h | 24 +++++++++
> kernel/audit.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++-
> kernel/nsproxy.c | 4 ++
> net/netfilter/nft_log.c | 11 +++-
> net/netfilter/xt_AUDIT.c | 11 +++-
> 5 files changed, 176 insertions(+), 6 deletions(-)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 5531d37a4226..ed8d5b74758d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -12,6 +12,7 @@
> #include <linux/sched.h>
> #include <linux/ptrace.h>
> #include <uapi/linux/audit.h>
> +#include <linux/refcount.h>
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -121,6 +122,13 @@ struct audit_task_info {
>
> extern struct audit_task_info init_struct_audit;
>
> +struct audit_contobj_netns {
> + struct list_head list;
> + u64 id;
Since we now track audit container IDs in their own structure, why not
link directly to the audit container ID object (and bump the
refcount)?
> + refcount_t refcount;
> + struct rcu_head rcu;
> +};
> +
> extern int is_audit_feature_set(int which);
>
> extern int __init audit_register_class(int class, unsigned *list);
> @@ -225,6 +233,12 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> }
>
> extern void audit_log_container_id(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 void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context);
>
> extern u32 audit_enabled;
>
> @@ -297,6 +311,16 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
>
> static inline void audit_log_container_id(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)
> +{ }
> +static inline void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context)
> +{ }
>
> #define audit_enabled AUDIT_OFF
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index d4e6eafe5644..f7a8d3288ca0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -59,6 +59,7 @@
> #include <linux/freezer.h>
> #include <linux/pid_namespace.h>
> #include <net/netns/generic.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -86,9 +87,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;
> };
>
> /**
> @@ -305,8 +310,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.
> */
> @@ -409,6 +417,120 @@ 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;
> + struct list_head *contid_list;
> + struct audit_contobj_netns *cont;
> +
> + if (!net)
> + return;
> + if (!audit_contid_valid(contid))
> + return;
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + return;
> + contid_list = &aunet->contid_list;
> + rcu_read_lock();
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + spin_lock(&aunet->contid_list_lock);
> + refcount_inc(&cont->refcount);
> + spin_unlock(&aunet->contid_list_lock);
> + goto out;
> + }
> + cont = kmalloc(sizeof(*cont), GFP_ATOMIC);
> + if (cont) {
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + spin_lock(&aunet->contid_list_lock);
> + list_add_rcu(&cont->list, contid_list);
> + spin_unlock(&aunet->contid_list_lock);
> + }
> +out:
> + rcu_read_unlock();
> +}
See my comments about refcount_t, spinlocks, and list manipulation
races from earlier in the patchset; the same thing applies to the
function above.
^ permalink raw reply
* Re: [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon
From: Paul Moore @ 2020-01-22 21:28 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux-Audit Mailing List,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, LKML,
netdev-u79uwXL29TY76Z2rM5mHXA,
netfilter-devel-u79uwXL29TY76Z2rM5mHXA,
sgrubb-H+wXaHxf7aLQT0dZR+AlfA, omosnace-H+wXaHxf7aLQT0dZR+AlfA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA, simo-H+wXaHxf7aLQT0dZR+AlfA,
Eric Paris, Serge Hallyn, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, Dan Walsh,
mpatel-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <7d7933d742fdf4a94c84b791906a450b16f2e81f.1577736799.git.rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Add audit container identifier support to the action of signalling the
> audit daemon.
>
> Since this would need to add an element to the audit_sig_info struct,
> a new record type AUDIT_SIGNAL_INFO2 was created with a new
> audit_sig_info2 struct. Corresponding support is required in the
> userspace code to reflect the new record request and reply type.
> An older userspace won't break since it won't know to request this
> record type.
>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> include/linux/audit.h | 7 +++++++
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++
> kernel/audit.h | 1 +
> security/selinux/nlmsgtab.c | 1 +
> 5 files changed, 45 insertions(+)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 0871c3e5d6df..51159c94041c 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -126,6 +126,14 @@ struct auditd_connection {
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> u32 audit_sig_sid = 0;
> +/* Since the signal information is stored in the record buffer at the
> + * time of the signal, but not retrieved until later, there is a chance
> + * that the last process in the container could terminate before the
> + * signal record is delivered. In this circumstance, there is a chance
> + * the orchestrator could reuse the audit container identifier, causing
> + * an overlap of audit records that refer to the same audit container
> + * identifier, but a different container instance. */
> +u64 audit_sig_cid = AUDIT_CID_UNSET;
I believe we could prevent the case mentioned above by taking an
additional reference to the audit container ID object when the signal
information is collected, dropping it only after the signal
information is collected by userspace or another process signals the
audit daemon. Yes, it would block that audit container ID from being
reused immediately, but since we are talking about one number out of
2^64 that seems like a reasonable tradeoff.
^ permalink raw reply
* Re: [PATCH ghak90 V8 05/16] audit: log drop of contid on exit of last task
From: Paul Moore @ 2020-01-22 21:28 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, sgrubb, omosnace, dhowells, simo,
Eric Paris, Serge Hallyn, ebiederm, nhorman, Dan Walsh, mpatel
In-Reply-To: <b3725abab452beaba740ac58f76144e6c3bda2fa.1577736799.git.rgb@redhat.com>
On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Since we are tracking the life of each audit container indentifier, we
> can match the creation event with the destruction event. Log the
> destruction of the audit container identifier when the last process in
> that container exits.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 17 +++++++++++++++++
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 2 ++
> 3 files changed, 21 insertions(+)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 4bab20f5f781..fa8f1aa3a605 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2502,6 +2502,23 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> return rc;
> }
>
> +void audit_log_container_drop(void)
> +{
> + struct audit_buffer *ab;
> +
> + if (!current->audit || !current->audit->cont ||
> + refcount_read(¤t->audit->cont->refcount) > 1)
> + return;
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> + if (!ab)
> + return;
> +
> + audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=%llu",
> + task_tgid_nr(current), audit_get_contid(current),
> + audit_get_contid(current));
> + audit_log_end(ab);
> +}
Assumine we are careful about where we call it in audit_free(...), you
are confident we can't do this as part of _audit_contobj_put(...),
yes?
> /**
> * audit_log_end - end one audit record
> * @ab: the audit_buffer
> diff --git a/kernel/audit.h b/kernel/audit.h
> index e4a31aa92dfe..162de8366b32 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -255,6 +255,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
> extern struct tty_struct *audit_get_tty(void);
> extern void audit_put_tty(struct tty_struct *tty);
>
> +extern void audit_log_container_drop(void);
> +
> /* audit watch/mark/tree functions */
> #ifdef CONFIG_AUDITSYSCALL
> extern unsigned int audit_serial(void);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0e2d50533959..bd855794ad26 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1568,6 +1568,8 @@ static void audit_log_exit(void)
>
> audit_log_proctitle();
>
> + audit_log_container_drop();
> +
> /* 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
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 V8 04/16] audit: convert to contid list to check for orch/engine ownership
From: Paul Moore @ 2020-01-22 21:28 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux-Audit Mailing List,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, LKML,
netdev-u79uwXL29TY76Z2rM5mHXA,
netfilter-devel-u79uwXL29TY76Z2rM5mHXA,
sgrubb-H+wXaHxf7aLQT0dZR+AlfA, omosnace-H+wXaHxf7aLQT0dZR+AlfA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA, simo-H+wXaHxf7aLQT0dZR+AlfA,
Eric Paris, Serge Hallyn, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, Dan Walsh,
mpatel-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <a911acf0b209c05dc156fb6b57f9da45778747ce.1577736799.git.rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Dec 31, 2019 at 2:50 PM Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Store the audit container identifier in a refcounted kernel object that
> is added to the master list of audit container identifiers. This will
> allow multiple container orchestrators/engines to work on the same
> machine without danger of inadvertantly re-using an existing identifier.
> It will also allow an orchestrator to inject a process into an existing
> container by checking if the original container owner is the one
> injecting the task. A hash table list is used to optimize searches.
>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> include/linux/audit.h | 14 ++++++--
> kernel/audit.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++---
> kernel/audit.h | 8 +++++
> 3 files changed, 112 insertions(+), 8 deletions(-)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index a045b34ecf44..0e6dbe943ae4 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -94,10 +94,18 @@ struct audit_ntp_data {
> struct audit_ntp_data {};
> #endif
>
> +struct audit_contobj {
> + struct list_head list;
> + u64 id;
> + struct task_struct *owner;
> + refcount_t refcount;
> + struct rcu_head rcu;
> +};
> +
> struct audit_task_info {
> kuid_t loginuid;
> unsigned int sessionid;
> - u64 contid;
> + struct audit_contobj *cont;
> #ifdef CONFIG_AUDITSYSCALL
> struct audit_context *ctx;
> #endif
> @@ -203,9 +211,9 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
>
> static inline u64 audit_get_contid(struct task_struct *tsk)
> {
> - if (!tsk->audit)
> + if (!tsk->audit || !tsk->audit->cont)
> return AUDIT_CID_UNSET;
> - return tsk->audit->contid;
> + return tsk->audit->cont->id;
> }
>
> extern u32 audit_enabled;
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2d7707426b7d..4bab20f5f781 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -212,6 +218,31 @@ void __init audit_task_init(void)
> 0, SLAB_PANIC, NULL);
> }
>
> +static struct audit_contobj *_audit_contobj(struct task_struct *tsk)
> +{
> + if (!tsk->audit)
> + return NULL;
> + return tsk->audit->cont;
It seems like it would be safer to grab a reference here (e.g.
_audit_contobj_hold(...)), yes? Or are you confident we will never
have tsk go away while the caller is holding on to the returned audit
container ID object?
> +}
> +
> +/* audit_contobj_list_lock must be held by caller unless new */
> +static void _audit_contobj_hold(struct audit_contobj *cont)
> +{
> + refcount_inc(&cont->refcount);
> +}
If we are going to call the matching decrement function "_put" it
seems like we might want to call the function about "_get". Further,
we can also have it return an audit_contobj pointer in case the caller
needs to do an assignment as well (which seems typical if you need to
bump the refcount):
_audit_contobj_get(audit_contobj *cont)
{
if (cont)
refcount_inc(cont);
return cont;
}
> +/* audit_contobj_list_lock must be held by caller */
> +static void _audit_contobj_put(struct audit_contobj *cont)
> +{
> + if (!cont)
> + return;
> + if (refcount_dec_and_test(&cont->refcount)) {
> + put_task_struct(cont->owner);
> + list_del_rcu(&cont->list);
> + kfree_rcu(cont, rcu);
> + }
> +}
> +
> /**
> * audit_alloc - allocate an audit info block for a task
> * @tsk: task
> @@ -232,7 +263,11 @@ int audit_alloc(struct task_struct *tsk)
> }
> info->loginuid = audit_get_loginuid(current);
> info->sessionid = audit_get_sessionid(current);
> - info->contid = audit_get_contid(current);
> + spin_lock(&audit_contobj_list_lock);
> + info->cont = _audit_contobj(current);
> + if (info->cont)
> + _audit_contobj_hold(info->cont);
> + spin_unlock(&audit_contobj_list_lock);
If we are taking a spinlock in order to bump the refcount, does it
really need to be a refcount_t or can we just use a normal integer?
In RCU protected lists a spinlock is usually used to protect
adds/removes to the list, not the content of individual list items.
My guess is you probably want to use the spinlock as described above
(list add/remove protection) and manipulate the refcount_t inside a
RCU read lock protected region.
> tsk->audit = info;
>
> ret = audit_alloc_syscall(tsk);
> @@ -267,6 +302,9 @@ void audit_free(struct task_struct *tsk)
> /* Freeing the audit_task_info struct must be performed after
> * audit_log_exit() due to need for loginuid and sessionid.
> */
> + spin_lock(&audit_contobj_list_lock);
> + _audit_contobj_put(tsk->audit->cont);
> + spin_unlock(&audit_contobj_list_lock);
This is another case of refcount_t vs normal integer in a spinlock
protected region.
> info = tsk->audit;
> tsk->audit = NULL;
> kmem_cache_free(audit_task_cache, info);
> @@ -2365,6 +2406,9 @@ int audit_signal_info(int sig, struct task_struct *t)
> *
> * Returns 0 on success, -EPERM on permission failure.
> *
> + * If the original container owner goes away, no task injection is
> + * possible to an existing container.
> + *
> * Called (set) from fs/proc/base.c::proc_contid_write().
> */
> int audit_set_contid(struct task_struct *task, u64 contid)
> @@ -2381,9 +2425,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> }
> oldcontid = audit_get_contid(task);
> read_lock(&tasklist_lock);
> - /* Don't allow the audit containerid to be unset */
> + /* Don't allow the contid to be unset */
> if (!audit_contid_valid(contid))
> rc = -EINVAL;
> + /* Don't allow the contid to be set to the same value again */
> + else if (contid == oldcontid) {
> + rc = -EADDRINUSE;
First, is that brace a typo? It looks like it. Did this compile?
Second, can you explain why (re)setting the audit container ID to the
same value is something we need to prohibit? I'm guessing it has
something to do with explicitly set vs inherited, but I don't want to
assume too much about your thinking behind this.
If it is "set vs inherited", would allowing an orchestrator to
explicitly "set" an inherited audit container ID provide some level or
protection against moving the task?
> /* if we don't have caps, reject */
> else if (!capable(CAP_AUDIT_CONTROL))
> rc = -EPERM;
> @@ -2396,8 +2443,49 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> else if (audit_contid_set(task))
> rc = -ECHILD;
> read_unlock(&tasklist_lock);
> - if (!rc)
> - task->audit->contid = contid;
> + if (!rc) {
> + struct audit_contobj *oldcont = _audit_contobj(task);
> + struct audit_contobj *cont = NULL, *newcont = NULL;
> + int h = audit_hash_contid(contid);
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
> + if (cont->id == contid) {
> + /* task injection to existing container */
> + if (current == cont->owner) {
Do we have any protection against the task pointed to by cont->owner
going away and a new task with the same current pointer value (no
longer the legitimate audit container ID owner) manipulating the
target task's audit container ID?
> + spin_lock(&audit_contobj_list_lock);
> + _audit_contobj_hold(cont);
> + spin_unlock(&audit_contobj_list_lock);
More of the recount_t vs integer/spinlock question.
> + newcont = cont;
> + } else {
> + rc = -ENOTUNIQ;
> + goto conterror;
> + }
> + break;
> + }
> + if (!newcont) {
> + newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC);
> + if (newcont) {
> + INIT_LIST_HEAD(&newcont->list);
> + newcont->id = contid;
> + get_task_struct(current);
> + newcont->owner = current;
newcont->owner = get_task_struct(current);
(This is what I was talking about above with returning the struct
pointer in the _get/_hold function)
> + refcount_set(&newcont->refcount, 1);
> + spin_lock(&audit_contobj_list_lock);
> + list_add_rcu(&newcont->list, &audit_contid_hash[h]);
> + spin_unlock(&audit_contobj_list_lock);
I think we might have a problem where multiple tasks could race adding
the same audit container ID and since there is no check inside the
spinlock protected region we could end up adding multiple instances of
the same audit container ID, yes?
> + } else {
> + rc = -ENOMEM;
> + goto conterror;
> + }
> + }
> + task->audit->cont = newcont;
> + spin_lock(&audit_contobj_list_lock);
> + _audit_contobj_put(oldcont);
> + spin_unlock(&audit_contobj_list_lock);
More of the refcount_t/integer/spinlock question.
> +conterror:
> + rcu_read_unlock();
> + }
> task_unlock(task);
>
> if (!audit_enabled)
^ permalink raw reply
* Re: [PATCH ghak90 V8 02/16] audit: add container id
From: Paul Moore @ 2020-01-22 21:28 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux-Audit Mailing List,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, LKML,
netdev-u79uwXL29TY76Z2rM5mHXA,
netfilter-devel-u79uwXL29TY76Z2rM5mHXA,
sgrubb-H+wXaHxf7aLQT0dZR+AlfA, omosnace-H+wXaHxf7aLQT0dZR+AlfA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA, simo-H+wXaHxf7aLQT0dZR+AlfA,
Eric Paris, Serge Hallyn, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
nhorman-2XuSBdqkA4R54TAoqtyWWQ, Dan Walsh,
mpatel-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <70ad50e69185c50843d5e14462f1c4f03655d503.1577736799.git.rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Tue, Dec 31, 2019 at 2:49 PM Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Implement the proc fs write to set the audit container identifier of a
> process, emitting an AUDIT_CONTAINER_OP record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/audit_containerid where PID is the process ID of the
> newly created task that is to become the first task in a container, or
> an additional task added to a container.
>
> The write expects up to a u64 value (unset: 18446744073709551615).
>
> The writer must have capability CAP_AUDIT_CONTROL.
>
> This will produce a record such as this:
> type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 contid=123456 old-contid=18446744073709551615
>
> The "op" field indicates an initial set. The "opid" field is the
> object's PID, the process being "contained". New and old audit
> container identifier values are given in the "contid" fields.
>
> It is not permitted to unset the audit container identifier.
> A child inherits its parent's audit container identifier.
>
> Please see the github audit kernel issue for the main feature:
> https://github.com/linux-audit/audit-kernel/issues/90
> Please see the github audit userspace issue for supporting additions:
> https://github.com/linux-audit/audit-userspace/issues/51
> Please see the github audit testsuiite issue for the test case:
> https://github.com/linux-audit/audit-testsuite/issues/64
> Please see the github audit wiki for the feature overview:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Serge Hallyn <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
> Acked-by: Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Reviewed-by: Ondrej Mosnacek <omosnace-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> fs/proc/base.c | 36 ++++++++++++++++++++++++++++
> include/linux/audit.h | 25 ++++++++++++++++++++
> include/uapi/linux/audit.h | 2 ++
> kernel/audit.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 4 ++++
> 6 files changed, 126 insertions(+)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 397f8fb4836a..2d7707426b7d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2356,6 +2358,62 @@ int audit_signal_info(int sig, struct task_struct *t)
> return audit_signal_info_syscall(t);
> }
>
> +/*
> + * audit_set_contid - set current task's audit contid
> + * @task: target task
> + * @contid: contid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_contid_write().
> + */
> +int audit_set_contid(struct task_struct *task, u64 contid)
> +{
> + u64 oldcontid;
> + int rc = 0;
> + struct audit_buffer *ab;
> +
> + task_lock(task);
> + /* Can't set if audit disabled */
> + if (!task->audit) {
> + task_unlock(task);
> + return -ENOPROTOOPT;
> + }
> + oldcontid = audit_get_contid(task);
> + read_lock(&tasklist_lock);
> + /* Don't allow the audit containerid to be unset */
> + if (!audit_contid_valid(contid))
> + rc = -EINVAL;
> + /* if we don't have caps, reject */
> + else if (!capable(CAP_AUDIT_CONTROL))
> + rc = -EPERM;
> + /* if task has children or is not single-threaded, deny */
> + else if (!list_empty(&task->children))
> + rc = -EBUSY;
> + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> + rc = -EALREADY;
[NOTE: there is a bigger issue below which I think is going to require
a respin/fixup of this patch so I'm going to take the opportunity to
do a bit more bikeshedding ;)]
It seems like we could combine both the thread/children checks under a
single -EBUSY return value. In both cases the caller should be able
to determine if the target process is multi-threaded for has spawned
children, yes? FWIW, my motivation for this question is that
-EALREADY seems like a poor choice here.
> + /* if contid is already set, deny */
> + else if (audit_contid_set(task))
> + rc = -ECHILD;
Does -EEXIST make more sense here?
> + read_unlock(&tasklist_lock);
> + if (!rc)
> + task->audit->contid = contid;
> + task_unlock(task);
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> + if (!ab)
> + return rc;
> +
> + audit_log_format(ab,
> + "op=set opid=%d contid=%llu old-contid=%llu",
> + task_tgid_nr(task), contid, oldcontid);
> + audit_log_end(ab);
Assuming audit is enabled we always emit the record above, even if we
were not actually able to set the Audit Container ID (ACID); this
seems wrong to me. I think the proper behavior would be to either add
a "res=" field to indicate success/failure or only emit the record
when we actually change a task's ACID. Considering the impact that
the ACID value will potentially have on the audit stream, it seems
like always logging the record and including a "res=" field may be the
safer choice.
> + return rc;
> +}
> +
> /**
> * audit_log_end - end one audit record
> * @ab: the audit_buffer
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Mathieu Desnoyers @ 2020-01-22 17:16 UTC (permalink / raw)
To: Jan Ziak
Cc: Andi Kleen, Ben Maurer, Boqun Feng, Catalin Marinas,
Chris Lameter, Dave Watson, H. Peter Anvin, Joel Fernandes,
Josh Triplett, linux-api, linux-kernel, linux-kselftest,
Russell King, Andy Lutomirski, Ingo Molnar, Michael Kerrisk, Paul,
Peter Zijlstra, Paul Turner, rostedt, shuah
In-Reply-To: <CAODFU0rTLmb-Ph_n1EHaZmdOAjsa6Jmx=3zkuT8LH3No=sOk5w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
(replying again as plain text for mailing lists)
----- On Jan 22, 2020, at 10:44 AM, Jan Ziak 0xe2.0x9a.0x9b-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> Hello
> I would like to note that this does not help userspace to express dynamic
> scheduling relationships among processes/threads such as "do not run processes
> A and B on the same core" or "run processes A and B on cores sharing the same
> L2 cache".
Indeed, this is not what this system call is trying to solve. Does the name "pin_on_cpu" lead
to confusion here ?
I thought that cgroups was already the mechanism taking care of this kind of requirement.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v1] pin_on_cpu: Introduce thread CPU pinning system call
From: Jan Ziak @ 2020-01-22 15:48 UTC (permalink / raw)
To: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-kselftest-u79uwXL29TY76Z2rM5mHXA
Hello
I would like to note that this does not help userspace to express
dynamic scheduling relationships among processes/threads such as "do
not run processes A and B on the same core" or "run processes A and B
on cores sharing the same L2 cache".
Sincerely
Jan
^ permalink raw reply
* Re: [RESEND Patch v6 2/2] fpga: dfl: fme: add performance reporting support
From: Greg KH @ 2020-01-22 14:10 UTC (permalink / raw)
To: Wu Hao
Cc: will-DgEjT+Ai2ygdnm+yROfE0A, mdf-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, linux-fpga-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, atull-DgEjT+Ai2ygdnm+yROfE0A,
Luwei Kang, Xu Yilun
In-Reply-To: <1579230628-22243-3-git-send-email-hao.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On Fri, Jan 17, 2020 at 11:10:28AM +0800, Wu Hao wrote:
> +static const struct attribute_group fme_perf_fabric_events_group = {
> + .name = "events",
> + .attrs = fme_perf_fabric_events_attrs,
> + .is_visible = fme_perf_fabric_events_visible,
> +};
> +
I don't see any Documentation/ABI/ entries for all of these new files.
Please properly document them so we know how to review this.
thanks,
greg k-h
^ permalink raw reply
* Re: Re: Re: [PATCH v2 2/5] mm: introduce external memory hinting API
From: SeongJae Park @ 2020-01-22 13:28 UTC (permalink / raw)
To: Michal Hocko
Cc: SeongJae Park, Minchan Kim, sspatil-hpIqsD4AKlfQT0dZR+AlfA,
kirill-oKw7cIdHH8eLwutG50LtGA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA,
oleksandr-H+wXaHxf7aLQT0dZR+AlfA, surenb-hpIqsD4AKlfQT0dZR+AlfA,
timmurray-hpIqsD4AKlfQT0dZR+AlfA, dancol-hpIqsD4AKlfQT0dZR+AlfA,
sonnyrao-hpIqsD4AKlfQT0dZR+AlfA, bgeffon-hpIqsD4AKlfQT0dZR+AlfA,
hannes-druUgvl0LCNAfugRpC6u6w, shakeelb-hpIqsD4AKlfQT0dZR+AlfA,
joaodias-hpIqsD4AKlfQT0dZR+AlfA, ktkhai-5HdwGun5lf+gSpxsJD1C4w,
christian.brauner-GeWIH/nMZzLQT0dZR+AlfA,
sjpark-ebkRAfMGSJGzQB+pC5nmwQ
In-Reply-To: <20200122100233.GT29276-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org>
On Wed, 22 Jan 2020 11:02:33 +0100 Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed 22-01-20 10:36:24, SeongJae Park wrote:
> > On Wed, 22 Jan 2020 09:28:53 +0100 Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > > On Tue 21-01-20 10:32:12, Minchan Kim wrote:
> > > > On Mon, Jan 20, 2020 at 08:58:25AM +0100, Michal Hocko wrote:
> > > [...]
> > > > > The interface really has to be robust to future potential usecases.
> > > >
> > > > I do understand your concern but for me, it's chicken and egg problem.
> > > > We usually do best effort to make something perfect as far as possible
> > > > but we also don't do over-engineering without real usecase from the
> > > > beginning.
> > > >
> > > > I already told you how we could synchronize among processes and potential
> > > > way to be extended Daniel suggested(That's why current API has extra field
> > > > for the cookie) even though we don't need it right now.
> > >
> > > If you can synchronize with the target task then you do not need a
> > > remote interface. Just use ptrace and you are done with it.
> > >
> > > > If you want to suggest the other way, please explain why your idea is
> > > > better and why we need it at this moment.
> > >
> > > I believe I have explained my concerns and why they matter. All you are
> > > saying is that you do not care because your particular usecase doesn't
> > > care. And that is a first signal of a future disaster when we end up
> > > with a broken and unfixable interface we have to maintain for ever.
> > >
> > > I will not go as far as to nack this but you should seriously think
> > > about other potential usecases and how they would work and what we are
> > > going to do when a first non-cooperative userspace memory management
> > > usecase materializes.
> >
> > Beside of the specific environment of Android, I think there are many ways to
> > know the address space layout and access patterns of other processes. The
> > idle_page_tracking might be an example that widelay available.
> >
> > Of course, the information might not strictly correct due to the timing issue,
> > but could be still worth to be used under some extreme situations, such as
> > memory pressure or fragmentation. For the same reason, ptrace() would not be
> > sufficient, as we have no perfect control, but only some level of control that
> > would be useful under specific situations.
>
> I am not sure I see your point. I am talking about races where a remote
> task is operating on a completely different object because the one it
> checked for has been unmapped and new one mapped over it. Memory
> pressure or a fragmentation will not change the object itself. Sure the
> memory might be reclaimed but that should be completely OK unless I am
> missing something.
Thank you for pointing out your concerns in more detail. I was assuming a case
using MADV_PAGEOUT or MADV_HUGEPAGE like hints under access frequency
monitoring for better performance under memory pressure or fragmentation,
respectively. Under the race, such hints might incur some performance
degradation, but no critical problem such as SEGV. I previously implemented
such optimization for research purpose and it was worthy. Nonetheless, it was
just a research purpose hack.
MADV_FREE like hints might result in SEGV and thus of course should be avoided.
But, to my perspective, the 4 hints madvise_process() is currently supporting
(COLD, PAGEOUT, MERGEABLE, UNMERGEABLE) are not too risky even under the race.
That's why I said the incorrect information could be worth to be used under
some extreme situations.
>
> > I assume the users of this systemcall would understand the tradeoff and make
> > decisions.
>
> I disagree. My experience tells me that users tend to squeeze the
> maximum and beyond and hope they get what they want.
>
> > Also, as the users already have the right to do the tradeoff, I
> > think it's fair. In other words, I think the caller has both the power and the
> > responsibility to deal with the time-to-check-time-to-react problem.
> >
> > Nonetheless, I also agree this is important concern and the patch would be
> > better if it adds more detailed documentation regarding this issue.
>
> If there is _really_ a strong consensus that the racy interface is
> reasonable then it absolutely has to be described with a clearly state
> that those races might result in hard to predict behavior unless all
> tasks sharing the address space are blocked between the check and the
> madvise call.
So, it's still too risky to simply believe users to do the things well on their
responsibility, but a strong real consensus on needs and clear description
might justify this. I also agreed.
Thanks,
SeongJae Park
> --
> Michal Hocko
> SUSE Labs
>
^ 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