* [RFC PATCH v4 2/5] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux
From: Mathieu Desnoyers @ 2018-11-21 18:39 UTC (permalink / raw)
To: Carlos O'Donell
Cc: Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Mathieu Desnoyers, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, linux-kernel, linux-api
In-Reply-To: <20181121183936.8176-1-mathieu.desnoyers@efficios.com>
When available, use the cpu_id field from __rseq_abi on Linux to
implement sched_getcpu(). Fall-back on the vgetcpu vDSO if unavailable.
Benchmarks:
x86-64: Intel E5-2630 v3@2.40GHz, 16-core, hyperthreading
glibc sched_getcpu(): 13.7 ns (baseline)
glibc sched_getcpu() using rseq: 2.5 ns (speedup: 5.5x)
inline load cpuid from __rseq_abi TLS: 0.8 ns (speedup: 17.1x)
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
CC: Joseph Myers <joseph@codesourcery.com>
CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ben Maurer <bmaurer@fb.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Dave Watson <davejwatson@fb.com>
CC: Paul Turner <pjt@google.com>
CC: libc-alpha@sourceware.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
---
sysdeps/unix/sysv/linux/sched_getcpu.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/sysdeps/unix/sysv/linux/sched_getcpu.c b/sysdeps/unix/sysv/linux/sched_getcpu.c
index b69eeda15c..e1a206075c 100644
--- a/sysdeps/unix/sysv/linux/sched_getcpu.c
+++ b/sysdeps/unix/sysv/linux/sched_getcpu.c
@@ -24,8 +24,8 @@
#endif
#include <sysdep-vdso.h>
-int
-sched_getcpu (void)
+static int
+vsyscall_sched_getcpu (void)
{
#ifdef __NR_getcpu
unsigned int cpu;
@@ -37,3 +37,24 @@ sched_getcpu (void)
return -1;
#endif
}
+
+#ifdef __NR_rseq
+#include <linux/rseq.h>
+
+extern __attribute__ ((tls_model ("initial-exec")))
+__thread volatile struct rseq __rseq_abi;
+
+int
+sched_getcpu (void)
+{
+ int cpu_id = __rseq_abi.cpu_id;
+
+ return cpu_id >= 0 ? cpu_id : vsyscall_sched_getcpu ();
+}
+#else
+int
+sched_getcpu (void)
+{
+ return vsyscall_sched_getcpu ();
+}
+#endif
--
2.17.1
^ permalink raw reply related
* [PATCH] Add /proc/pid_generation
From: Daniel Colascione @ 2018-11-21 20:14 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: timmurray, primiano, joelaf, Daniel Colascione, Jonathan Corbet,
Andrew Morton, Mike Rapoport, Roman Gushchin, Vlastimil Babka,
Dennis Zhou (Facebook), Prashant Dhamdhere, Eric W. Biederman,
Steven Rostedt (VMware), Thomas Gleixner, Ingo Molnar,
Dominik Brodowski, Pavel Tatashin, Josh Poimboeuf, Ard Biesheuvel,
Michal Hocko, Matthew
Trace analysis code needs a coherent picture of the set of processes
and threads running on a system. While it's possible to enumerate all
tasks via /proc, this enumeration is not atomic. If PID numbering
rolls over during snapshot collection, the resulting snapshot of the
process and thread state of the system may be incoherent, confusing
trace analysis tools. The fundamental problem is that if a PID is
reused during a userspace scan of /proc, it's impossible to tell, in
post-processing, whether a fact that the userspace /proc scanner
reports regarding a given PID refers to the old or new task named by
that PID, as the scan of that PID may or may not have occurred before
the PID reuse, and there's no way to "stamp" a fact read from the
kernel with a trace timestamp.
This change adds a per-pid-namespace 64-bit generation number,
incremented on PID rollover, and exposes it via a new proc file
/proc/pid_generation. By examining this file before and after /proc
enumeration, user code can detect the potential reuse of a PID and
restart the task enumeration process, repeating until it gets a
coherent snapshot.
PID rollover ought to be rare, so in practice, scan repetitions will
be rare.
Signed-off-by: Daniel Colascione <dancol@google.com>
---
Documentation/filesystems/proc.txt | 1 +
include/linux/pid.h | 1 +
include/linux/pid_namespace.h | 2 ++
init/main.c | 1 +
kernel/pid.c | 36 +++++++++++++++++++++++++++++-
5 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 12a5e6e693b6..f58a359f9a2c 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -615,6 +615,7 @@ Table 1-5: Kernel info in /proc
partitions Table of partitions known to the system
pci Deprecated info of PCI bus (new way -> /proc/bus/pci/,
decoupled by lspci (2.4)
+ pid_gen PID rollover count
rtc Real time clock
scsi SCSI info (see text)
slabinfo Slab pool info
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 14a9a39da9c7..2e4b41a32e86 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -112,6 +112,7 @@ extern struct pid *find_ge_pid(int nr, struct pid_namespace *);
int next_pidmap(struct pid_namespace *pid_ns, unsigned int last);
extern struct pid *alloc_pid(struct pid_namespace *ns);
+extern u64 read_pid_generation(struct pid_namespace *ns);
extern void free_pid(struct pid *pid);
extern void disable_pid_allocation(struct pid_namespace *ns);
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 49538b172483..fa92ae66fb98 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -44,6 +44,7 @@ struct pid_namespace {
kgid_t pid_gid;
int hide_pid;
int reboot; /* group exit code if this pidns was rebooted */
+ u64 generation; /* incremented on wraparound */
struct ns_common ns;
} __randomize_layout;
@@ -99,5 +100,6 @@ static inline int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
extern struct pid_namespace *task_active_pid_ns(struct task_struct *tsk);
void pidhash_init(void);
void pid_idr_init(void);
+void pid_proc_init(void);
#endif /* _LINUX_PID_NS_H */
diff --git a/init/main.c b/init/main.c
index ee147103ba1b..20c595e852c6 100644
--- a/init/main.c
+++ b/init/main.c
@@ -730,6 +730,7 @@ asmlinkage __visible void __init start_kernel(void)
cgroup_init();
taskstats_init_early();
delayacct_init();
+ pid_proc_init();
check_bugs();
diff --git a/kernel/pid.c b/kernel/pid.c
index b2f6c506035d..cd5f4aa8eb55 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -174,6 +174,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
for (i = ns->level; i >= 0; i--) {
int pid_min = 1;
+ unsigned int old_cursor;
idr_preload(GFP_KERNEL);
spin_lock_irq(&pidmap_lock);
@@ -182,7 +183,8 @@ struct pid *alloc_pid(struct pid_namespace *ns)
* init really needs pid 1, but after reaching the maximum
* wrap back to RESERVED_PIDS
*/
- if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS)
+ old_cursor = idr_get_cursor(&tmp->idr);
+ if (old_cursor > RESERVED_PIDS)
pid_min = RESERVED_PIDS;
/*
@@ -191,6 +193,8 @@ struct pid *alloc_pid(struct pid_namespace *ns)
*/
nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min,
pid_max, GFP_ATOMIC);
+ if (unlikely(idr_get_cursor(&tmp->idr) <= old_cursor))
+ tmp->generation += 1;
spin_unlock_irq(&pidmap_lock);
idr_preload_end();
@@ -246,6 +250,16 @@ struct pid *alloc_pid(struct pid_namespace *ns)
return ERR_PTR(retval);
}
+u64 read_pid_generation(struct pid_namespace *ns)
+{
+ u64 generation;
+
+ spin_lock_irq(&pidmap_lock);
+ generation = ns->generation;
+ spin_unlock_irq(&pidmap_lock);
+ return generation;
+}
+
void disable_pid_allocation(struct pid_namespace *ns)
{
spin_lock_irq(&pidmap_lock);
@@ -449,6 +463,17 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
return idr_get_next(&ns->idr, &nr);
}
+#ifdef CONFIG_PROC_FS
+static int pid_generation_show(struct seq_file *m, void *v)
+{
+ u64 generation =
+ read_pid_generation(proc_pid_ns(file_inode(m->file)));
+ seq_printf(m, "%llu\n", generation);
+ return 0;
+
+};
+#endif
+
void __init pid_idr_init(void)
{
/* Verify no one has done anything silly: */
@@ -465,4 +490,13 @@ void __init pid_idr_init(void)
init_pid_ns.pid_cachep = KMEM_CACHE(pid,
SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
+
+}
+
+void __init pid_proc_init(void)
+{
+ /* pid_idr_init is too early, so get a separate init function. */
+#ifdef CONFIG_PROC_FS
+ WARN_ON(!proc_create_single("pid_gen", 0, NULL, pid_generation_show));
+#endif
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [PATCH] Add /proc/pid_generation
From: Matthew Wilcox @ 2018-11-21 20:31 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, linux-api, timmurray, primiano, joelaf,
Jonathan Corbet, Andrew Morton, Mike Rapoport, Roman Gushchin,
Vlastimil Babka, Dennis Zhou (Facebook), Prashant Dhamdhere,
Eric W. Biederman, Steven Rostedt (VMware), Thomas Gleixner,
Ingo Molnar, Dominik Brodowski, Pavel Tatashin, Josh Poimboeuf,
Ard Biesheuvel, Michal Hocko
In-Reply-To: <20181121201452.77173-1-dancol@google.com>
On Wed, Nov 21, 2018 at 12:14:44PM -0800, Daniel Colascione wrote:
> This change adds a per-pid-namespace 64-bit generation number,
> incremented on PID rollover, and exposes it via a new proc file
> /proc/pid_generation. By examining this file before and after /proc
> enumeration, user code can detect the potential reuse of a PID and
> restart the task enumeration process, repeating until it gets a
> coherent snapshot.
>
> PID rollover ought to be rare, so in practice, scan repetitions will
> be rare.
Then why does it need to be 64-bit?
^ permalink raw reply
* Re: [PATCH] Add /proc/pid_generation
From: Daniel Colascione @ 2018-11-21 20:38 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Andrew Morton, Mike Rapoport,
Roman Gushchin, Vlastimil Babka, Dennis Zhou (Facebook),
Prashant Dhamdhere, Eric W. Biederman, rostedt, tglx, mingo,
linux, pasha.tatashin, jpoimboe, ard.biesheuvel, Michal Hocko,
David Howells, ktsanaktsidis
In-Reply-To: <20181121203150.GK3065@bombadil.infradead.org>
On Wed, Nov 21, 2018 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Nov 21, 2018 at 12:14:44PM -0800, Daniel Colascione wrote:
> > This change adds a per-pid-namespace 64-bit generation number,
> > incremented on PID rollover, and exposes it via a new proc file
> > /proc/pid_generation. By examining this file before and after /proc
> > enumeration, user code can detect the potential reuse of a PID and
> > restart the task enumeration process, repeating until it gets a
> > coherent snapshot.
> >
> > PID rollover ought to be rare, so in practice, scan repetitions will
> > be rare.
>
> Then why does it need to be 64-bit?
[Resending because of accidental HTML. I really need to switch to a
better email client.]
Because 64 bits is enough for anyone. :-) A u64 is big enough that
we'll never observe an overflow on a running system, and PID
namespaces are rare enough that we won't miss the four extra bytes we
use by upgrading from a u32. And after reading about some security
problems caused by too-clever handling of 32-bit rollover, I'd rather
the code be obviously correct than save a trivial amount of space.
^ permalink raw reply
* [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-21 20:54 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: timmurray, primiano, joelaf, Daniel Colascione, Jonathan Corbet,
Andrew Morton, Mike Rapoport, Vlastimil Babka, Roman Gushchin,
Prashant Dhamdhere, Dennis Zhou (Facebook), Eric W. Biederman,
Steven Rostedt (VMware), Thomas Gleixner, Ingo Molnar,
Dominik Brodowski, Josh Poimboeuf, Ard Biesheuvel, Michal Hocko,
Stephen Rothwell
In-Reply-To: <20181121201452.77173-1-dancol@google.com>
Trace analysis code needs a coherent picture of the set of processes
and threads running on a system. While it's possible to enumerate all
tasks via /proc, this enumeration is not atomic. If PID numbering
rolls over during snapshot collection, the resulting snapshot of the
process and thread state of the system may be incoherent, confusing
trace analysis tools. The fundamental problem is that if a PID is
reused during a userspace scan of /proc, it's impossible to tell, in
post-processing, whether a fact that the userspace /proc scanner
reports regarding a given PID refers to the old or new task named by
that PID, as the scan of that PID may or may not have occurred before
the PID reuse, and there's no way to "stamp" a fact read from the
kernel with a trace timestamp.
This change adds a per-pid-namespace 64-bit generation number,
incremented on PID rollover, and exposes it via a new proc file
/proc/pid_gen. By examining this file before and after /proc
enumeration, user code can detect the potential reuse of a PID and
restart the task enumeration process, repeating until it gets a
coherent snapshot.
PID rollover ought to be rare, so in practice, scan repetitions will
be rare.
Signed-off-by: Daniel Colascione <dancol@google.com>
---
Make commit message match the code.
Documentation/filesystems/proc.txt | 1 +
include/linux/pid.h | 1 +
include/linux/pid_namespace.h | 2 ++
init/main.c | 1 +
kernel/pid.c | 36 +++++++++++++++++++++++++++++-
5 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 12a5e6e693b6..f58a359f9a2c 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -615,6 +615,7 @@ Table 1-5: Kernel info in /proc
partitions Table of partitions known to the system
pci Deprecated info of PCI bus (new way -> /proc/bus/pci/,
decoupled by lspci (2.4)
+ pid_gen PID rollover count
rtc Real time clock
scsi SCSI info (see text)
slabinfo Slab pool info
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 14a9a39da9c7..2e4b41a32e86 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -112,6 +112,7 @@ extern struct pid *find_ge_pid(int nr, struct pid_namespace *);
int next_pidmap(struct pid_namespace *pid_ns, unsigned int last);
extern struct pid *alloc_pid(struct pid_namespace *ns);
+extern u64 read_pid_generation(struct pid_namespace *ns);
extern void free_pid(struct pid *pid);
extern void disable_pid_allocation(struct pid_namespace *ns);
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 49538b172483..fa92ae66fb98 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -44,6 +44,7 @@ struct pid_namespace {
kgid_t pid_gid;
int hide_pid;
int reboot; /* group exit code if this pidns was rebooted */
+ u64 generation; /* incremented on wraparound */
struct ns_common ns;
} __randomize_layout;
@@ -99,5 +100,6 @@ static inline int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
extern struct pid_namespace *task_active_pid_ns(struct task_struct *tsk);
void pidhash_init(void);
void pid_idr_init(void);
+void pid_proc_init(void);
#endif /* _LINUX_PID_NS_H */
diff --git a/init/main.c b/init/main.c
index ee147103ba1b..20c595e852c6 100644
--- a/init/main.c
+++ b/init/main.c
@@ -730,6 +730,7 @@ asmlinkage __visible void __init start_kernel(void)
cgroup_init();
taskstats_init_early();
delayacct_init();
+ pid_proc_init();
check_bugs();
diff --git a/kernel/pid.c b/kernel/pid.c
index b2f6c506035d..cd5f4aa8eb55 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -174,6 +174,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
for (i = ns->level; i >= 0; i--) {
int pid_min = 1;
+ unsigned int old_cursor;
idr_preload(GFP_KERNEL);
spin_lock_irq(&pidmap_lock);
@@ -182,7 +183,8 @@ struct pid *alloc_pid(struct pid_namespace *ns)
* init really needs pid 1, but after reaching the maximum
* wrap back to RESERVED_PIDS
*/
- if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS)
+ old_cursor = idr_get_cursor(&tmp->idr);
+ if (old_cursor > RESERVED_PIDS)
pid_min = RESERVED_PIDS;
/*
@@ -191,6 +193,8 @@ struct pid *alloc_pid(struct pid_namespace *ns)
*/
nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min,
pid_max, GFP_ATOMIC);
+ if (unlikely(idr_get_cursor(&tmp->idr) <= old_cursor))
+ tmp->generation += 1;
spin_unlock_irq(&pidmap_lock);
idr_preload_end();
@@ -246,6 +250,16 @@ struct pid *alloc_pid(struct pid_namespace *ns)
return ERR_PTR(retval);
}
+u64 read_pid_generation(struct pid_namespace *ns)
+{
+ u64 generation;
+
+ spin_lock_irq(&pidmap_lock);
+ generation = ns->generation;
+ spin_unlock_irq(&pidmap_lock);
+ return generation;
+}
+
void disable_pid_allocation(struct pid_namespace *ns)
{
spin_lock_irq(&pidmap_lock);
@@ -449,6 +463,17 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
return idr_get_next(&ns->idr, &nr);
}
+#ifdef CONFIG_PROC_FS
+static int pid_generation_show(struct seq_file *m, void *v)
+{
+ u64 generation =
+ read_pid_generation(proc_pid_ns(file_inode(m->file)));
+ seq_printf(m, "%llu\n", generation);
+ return 0;
+
+};
+#endif
+
void __init pid_idr_init(void)
{
/* Verify no one has done anything silly: */
@@ -465,4 +490,13 @@ void __init pid_idr_init(void)
init_pid_ns.pid_cachep = KMEM_CACHE(pid,
SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
+
+}
+
+void __init pid_proc_init(void)
+{
+ /* pid_idr_init is too early, so get a separate init function. */
+#ifdef CONFIG_PROC_FS
+ WARN_ON(!proc_create_single("pid_gen", 0, NULL, pid_generation_show));
+#endif
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Serge E. Hallyn @ 2018-11-21 21:39 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, Daniel Colascione, Aleksa Sarai, linux-kernel,
Serge E. Hallyn, Jann Horn, Andy Lutomirski, Andrew Morton,
Oleg Nesterov, Al Viro, Linux FS Devel, Linux API, Tim Murray,
linux-man, Kees Cook
In-Reply-To: <20181120103111.etlqp7zop34v6nv4@brauner.io>
On Tue, Nov 20, 2018 at 11:31:13AM +0100, Christian Brauner wrote:
> On Mon, Nov 19, 2018 at 10:59:12PM -0600, Eric W. Biederman wrote:
> > Daniel Colascione <dancol@google.com> writes:
> >
> > > On Mon, Nov 19, 2018 at 1:37 PM Christian Brauner <christian@brauner.io> wrote:
> > >>
> > >> On Mon, Nov 19, 2018 at 01:26:22PM -0800, Daniel Colascione wrote:
> > >> > On Mon, Nov 19, 2018 at 1:21 PM, Christian Brauner <christian@brauner.io> wrote:
> > >> > > That can be done without a loop by comparing the level counter for the
> > >> > > two pid namespaces.
> > >> > >
> > >> > >>
> > >> > >> And you can rewrite pidns_get_parent to use it. So you would instead be
> > >> > >> doing:
> > >> > >>
> > >> > >> if (pidns_is_descendant(proc_pid_ns, task_active_pid_ns(current)))
> > >> > >> return -EPERM;
> > >> > >>
> > >> > >> (Or you can just copy the 5-line loop into procfd_signal -- though I
> > >> > >> imagine we'll need this for all of the procfd_* APIs.)
> > >> >
> > >> > Why is any of this even necessary? Why does the child namespace we're
> > >> > considering even have a file descriptor to its ancestor's procfs? If
> > >>
> > >> Because you can send file descriptors between processes and container
> > >> runtimes tend to do that.
> > >
> > > Right. But why *would* a container runtime send one of these procfs
> > > FDs to a container?
> > >
> > >> > it has one of these FDs, it can already *read* all sorts of
> > >> > information it really shouldn't be able to acquire, so the additional
> > >> > ability to send a signal (subject to the usual permission checks)
> > >> > feels like sticking a finger in a dike that's already well-perforated.
> > >> > IMHO, we shouldn't bother with this check. The patch would be simpler
> > >> > without it.
> > >>
> > >> We will definitely not allow signaling processes in an ancestor pid
> > >> namespace! That is a security issue! I can imagine container runtimes
> > >> killing their monitoring process etc. pp. Not happening, unless someone
> > >> with deep expertise in signals can convince me otherwise.
> > >
> > > If parent namespace procfs FDs or mounts really can leak into child
> > > namespaces as easily as Aleksa says, then I don't mind adding the
> > > check. I was under the impression that if you find yourself in this
> > > situation, you already have a big problem.
> >
> > There is one big reason to have the check, and I have not seen it
> > mentioned yet in this thread.
> >
> > When SI_USER is set we report the pid of the sender of the signal in
> > si_pid. When the signal comes from the kernel si_pid == 0. When signal
> > is sent from an ancestor pid namespace si_pid also equals 0 (which is
> > reasonable).
> >
> > A signal out to a process in a parent pid namespace such as SIGCHLD is
> > reasonable as we can map the pid. I really don't see the point of
> > forbidding that. From the perspective of the process in the parent pid
> > namespace it is just another process in it's pid namespace. So it
> > should pose no problem from the perspective of the receiving process.
> >
> > A signal to a process in a pid namespace that is neither a parent nor a
> > descendent pid namespace would be a problem, as there is no well defined
> > notion of what si_pid should be set to. So for that case perhaps we
> > should have something like a noprocess pid that we can set. Perhaps we
> > could set si_pid to 0xffffffff. That would take a small extension to
> > pid_nr_ns.
> >
> > File descriptors are not namespaced. It is completely legitimate to use
> > file descriptors to get around limitations of namespaces.
>
> Frankly, I don't see a good argument for why we would allow that even if
> safe. I have not heard a legitimate use-case or need for this.
> At this point I care about very simple semantics. Being able to signal
> into ancestor pid namespaces and cousin namespaces is interesting but
> makes the syscall more brittle and harder to understand.
Yeah, I'm with you on that. We can always open that door later if a good
use case comes up, but I prefer simple at first.
> Changing pid_nr_ns() might be the solution but this function is called
> all over the place in the kernel and I'm not going to risk breaking
> something by changing it for a feature that no one so far has ever
> asked for.
> If you are ok with this then we should hold off on this. We can always
> add this feature later by removing the check when someone has a use-case
> for it.
> I'll send a v2 of the patch that keeps the restriction for now. If you
> insist on it being removed we can make the change in a follow-up
> iteration.
>
> Christian
>
> >
> > Adding limitations to a file descriptor based api because someone else
> > can't set up their processes in such a way as to get the restrictions
> > they are looking for seems very sad.
> >
> > Frankly I think it is one of the better features of namespaces that we
> > have to carefully handle and define these cases so that when the
> > inevitable leaks happen you are not immediately in a world of hurt. All
> > of the other permission checks etc continue to do their job. Plus you
> > are prepared for the case when someone wants their containers to have an
> > interesting communication primitive.
> >
> > Eric
> >
> >
> >
> >
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Andrew Morton @ 2018-11-21 22:12 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, linux-api, timmurray, primiano, joelaf,
Jonathan Corbet, Mike Rapoport, Vlastimil Babka, Roman Gushchin,
Prashant Dhamdhere, Dennis Zhou (Facebook), Eric W. Biederman,
Steven Rostedt (VMware), Thomas Gleixner, Ingo Molnar,
Dominik Brodowski, Josh Poimboeuf, Ard Biesheuvel, Michal Hocko,
Stephen Rothwell, KJ Tsanaktsidis
In-Reply-To: <20181121205428.165205-1-dancol@google.com>
On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
> Trace analysis code needs a coherent picture of the set of processes
> and threads running on a system. While it's possible to enumerate all
> tasks via /proc, this enumeration is not atomic. If PID numbering
> rolls over during snapshot collection, the resulting snapshot of the
> process and thread state of the system may be incoherent, confusing
> trace analysis tools. The fundamental problem is that if a PID is
> reused during a userspace scan of /proc, it's impossible to tell, in
> post-processing, whether a fact that the userspace /proc scanner
> reports regarding a given PID refers to the old or new task named by
> that PID, as the scan of that PID may or may not have occurred before
> the PID reuse, and there's no way to "stamp" a fact read from the
> kernel with a trace timestamp.
>
> This change adds a per-pid-namespace 64-bit generation number,
> incremented on PID rollover, and exposes it via a new proc file
> /proc/pid_gen. By examining this file before and after /proc
> enumeration, user code can detect the potential reuse of a PID and
> restart the task enumeration process, repeating until it gets a
> coherent snapshot.
>
> PID rollover ought to be rare, so in practice, scan repetitions will
> be rare.
In general, tracing is a rather specialized thing. Why is this very
occasional confusion a sufficiently serious problem to warrant addition
of this code?
Which userspace tools will be using pid_gen? Are the developers of
those tools signed up to use pid_gen?
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -112,6 +112,7 @@ extern struct pid *find_ge_pid(int nr, struct pid_namespace *);
> int next_pidmap(struct pid_namespace *pid_ns, unsigned int last);
>
> extern struct pid *alloc_pid(struct pid_namespace *ns);
> +extern u64 read_pid_generation(struct pid_namespace *ns);
pig_generation_read() would be a better (and more idiomatic) name.
> extern void free_pid(struct pid *pid);
> extern void disable_pid_allocation(struct pid_namespace *ns);
>
> ...
>
> +u64 read_pid_generation(struct pid_namespace *ns)
> +{
> + u64 generation;
> +
> +
> + spin_lock_irq(&pidmap_lock);
> + generation = ns->generation;
> + spin_unlock_irq(&pidmap_lock);
> + return generation;
> +}
What is the spinlocking in here for? afaict the only purpose it serves
is to make the 64-bit read atomic, so it isn't needed on 32-bit?
> void disable_pid_allocation(struct pid_namespace *ns)
> {
> spin_lock_irq(&pidmap_lock);
> @@ -449,6 +463,17 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> return idr_get_next(&ns->idr, &nr);
> }
>
> +#ifdef CONFIG_PROC_FS
> +static int pid_generation_show(struct seq_file *m, void *v)
> +{
> + u64 generation =
> + read_pid_generation(proc_pid_ns(file_inode(m->file)));
u64 generation;
generation = read_pid_generation(proc_pid_ns(file_inode(m->file)));
is a nicer way of avoiding column wrap.
> + seq_printf(m, "%llu\n", generation);
> + return 0;
> +
> +};
> +#endif
> +
> void __init pid_idr_init(void)
> {
> /* Verify no one has done anything silly: */
> @@ -465,4 +490,13 @@ void __init pid_idr_init(void)
>
> init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> +
> +}
> +
> +void __init pid_proc_init(void)
> +{
> + /* pid_idr_init is too early, so get a separate init function. */
s/get a/use a/
> +#ifdef CONFIG_PROC_FS
> + WARN_ON(!proc_create_single("pid_gen", 0, NULL, pid_generation_show));
> +#endif
> }
This whole function could vanish if !CONFIG_PROC_FS. Doesn't matter
much with __init code though.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-21 22:40 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
ard.biesheuvel, Michal Hocko, sfr, ktsanaktsidis, David Howells,
open list:DOCUMENTATION
In-Reply-To: <20181121141220.0e533c1dcb4792480efbf3ff@linux-foundation.org>
On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
>
> > Trace analysis code needs a coherent picture of the set of processes
> > and threads running on a system. While it's possible to enumerate all
> > tasks via /proc, this enumeration is not atomic. If PID numbering
> > rolls over during snapshot collection, the resulting snapshot of the
> > process and thread state of the system may be incoherent, confusing
> > trace analysis tools. The fundamental problem is that if a PID is
> > reused during a userspace scan of /proc, it's impossible to tell, in
> > post-processing, whether a fact that the userspace /proc scanner
> > reports regarding a given PID refers to the old or new task named by
> > that PID, as the scan of that PID may or may not have occurred before
> > the PID reuse, and there's no way to "stamp" a fact read from the
> > kernel with a trace timestamp.
> >
> > This change adds a per-pid-namespace 64-bit generation number,
> > incremented on PID rollover, and exposes it via a new proc file
> > /proc/pid_gen. By examining this file before and after /proc
> > enumeration, user code can detect the potential reuse of a PID and
> > restart the task enumeration process, repeating until it gets a
> > coherent snapshot.
> >
> > PID rollover ought to be rare, so in practice, scan repetitions will
> > be rare.
>
> In general, tracing is a rather specialized thing. Why is this very
> occasional confusion a sufficiently serious problem to warrant addition
> of this code?
I wouldn't call tracing a specialized thing: it's important enough to
justify its own summit and a whole ecosystem of trace collection and
analysis tools. We use it in every day in Android. It's tremendously
helpful for understanding system behavior, especially in cases where
multiple components interact in ways that we can't readily predict or
replicate. Reliability and precision in this area are essential:
retrospective analysis of difficult-to-reproduce problems involves
puzzling over trace files and testing hypothesis, and when the trace
system itself is occasionally unreliable, the set of hypothesis to
consider grows. I've tried to keep the amount of kernel infrastructure
needed to support this precision and reliability to a minimum, pushing
most of the complexity to userspace. But we do need, from the kernel,
reliable process disambiguation.
Besides: things like checkpoint and restart are also non-core
features, but the kernel has plenty of infrastructure to support them.
We're talking about a very lightweight feature in this thread.
> Which userspace tools will be using pid_gen? Are the developers of
> those tools signed up to use pid_gen?
I'll be changing Android tracing tools to capture process snapshots
using pid_gen, using the algorithm in the commit message.
> > --- a/include/linux/pid.h
> > +++ b/include/linux/pid.h
> > @@ -112,6 +112,7 @@ extern struct pid *find_ge_pid(int nr, struct pid_namespace *);
> > int next_pidmap(struct pid_namespace *pid_ns, unsigned int last);
> >
> > extern struct pid *alloc_pid(struct pid_namespace *ns);
> > +extern u64 read_pid_generation(struct pid_namespace *ns);
>
> pig_generation_read() would be a better (and more idiomatic) name.
Thanks. I'll change it.
>
> > extern void free_pid(struct pid *pid);
> > extern void disable_pid_allocation(struct pid_namespace *ns);
> >
> > ...
> >
> > +u64 read_pid_generation(struct pid_namespace *ns)
> > +{
> > + u64 generation;
> > +
> > +
> > + spin_lock_irq(&pidmap_lock);
> > + generation = ns->generation;
> > + spin_unlock_irq(&pidmap_lock);
> > + return generation;
> > +}
>
> What is the spinlocking in here for? afaict the only purpose it serves
> is to make the 64-bit read atomic, so it isn't needed on 32-bit?
ITYM the spinlock is necessary *only* on 32-bit, since 64-bit
architectures have atomic 64-bit reads, and 64-bit reads on 32-bit
architectures can tear. This function isn't a particularly hot path,
so I thought consistency across architectures would be more valuable
than avoiding the lock on some systems.
> > void disable_pid_allocation(struct pid_namespace *ns)
> > {
> > spin_lock_irq(&pidmap_lock);
> > @@ -449,6 +463,17 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> > return idr_get_next(&ns->idr, &nr);
> > }
> >
> > +#ifdef CONFIG_PROC_FS
> > +static int pid_generation_show(struct seq_file *m, void *v)
> > +{
> > + u64 generation =
> > + read_pid_generation(proc_pid_ns(file_inode(m->file)));
>
> u64 generation;
>
> generation = read_pid_generation(proc_pid_ns(file_inode(m->file)));
>
> is a nicer way of avoiding column wrap.
Sure.
> > + seq_printf(m, "%llu\n", generation);
> > + return 0;
> > +
> > +};
> > +#endif
> > +
> > void __init pid_idr_init(void)
> > {
> > /* Verify no one has done anything silly: */
> > @@ -465,4 +490,13 @@ void __init pid_idr_init(void)
> >
> > init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> > SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> > +
> > +}
> > +
> > +void __init pid_proc_init(void)
> > +{
> > + /* pid_idr_init is too early, so get a separate init function. */
>
> s/get a/use a/
Will change.
> > +#ifdef CONFIG_PROC_FS
> > + WARN_ON(!proc_create_single("pid_gen", 0, NULL, pid_generation_show));
> > +#endif
> > }
>
> This whole function could vanish if !CONFIG_PROC_FS. Doesn't matter
> much with __init code though.
I wanted to keep the ifdefed region as small as possible. I wonder
whether LTO is good enough to make the function and its call site
disappear entirely in configurations where it has an empty body.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Jann Horn @ 2018-11-21 22:48 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Morton, kernel list, Linux API, Tim Murray, primiano,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
guro, pdhamdhe, dennisszhou, Eric W. Biederman, Steven Rostedt,
Thomas Gleixner, Ingo Molnar, linux, Josh Poimboeuf,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David
In-Reply-To: <CAKOZuetW6WMNFdL5_2=UbrEf8mjxYTdz1+4BiRLSgdoKh8FsqQ@mail.gmail.com>
On Wed, Nov 21, 2018 at 11:40 PM Daniel Colascione <dancol@google.com> wrote:
> On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
> > > +u64 read_pid_generation(struct pid_namespace *ns)
> > > +{
> > > + u64 generation;
> > > +
> > > +
> > > + spin_lock_irq(&pidmap_lock);
> > > + generation = ns->generation;
> > > + spin_unlock_irq(&pidmap_lock);
> > > + return generation;
> > > +}
> >
> > What is the spinlocking in here for? afaict the only purpose it serves
> > is to make the 64-bit read atomic, so it isn't needed on 32-bit?
>
> ITYM the spinlock is necessary *only* on 32-bit, since 64-bit
> architectures have atomic 64-bit reads, and 64-bit reads on 32-bit
> architectures can tear. This function isn't a particularly hot path,
> so I thought consistency across architectures would be more valuable
> than avoiding the lock on some systems.
Linux has atomic64_t/atomic64_read()/atomic64_inc() for this, which
should automatically do the right thing - processor-supported atomic
ops when possible, spinlock otherwise.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Andrew Morton @ 2018-11-21 22:50 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
ard.biesheuvel, Michal Hocko, sfr, ktsanaktsidis, David Howells,
open list:DOCUMENTATION
In-Reply-To: <CAKOZuetW6WMNFdL5_2=UbrEf8mjxYTdz1+4BiRLSgdoKh8FsqQ@mail.gmail.com>
On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
> On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
> >
> > > Trace analysis code needs a coherent picture of the set of processes
> > > and threads running on a system. While it's possible to enumerate all
> > > tasks via /proc, this enumeration is not atomic. If PID numbering
> > > rolls over during snapshot collection, the resulting snapshot of the
> > > process and thread state of the system may be incoherent, confusing
> > > trace analysis tools. The fundamental problem is that if a PID is
> > > reused during a userspace scan of /proc, it's impossible to tell, in
> > > post-processing, whether a fact that the userspace /proc scanner
> > > reports regarding a given PID refers to the old or new task named by
> > > that PID, as the scan of that PID may or may not have occurred before
> > > the PID reuse, and there's no way to "stamp" a fact read from the
> > > kernel with a trace timestamp.
> > >
> > > This change adds a per-pid-namespace 64-bit generation number,
> > > incremented on PID rollover, and exposes it via a new proc file
> > > /proc/pid_gen. By examining this file before and after /proc
> > > enumeration, user code can detect the potential reuse of a PID and
> > > restart the task enumeration process, repeating until it gets a
> > > coherent snapshot.
> > >
> > > PID rollover ought to be rare, so in practice, scan repetitions will
> > > be rare.
> >
> > In general, tracing is a rather specialized thing. Why is this very
> > occasional confusion a sufficiently serious problem to warrant addition
> > of this code?
>
> I wouldn't call tracing a specialized thing: it's important enough to
> justify its own summit and a whole ecosystem of trace collection and
> analysis tools. We use it in every day in Android. It's tremendously
> helpful for understanding system behavior, especially in cases where
> multiple components interact in ways that we can't readily predict or
> replicate. Reliability and precision in this area are essential:
> retrospective analysis of difficult-to-reproduce problems involves
> puzzling over trace files and testing hypothesis, and when the trace
> system itself is occasionally unreliable, the set of hypothesis to
> consider grows. I've tried to keep the amount of kernel infrastructure
> needed to support this precision and reliability to a minimum, pushing
> most of the complexity to userspace. But we do need, from the kernel,
> reliable process disambiguation.
>
> Besides: things like checkpoint and restart are also non-core
> features, but the kernel has plenty of infrastructure to support them.
> We're talking about a very lightweight feature in this thread.
I'm still not understanding the seriousness of the problem. Presumably
you've hit problems in real-life which were serious and frequent enough
to justify getting down and writing the code. Please share some sob stories
with us!
> > Which userspace tools will be using pid_gen? Are the developers of
> > those tools signed up to use pid_gen?
>
> I'll be changing Android tracing tools to capture process snapshots
> using pid_gen, using the algorithm in the commit message.
Which other tools could use this and what was the feedback from their
developers? Those people are the intended audience and the
best-positioned reviewers so let's hear from them?
> > > +u64 read_pid_generation(struct pid_namespace *ns)
> > > +{
> > > + u64 generation;
> > > +
> > > +
> > > + spin_lock_irq(&pidmap_lock);
> > > + generation = ns->generation;
> > > + spin_unlock_irq(&pidmap_lock);
> > > + return generation;
> > > +}
> >
> > What is the spinlocking in here for? afaict the only purpose it serves
> > is to make the 64-bit read atomic, so it isn't needed on 32-bit?
>
> ITYM the spinlock is necessary *only* on 32-bit, since 64-bit
> architectures have atomic 64-bit reads, and 64-bit reads on 32-bit
> architectures can tear. This function isn't a particularly hot path,
> so I thought consistency across architectures would be more valuable
> than avoiding the lock on some systems.
OK.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-21 22:52 UTC (permalink / raw)
To: Jann Horn
Cc: Andrew Morton, linux-kernel, Linux API, Tim Murray,
Primiano Tucci, Joel Fernandes, Jonathan Corbet, Mike Rapoport,
Vlastimil Babka, Roman Gushchin, Prashant Dhamdhere,
Dennis Zhou (Facebook), Eric W. Biederman, rostedt, tglx, mingo,
linux, jpoimboe, Ard Biesheuvel, Michal Hocko, sfr, ktsanaktsidis,
David Howells
In-Reply-To: <CAG48ez3kgFC74jTmxqL5aPgmqcN-W4E9bR7Fz3V=Vt4qkpx2Pg@mail.gmail.com>
On Wed, Nov 21, 2018 at 2:49 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Nov 21, 2018 at 11:40 PM Daniel Colascione <dancol@google.com> wrote:
> > On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
> > > > +u64 read_pid_generation(struct pid_namespace *ns)
> > > > +{
> > > > + u64 generation;
> > > > +
> > > > +
> > > > + spin_lock_irq(&pidmap_lock);
> > > > + generation = ns->generation;
> > > > + spin_unlock_irq(&pidmap_lock);
> > > > + return generation;
> > > > +}
> > >
> > > What is the spinlocking in here for? afaict the only purpose it serves
> > > is to make the 64-bit read atomic, so it isn't needed on 32-bit?
> >
> > ITYM the spinlock is necessary *only* on 32-bit, since 64-bit
> > architectures have atomic 64-bit reads, and 64-bit reads on 32-bit
> > architectures can tear. This function isn't a particularly hot path,
> > so I thought consistency across architectures would be more valuable
> > than avoiding the lock on some systems.
>
> Linux has atomic64_t/atomic64_read()/atomic64_inc() for this, which
> should automatically do the right thing - processor-supported atomic
> ops when possible, spinlock otherwise.
I wanted to take advantage of the existing spinlock to synchronize
instead of adding more atomic operations to the rollover path.
^ permalink raw reply
* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Andy Lutomirski @ 2018-11-21 22:56 UTC (permalink / raw)
To: Elvira Khabirova, Kees Cook, Sasha Levin, Linux API, Jann Horn
Cc: Oleg Nesterov, Steven Rostedt, Ingo Molnar, LKML, Dmitry V. Levin,
Eugene Syromiatnikov, Andrew Lutomirski, strace-devel
In-Reply-To: <20181121165806.07da7c98@akathisia>
Please cc linux-api@vger.kernel.org for future versions.
On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova
<lineprinter@altlinux.org> wrote:
>
> struct ptrace_syscall_info {
> __u8 op; /* 0 for entry, 1 for exit */
Can you add proper defines, like:
#define PTRACE_SYSCALL_ENTRY 0
#define PTRACE_SYSCALL_EXIT 1
#define PTRACE_SYSCALL_SECCOMP 2
and make seccomp work from the start? I'd rather we don't merge an
implementation that doesn't work for seccomp and then have to rework
it later.
> __u8 __pad0[7];
> union {
> struct {
> __s32 nr;
__u64 please. Syscall numbers are, as a practical matter, 64 bits.
Admittedly, the actual effects of setting the high bits are unclear,
and seccomp has issues with it, but let's not perpetuate the problem.
> __u32 arch;
> __u64 instruction_pointer;
> __u64 args[6];
> } entry_info;
> struct {
> __s64 rval;
> __u8 is_error;
> __u8 __pad1[7];
> } exit_info;
> };
> };
Should seccomp events use entry_info or should they just literally
supply seccomp_data?
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-21 23:21 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <20181121145043.fa029f4f91afddc2a10bb81e@linux-foundation.org>
On Wed, Nov 21, 2018 at 2:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
>
> > On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
> > >
> > > > Trace analysis code needs a coherent picture of the set of processes
> > > > and threads running on a system. While it's possible to enumerate all
> > > > tasks via /proc, this enumeration is not atomic. If PID numbering
> > > > rolls over during snapshot collection, the resulting snapshot of the
> > > > process and thread state of the system may be incoherent, confusing
> > > > trace analysis tools. The fundamental problem is that if a PID is
> > > > reused during a userspace scan of /proc, it's impossible to tell, in
> > > > post-processing, whether a fact that the userspace /proc scanner
> > > > reports regarding a given PID refers to the old or new task named by
> > > > that PID, as the scan of that PID may or may not have occurred before
> > > > the PID reuse, and there's no way to "stamp" a fact read from the
> > > > kernel with a trace timestamp.
> > > >
> > > > This change adds a per-pid-namespace 64-bit generation number,
> > > > incremented on PID rollover, and exposes it via a new proc file
> > > > /proc/pid_gen. By examining this file before and after /proc
> > > > enumeration, user code can detect the potential reuse of a PID and
> > > > restart the task enumeration process, repeating until it gets a
> > > > coherent snapshot.
> > > >
> > > > PID rollover ought to be rare, so in practice, scan repetitions will
> > > > be rare.
> > >
> > > In general, tracing is a rather specialized thing. Why is this very
> > > occasional confusion a sufficiently serious problem to warrant addition
> > > of this code?
> >
> > I wouldn't call tracing a specialized thing: it's important enough to
> > justify its own summit and a whole ecosystem of trace collection and
> > analysis tools. We use it in every day in Android. It's tremendously
> > helpful for understanding system behavior, especially in cases where
> > multiple components interact in ways that we can't readily predict or
> > replicate. Reliability and precision in this area are essential:
> > retrospective analysis of difficult-to-reproduce problems involves
> > puzzling over trace files and testing hypothesis, and when the trace
> > system itself is occasionally unreliable, the set of hypothesis to
> > consider grows. I've tried to keep the amount of kernel infrastructure
> > needed to support this precision and reliability to a minimum, pushing
> > most of the complexity to userspace. But we do need, from the kernel,
> > reliable process disambiguation.
> >
> > Besides: things like checkpoint and restart are also non-core
> > features, but the kernel has plenty of infrastructure to support them.
> > We're talking about a very lightweight feature in this thread.
>
> I'm still not understanding the seriousness of the problem. Presumably
> you've hit problems in real-life which were serious and frequent enough
> to justify getting down and writing the code. Please share some sob stories
> with us!
The problem here is the possibility of confusion, even if it's rare.
Does the naive approach of just walking /proc and ignoring the
possibility of PID reuse races work most of the time? Sure. But "most
of the time" isn't good enough. It's not that there are tons of sob
stories: it's that without completely robust reporting, we can't rule
out of the possibility that weirdness we observe in a given trace is
actually just an artifact from a kinda-sort-working best-effort trace
collection system instead of a real anomaly in behavior. Tracing,
essentially, gives us deltas for system state, and without an accurate
baseline, collected via some kind of scan on trace startup, it's
impossible to use these deltas to robustly reconstruct total system
state at a given time. And this matters, because errors in
reconstruction (e.g., assigning a thread to the wrong process because
the IDs happen to be reused) can affect processing of the whole trace.
If it's 3am and I'm analyzing the lone trace from a dogfooder
demonstrating a particularly nasty problem, I don't want to find out
that the trace I'm analyzing ended up being useless because the
kernel's trace system is merely best effort. It's very cheap to be
100% reliable here, so let's be reliable and rule out sources of
error.
> > > Which userspace tools will be using pid_gen? Are the developers of
> > > those tools signed up to use pid_gen?
> >
> > I'll be changing Android tracing tools to capture process snapshots
> > using pid_gen, using the algorithm in the commit message.
>
> Which other tools could use this and what was the feedback from their
> developers?
I'm going to have Android's systrace and Perfetto use this approach.
Exactly how many tools signed up to use this feature do you need?
> Those people are the intended audience and the
> best-positioned reviewers so let's hear from them?
I'm writing plenty of trace analysis tools myself, so I'm part of this
intended audience. Other tracing tool authors have told me about
out-of-tree hacks for process atomic snapshots via ftrace events. This
approach avoids the necessity of these more-invasive hacks.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Andy Lutomirski @ 2018-11-21 23:35 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Morton, linux-kernel, Linux API, Tim Murray,
Primiano Tucci, Joel Fernandes, Jonathan Corbet, Mike Rapoport,
Vlastimil Babka, Roman Gushchin, Prashant Dhamdhere,
Dennis Zhou (Facebook), Eric W. Biederman, rostedt, tglx, mingo,
linux, jpoimboe, Ard Biesheuvel, Michal Hocko, Stephen Rothwell,
ktsanaktsidis, Davi
In-Reply-To: <CAKOZuesF5zi9mpEE6No7oq0hDnM8KFhQFU=NM7qUF635s0r3-A@mail.gmail.com>
> On Nov 21, 2018, at 4:21 PM, Daniel Colascione <dancol@google.com> wrote:
>
>> On Wed, Nov 21, 2018 at 2:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>>> On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
>>>
>>>> On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>>>
>>>>> On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
>>>>>
>>>>> Trace analysis code needs a coherent picture of the set of processes
>>>>> and threads running on a system. While it's possible to enumerate all
>>>>> tasks via /proc, this enumeration is not atomic. If PID numbering
>>>>> rolls over during snapshot collection, the resulting snapshot of the
>>>>> process and thread state of the system may be incoherent, confusing
>>>>> trace analysis tools. The fundamental problem is that if a PID is
>>>>> reused during a userspace scan of /proc, it's impossible to tell, in
>>>>> post-processing, whether a fact that the userspace /proc scanner
>>>>> reports regarding a given PID refers to the old or new task named by
>>>>> that PID, as the scan of that PID may or may not have occurred before
>>>>> the PID reuse, and there's no way to "stamp" a fact read from the
>>>>> kernel with a trace timestamp.
>>>>>
>>>>> This change adds a per-pid-namespace 64-bit generation number,
>>>>> incremented on PID rollover, and exposes it via a new proc file
>>>>> /proc/pid_gen. By examining this file before and after /proc
>>>>> enumeration, user code can detect the potential reuse of a PID and
>>>>> restart the task enumeration process, repeating until it gets a
>>>>> coherent snapshot.
>>>>>
>>>>> PID rollover ought to be rare, so in practice, scan repetitions will
>>>>> be rare.
>>>>
>>>> In general, tracing is a rather specialized thing. Why is this very
>>>> occasional confusion a sufficiently serious problem to warrant addition
>>>> of this code?
>>>
>>> I wouldn't call tracing a specialized thing: it's important enough to
>>> justify its own summit and a whole ecosystem of trace collection and
>>> analysis tools. We use it in every day in Android. It's tremendously
>>> helpful for understanding system behavior, especially in cases where
>>> multiple components interact in ways that we can't readily predict or
>>> replicate. Reliability and precision in this area are essential:
>>> retrospective analysis of difficult-to-reproduce problems involves
>>> puzzling over trace files and testing hypothesis, and when the trace
>>> system itself is occasionally unreliable, the set of hypothesis to
>>> consider grows. I've tried to keep the amount of kernel infrastructure
>>> needed to support this precision and reliability to a minimum, pushing
>>> most of the complexity to userspace. But we do need, from the kernel,
>>> reliable process disambiguation.
>>>
>>> Besides: things like checkpoint and restart are also non-core
>>> features, but the kernel has plenty of infrastructure to support them.
>>> We're talking about a very lightweight feature in this thread.
>>
>> I'm still not understanding the seriousness of the problem. Presumably
>> you've hit problems in real-life which were serious and frequent enough
>> to justify getting down and writing the code. Please share some sob stories
>> with us!
>
> The problem here is the possibility of confusion, even if it's rare.
> Does the naive approach of just walking /proc and ignoring the
> possibility of PID reuse races work most of the time? Sure. But "most
> of the time" isn't good enough. It's not that there are tons of sob
> stories: it's that without completely robust reporting, we can't rule
> out of the possibility that weirdness we observe in a given trace is
> actually just an artifact from a kinda-sort-working best-effort trace
> collection system instead of a real anomaly in behavior. Tracing,
> essentially, gives us deltas for system state, and without an accurate
> baseline, collected via some kind of scan on trace startup, it's
> impossible to use these deltas to robustly reconstruct total system
> state at a given time. And this matters, because errors in
> reconstruction (e.g., assigning a thread to the wrong process because
> the IDs happen to be reused) can affect processing of the whole trace.
> If it's 3am and I'm analyzing the lone trace from a dogfooder
> demonstrating a particularly nasty problem, I don't want to find out
> that the trace I'm analyzing ended up being useless because the
> kernel's trace system is merely best effort. It's very cheap to be
> 100% reliable here, so let's be reliable and rule out sources of
> error.
>
>>>> Which userspace tools will be using pid_gen? Are the developers of
>>>> those tools signed up to use pid_gen?
>>>
>>> I'll be changing Android tracing tools to capture process snapshots
>>> using pid_gen, using the algorithm in the commit message.
>>
>> Which other tools could use this and what was the feedback from their
>> developers?
>
> I'm going to have Android's systrace and Perfetto use this approach.
> Exactly how many tools signed up to use this feature do you need?
>
>> Those people are the intended audience and the
>> best-positioned reviewers so let's hear from them?
>
> I'm writing plenty of trace analysis tools myself, so I'm part of this
> intended audience. Other tracing tool authors have told me about
> out-of-tree hacks for process atomic snapshots via ftrace events. This
> approach avoids the necessity of these more-invasive hacks.
Would a tracepoint for pid reuse solve your problem?
^ permalink raw reply
* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-11-21 23:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
Oleg Nesterov, Steven Rostedt, LKML, Sasha Levin, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <CALCETrV3TPCkYyhoLLcikXVeF-RdZUuLTCvKReK3Qb9LSS9Csw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 2194 bytes --]
On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
>
> On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> >
> > struct ptrace_syscall_info {
> > __u8 op; /* 0 for entry, 1 for exit */
>
> Can you add proper defines, like:
>
> #define PTRACE_SYSCALL_ENTRY 0
> #define PTRACE_SYSCALL_EXIT 1
> #define PTRACE_SYSCALL_SECCOMP 2
>
> and make seccomp work from the start? I'd rather we don't merge an
> implementation that doesn't work for seccomp and then have to rework
> it later.
What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
with regards to PTRACE_GET_SYSCALL_INFO request? At least they have the
same entry_info to return.
As long as implementation (ab)uses ptrace_message to tell one kind of stop
from another, it can distinguish syscall-entry-stop and syscall-exit-stop
from each other and from many other kinds of stops, but it cannot
distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.
> > __u8 __pad0[7];
> > union {
> > struct {
> > __s32 nr;
>
> __u64 please. Syscall numbers are, as a practical matter, 64 bits.
> Admittedly, the actual effects of setting the high bits are unclear,
> and seccomp has issues with it, but let's not perpetuate the problem.
I agree. Although the implementation uses syscall_get_nr()
which returns int, this could potentially be fixed in the future.
> > __u32 arch;
> > __u64 instruction_pointer;
> > __u64 args[6];
> > } entry_info;
> > struct {
> > __s64 rval;
> > __u8 is_error;
> > __u8 __pad1[7];
> > } exit_info;
> > };
> > };
>
> Should seccomp events use entry_info or should they just literally
> supply seccomp_data?
It certainly can use entry_info.
I'd prefer to avoid using in uapi/linux/ptrace.h those types
that are defined in uapi/linux/seccomp.h.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-22 0:21 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Morton, linux-kernel, Linux API, Tim Murray,
Primiano Tucci, Joel Fernandes, Jonathan Corbet, Mike Rapoport,
Vlastimil Babka, Roman Gushchin, Prashant Dhamdhere,
Dennis Zhou (Facebook), Eric W. Biederman, rostedt, tglx, mingo,
linux, jpoimboe, Ard Biesheuvel, Michal Hocko, Stephen Rothwell,
ktsanaktsidis, Davi
In-Reply-To: <37255927-1A93-4B8B-A916-B5A3983D56B6@amacapital.net>
On Wed, Nov 21, 2018 at 3:35 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > On Nov 21, 2018, at 4:21 PM, Daniel Colascione <dancol@google.com> wrote:
> >
> >> On Wed, Nov 21, 2018 at 2:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >>
> >>> On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
> >>>
> >>>> On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >>>>
> >>>>> On Wed, 21 Nov 2018 12:54:20 -0800 Daniel Colascione <dancol@google.com> wrote:
> >>>>>
> >>>>> Trace analysis code needs a coherent picture of the set of processes
> >>>>> and threads running on a system. While it's possible to enumerate all
> >>>>> tasks via /proc, this enumeration is not atomic. If PID numbering
> >>>>> rolls over during snapshot collection, the resulting snapshot of the
> >>>>> process and thread state of the system may be incoherent, confusing
> >>>>> trace analysis tools. The fundamental problem is that if a PID is
> >>>>> reused during a userspace scan of /proc, it's impossible to tell, in
> >>>>> post-processing, whether a fact that the userspace /proc scanner
> >>>>> reports regarding a given PID refers to the old or new task named by
> >>>>> that PID, as the scan of that PID may or may not have occurred before
> >>>>> the PID reuse, and there's no way to "stamp" a fact read from the
> >>>>> kernel with a trace timestamp.
> >>>>>
> >>>>> This change adds a per-pid-namespace 64-bit generation number,
> >>>>> incremented on PID rollover, and exposes it via a new proc file
> >>>>> /proc/pid_gen. By examining this file before and after /proc
> >>>>> enumeration, user code can detect the potential reuse of a PID and
> >>>>> restart the task enumeration process, repeating until it gets a
> >>>>> coherent snapshot.
> >>>>>
> >>>>> PID rollover ought to be rare, so in practice, scan repetitions will
> >>>>> be rare.
> >>>>
> >>>> In general, tracing is a rather specialized thing. Why is this very
> >>>> occasional confusion a sufficiently serious problem to warrant addition
> >>>> of this code?
> >>>
> >>> I wouldn't call tracing a specialized thing: it's important enough to
> >>> justify its own summit and a whole ecosystem of trace collection and
> >>> analysis tools. We use it in every day in Android. It's tremendously
> >>> helpful for understanding system behavior, especially in cases where
> >>> multiple components interact in ways that we can't readily predict or
> >>> replicate. Reliability and precision in this area are essential:
> >>> retrospective analysis of difficult-to-reproduce problems involves
> >>> puzzling over trace files and testing hypothesis, and when the trace
> >>> system itself is occasionally unreliable, the set of hypothesis to
> >>> consider grows. I've tried to keep the amount of kernel infrastructure
> >>> needed to support this precision and reliability to a minimum, pushing
> >>> most of the complexity to userspace. But we do need, from the kernel,
> >>> reliable process disambiguation.
> >>>
> >>> Besides: things like checkpoint and restart are also non-core
> >>> features, but the kernel has plenty of infrastructure to support them.
> >>> We're talking about a very lightweight feature in this thread.
> >>
> >> I'm still not understanding the seriousness of the problem. Presumably
> >> you've hit problems in real-life which were serious and frequent enough
> >> to justify getting down and writing the code. Please share some sob stories
> >> with us!
> >
> > The problem here is the possibility of confusion, even if it's rare.
> > Does the naive approach of just walking /proc and ignoring the
> > possibility of PID reuse races work most of the time? Sure. But "most
> > of the time" isn't good enough. It's not that there are tons of sob
> > stories: it's that without completely robust reporting, we can't rule
> > out of the possibility that weirdness we observe in a given trace is
> > actually just an artifact from a kinda-sort-working best-effort trace
> > collection system instead of a real anomaly in behavior. Tracing,
> > essentially, gives us deltas for system state, and without an accurate
> > baseline, collected via some kind of scan on trace startup, it's
> > impossible to use these deltas to robustly reconstruct total system
> > state at a given time. And this matters, because errors in
> > reconstruction (e.g., assigning a thread to the wrong process because
> > the IDs happen to be reused) can affect processing of the whole trace.
> > If it's 3am and I'm analyzing the lone trace from a dogfooder
> > demonstrating a particularly nasty problem, I don't want to find out
> > that the trace I'm analyzing ended up being useless because the
> > kernel's trace system is merely best effort. It's very cheap to be
> > 100% reliable here, so let's be reliable and rule out sources of
> > error.
> >
> >>>> Which userspace tools will be using pid_gen? Are the developers of
> >>>> those tools signed up to use pid_gen?
> >>>
> >>> I'll be changing Android tracing tools to capture process snapshots
> >>> using pid_gen, using the algorithm in the commit message.
> >>
> >> Which other tools could use this and what was the feedback from their
> >> developers?
> >
> > I'm going to have Android's systrace and Perfetto use this approach.
> > Exactly how many tools signed up to use this feature do you need?
> >
> >> Those people are the intended audience and the
> >> best-positioned reviewers so let's hear from them?
> >
> > I'm writing plenty of trace analysis tools myself, so I'm part of this
> > intended audience. Other tracing tool authors have told me about
> > out-of-tree hacks for process atomic snapshots via ftrace events. This
> > approach avoids the necessity of these more-invasive hacks.
>
> Would a tracepoint for pid reuse solve your problem?
I initially thought "no", but maybe it would after all.
The /proc scanner would need some way of consuming this tracepoint so
that it would know to re-run the scan. For some tracing systems (like
Perfetto) doing simultaneous event consumption and recording would
work (although it'd be awkward), but other systems (like things based
on the simpler atrace framework) aren't able to process events, and
inferring PID reuse after the fact it's as helpful as being able to
re-scan in response to PID rollover.
OTOH, the ftrace histogram stuff could record a running count of
rollovers events pretty easily independent of the trace recording
machinery, and that running event count would be good enough for doing
the re-scan I want. If /proc/pid_gen isn't on the table, doing it that
way (with a new tracepoint) would work too. There'd be no way to
separate out rollovers per PID namespace though, so we'd have to
re-scan when *any* PID namespace rolled over, not just the current
one. But that's probably not a problem.
I'll send a patch with a PID rollover tracepoint. Thanks.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Andrew Morton @ 2018-11-22 0:22 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <CAKOZuesF5zi9mpEE6No7oq0hDnM8KFhQFU=NM7qUF635s0r3-A@mail.gmail.com>
On Wed, 21 Nov 2018 15:21:40 -0800 Daniel Colascione <dancol@google.com> wrote:
> On Wed, Nov 21, 2018 at 2:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
> >
> > > On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> ...
>
> > > I wouldn't call tracing a specialized thing: it's important enough to
> > > justify its own summit and a whole ecosystem of trace collection and
> > > analysis tools. We use it in every day in Android. It's tremendously
> > > helpful for understanding system behavior, especially in cases where
> > > multiple components interact in ways that we can't readily predict or
> > > replicate. Reliability and precision in this area are essential:
> > > retrospective analysis of difficult-to-reproduce problems involves
> > > puzzling over trace files and testing hypothesis, and when the trace
> > > system itself is occasionally unreliable, the set of hypothesis to
> > > consider grows. I've tried to keep the amount of kernel infrastructure
> > > needed to support this precision and reliability to a minimum, pushing
> > > most of the complexity to userspace. But we do need, from the kernel,
> > > reliable process disambiguation.
> > >
> > > Besides: things like checkpoint and restart are also non-core
> > > features, but the kernel has plenty of infrastructure to support them.
> > > We're talking about a very lightweight feature in this thread.
> >
> > I'm still not understanding the seriousness of the problem. Presumably
> > you've hit problems in real-life which were serious and frequent enough
> > to justify getting down and writing the code. Please share some sob stories
> > with us!
>
> The problem here is the possibility of confusion, even if it's rare.
> Does the naive approach of just walking /proc and ignoring the
> possibility of PID reuse races work most of the time? Sure. But "most
> of the time" isn't good enough. It's not that there are tons of sob
> stories: it's that without completely robust reporting, we can't rule
> out of the possibility that weirdness we observe in a given trace is
> actually just an artifact from a kinda-sort-working best-effort trace
> collection system instead of a real anomaly in behavior. Tracing,
> essentially, gives us deltas for system state, and without an accurate
> baseline, collected via some kind of scan on trace startup, it's
> impossible to use these deltas to robustly reconstruct total system
> state at a given time. And this matters, because errors in
> reconstruction (e.g., assigning a thread to the wrong process because
> the IDs happen to be reused) can affect processing of the whole trace.
> If it's 3am and I'm analyzing the lone trace from a dogfooder
> demonstrating a particularly nasty problem, I don't want to find out
> that the trace I'm analyzing ended up being useless because the
> kernel's trace system is merely best effort. It's very cheap to be
> 100% reliable here, so let's be reliable and rule out sources of
> error.
So we're solving a problem which isn't known to occur, but solving it
provides some peace-of-mind? Sounds thin!
btw, how should tool developers test their pid_gen-based disambiguation
code?
> > > > Which userspace tools will be using pid_gen? Are the developers of
> > > > those tools signed up to use pid_gen?
> > >
> > > I'll be changing Android tracing tools to capture process snapshots
> > > using pid_gen, using the algorithm in the commit message.
> >
> > Which other tools could use this and what was the feedback from their
> > developers?
>
> I'm going to have Android's systrace and Perfetto use this approach.
> Exactly how many tools signed up to use this feature do you need?
What other ones are there?
> > Those people are the intended audience and the
> > best-positioned reviewers so let's hear from them?
>
> I'm writing plenty of trace analysis tools myself, so I'm part of this
> intended audience. Other tracing tool authors have told me about
> out-of-tree hacks for process atomic snapshots via ftrace events. This
> approach avoids the necessity of these more-invasive hacks.
Those authors would make great reviewers! Adding a cc is cheap.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-22 0:28 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <20181121162247.467fcab6c0aca0819a822286@linux-foundation.org>
On Wed, Nov 21, 2018 at 4:22 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 21 Nov 2018 15:21:40 -0800 Daniel Colascione <dancol@google.com> wrote:
>
> > On Wed, Nov 21, 2018 at 2:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
> > >
> > > > On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > ...
> >
> > > > I wouldn't call tracing a specialized thing: it's important enough to
> > > > justify its own summit and a whole ecosystem of trace collection and
> > > > analysis tools. We use it in every day in Android. It's tremendously
> > > > helpful for understanding system behavior, especially in cases where
> > > > multiple components interact in ways that we can't readily predict or
> > > > replicate. Reliability and precision in this area are essential:
> > > > retrospective analysis of difficult-to-reproduce problems involves
> > > > puzzling over trace files and testing hypothesis, and when the trace
> > > > system itself is occasionally unreliable, the set of hypothesis to
> > > > consider grows. I've tried to keep the amount of kernel infrastructure
> > > > needed to support this precision and reliability to a minimum, pushing
> > > > most of the complexity to userspace. But we do need, from the kernel,
> > > > reliable process disambiguation.
> > > >
> > > > Besides: things like checkpoint and restart are also non-core
> > > > features, but the kernel has plenty of infrastructure to support them.
> > > > We're talking about a very lightweight feature in this thread.
> > >
> > > I'm still not understanding the seriousness of the problem. Presumably
> > > you've hit problems in real-life which were serious and frequent enough
> > > to justify getting down and writing the code. Please share some sob stories
> > > with us!
> >
> > The problem here is the possibility of confusion, even if it's rare.
> > Does the naive approach of just walking /proc and ignoring the
> > possibility of PID reuse races work most of the time? Sure. But "most
> > of the time" isn't good enough. It's not that there are tons of sob
> > stories: it's that without completely robust reporting, we can't rule
> > out of the possibility that weirdness we observe in a given trace is
> > actually just an artifact from a kinda-sort-working best-effort trace
> > collection system instead of a real anomaly in behavior. Tracing,
> > essentially, gives us deltas for system state, and without an accurate
> > baseline, collected via some kind of scan on trace startup, it's
> > impossible to use these deltas to robustly reconstruct total system
> > state at a given time. And this matters, because errors in
> > reconstruction (e.g., assigning a thread to the wrong process because
> > the IDs happen to be reused) can affect processing of the whole trace.
> > If it's 3am and I'm analyzing the lone trace from a dogfooder
> > demonstrating a particularly nasty problem, I don't want to find out
> > that the trace I'm analyzing ended up being useless because the
> > kernel's trace system is merely best effort. It's very cheap to be
> > 100% reliable here, so let's be reliable and rule out sources of
> > error.
>
> So we're solving a problem which isn't known to occur, but solving it
> provides some peace-of-mind? Sounds thin!
So you want to reject a cheap fix for a problem that you know occurs
at some non-zero frequency? There's a big difference between "may or
may not occur" and "will occur eventually, given enough time, and so
must be taken into account in analysis". Would you fix a refcount race
that you knew was possible, but didn't observe? What, exactly, is your
threshold for accepting a fix that makes tracing more reliable?
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-22 0:30 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <CAKOZuet7ipw8zoXCDQUB5PKs3kdSye-zesBbEv7jVDSByhXgkw@mail.gmail.com>
On Wed, Nov 21, 2018 at 4:28 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Wed, Nov 21, 2018 at 4:22 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 21 Nov 2018 15:21:40 -0800 Daniel Colascione <dancol@google.com> wrote:
> >
> > > On Wed, Nov 21, 2018 at 2:50 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > > >
> > > > On Wed, 21 Nov 2018 14:40:28 -0800 Daniel Colascione <dancol@google.com> wrote:
> > > >
> > > > > On Wed, Nov 21, 2018 at 2:12 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > ...
> > >
> > > > > I wouldn't call tracing a specialized thing: it's important enough to
> > > > > justify its own summit and a whole ecosystem of trace collection and
> > > > > analysis tools. We use it in every day in Android. It's tremendously
> > > > > helpful for understanding system behavior, especially in cases where
> > > > > multiple components interact in ways that we can't readily predict or
> > > > > replicate. Reliability and precision in this area are essential:
> > > > > retrospective analysis of difficult-to-reproduce problems involves
> > > > > puzzling over trace files and testing hypothesis, and when the trace
> > > > > system itself is occasionally unreliable, the set of hypothesis to
> > > > > consider grows. I've tried to keep the amount of kernel infrastructure
> > > > > needed to support this precision and reliability to a minimum, pushing
> > > > > most of the complexity to userspace. But we do need, from the kernel,
> > > > > reliable process disambiguation.
> > > > >
> > > > > Besides: things like checkpoint and restart are also non-core
> > > > > features, but the kernel has plenty of infrastructure to support them.
> > > > > We're talking about a very lightweight feature in this thread.
> > > >
> > > > I'm still not understanding the seriousness of the problem. Presumably
> > > > you've hit problems in real-life which were serious and frequent enough
> > > > to justify getting down and writing the code. Please share some sob stories
> > > > with us!
> > >
> > > The problem here is the possibility of confusion, even if it's rare.
> > > Does the naive approach of just walking /proc and ignoring the
> > > possibility of PID reuse races work most of the time? Sure. But "most
> > > of the time" isn't good enough. It's not that there are tons of sob
> > > stories: it's that without completely robust reporting, we can't rule
> > > out of the possibility that weirdness we observe in a given trace is
> > > actually just an artifact from a kinda-sort-working best-effort trace
> > > collection system instead of a real anomaly in behavior. Tracing,
> > > essentially, gives us deltas for system state, and without an accurate
> > > baseline, collected via some kind of scan on trace startup, it's
> > > impossible to use these deltas to robustly reconstruct total system
> > > state at a given time. And this matters, because errors in
> > > reconstruction (e.g., assigning a thread to the wrong process because
> > > the IDs happen to be reused) can affect processing of the whole trace.
> > > If it's 3am and I'm analyzing the lone trace from a dogfooder
> > > demonstrating a particularly nasty problem, I don't want to find out
> > > that the trace I'm analyzing ended up being useless because the
> > > kernel's trace system is merely best effort. It's very cheap to be
> > > 100% reliable here, so let's be reliable and rule out sources of
> > > error.
> >
> > So we're solving a problem which isn't known to occur, but solving it
> > provides some peace-of-mind? Sounds thin!
>
> So you want to reject a cheap fix for a problem that you know occurs
> at some non-zero frequency? There's a big difference between "may or
> may not occur" and "will occur eventually, given enough time, and so
> must be taken into account in analysis". Would you fix a refcount race
> that you knew was possible, but didn't observe? What, exactly, is your
> threshold for accepting a fix that makes tracing more reliable?
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Andrew Morton @ 2018-11-22 0:57 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <CAKOZuet7ipw8zoXCDQUB5PKs3kdSye-zesBbEv7jVDSByhXgkw@mail.gmail.com>
On Wed, 21 Nov 2018 16:28:56 -0800 Daniel Colascione <dancol@google.com> wrote:
> > > The problem here is the possibility of confusion, even if it's rare.
> > > Does the naive approach of just walking /proc and ignoring the
> > > possibility of PID reuse races work most of the time? Sure. But "most
> > > of the time" isn't good enough. It's not that there are tons of sob
> > > stories: it's that without completely robust reporting, we can't rule
> > > out of the possibility that weirdness we observe in a given trace is
> > > actually just an artifact from a kinda-sort-working best-effort trace
> > > collection system instead of a real anomaly in behavior. Tracing,
> > > essentially, gives us deltas for system state, and without an accurate
> > > baseline, collected via some kind of scan on trace startup, it's
> > > impossible to use these deltas to robustly reconstruct total system
> > > state at a given time. And this matters, because errors in
> > > reconstruction (e.g., assigning a thread to the wrong process because
> > > the IDs happen to be reused) can affect processing of the whole trace.
> > > If it's 3am and I'm analyzing the lone trace from a dogfooder
> > > demonstrating a particularly nasty problem, I don't want to find out
> > > that the trace I'm analyzing ended up being useless because the
> > > kernel's trace system is merely best effort. It's very cheap to be
> > > 100% reliable here, so let's be reliable and rule out sources of
> > > error.
> >
> > So we're solving a problem which isn't known to occur, but solving it
> > provides some peace-of-mind? Sounds thin!
>
> So you want to reject a cheap fix for a problem that you know occurs
> at some non-zero frequency? There's a big difference between "may or
> may not occur" and "will occur eventually, given enough time, and so
> must be taken into account in analysis". Would you fix a refcount race
> that you knew was possible, but didn't observe? What, exactly, is your
> threshold for accepting a fix that makes tracing more reliable?
Well for a start I'm looking for a complete patch changelog. One which
permits readers to fully understand the user-visible impact of the
problem.
If it is revealed that is a theoretical problem which has negligible
end-user impact then sure, it is rational to leave things as they are.
That's what "negligible" means!
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-22 1:08 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <20181121165741.ef089df784482632c4a66370@linux-foundation.org>
On Wed, Nov 21, 2018 at 4:57 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 21 Nov 2018 16:28:56 -0800 Daniel Colascione <dancol@google.com> wrote:
>
> > > > The problem here is the possibility of confusion, even if it's rare.
> > > > Does the naive approach of just walking /proc and ignoring the
> > > > possibility of PID reuse races work most of the time? Sure. But "most
> > > > of the time" isn't good enough. It's not that there are tons of sob
> > > > stories: it's that without completely robust reporting, we can't rule
> > > > out of the possibility that weirdness we observe in a given trace is
> > > > actually just an artifact from a kinda-sort-working best-effort trace
> > > > collection system instead of a real anomaly in behavior. Tracing,
> > > > essentially, gives us deltas for system state, and without an accurate
> > > > baseline, collected via some kind of scan on trace startup, it's
> > > > impossible to use these deltas to robustly reconstruct total system
> > > > state at a given time. And this matters, because errors in
> > > > reconstruction (e.g., assigning a thread to the wrong process because
> > > > the IDs happen to be reused) can affect processing of the whole trace.
> > > > If it's 3am and I'm analyzing the lone trace from a dogfooder
> > > > demonstrating a particularly nasty problem, I don't want to find out
> > > > that the trace I'm analyzing ended up being useless because the
> > > > kernel's trace system is merely best effort. It's very cheap to be
> > > > 100% reliable here, so let's be reliable and rule out sources of
> > > > error.
> > >
> > > So we're solving a problem which isn't known to occur, but solving it
> > > provides some peace-of-mind? Sounds thin!
> >
> > So you want to reject a cheap fix for a problem that you know occurs
> > at some non-zero frequency? There's a big difference between "may or
> > may not occur" and "will occur eventually, given enough time, and so
> > must be taken into account in analysis". Would you fix a refcount race
> > that you knew was possible, but didn't observe? What, exactly, is your
> > threshold for accepting a fix that makes tracing more reliable?
>
> Well for a start I'm looking for a complete patch changelog. One which
> permits readers to fully understand the user-visible impact of the
> problem.
The patch already describes the problem, the solution, and the way in
which this solution is provided. What more information do you want?
> If it is revealed that is a theoretical problem which has negligible
> end-user impact then sure, it is rational to leave things as they are.
> That's what "negligible" means!
I don't think the problem is negligible. There's a huge difference
between 99% and 100% reliability! The possibility of a theoretical
problem is a real problem when, in retrospective analysis, the
possibility of theoretical problems must be taken into account when
trying to figure out how the system got into whatever state it was
observed to be in.
Look, if I were proposing some expensive new bit of infrastructure,
that would be one thing. But this is trivial. What form of patch
*would* you take here? Would you take a tracepoint, as I discussed in
your other message? Is there *any* snapshot approach here that you
would take? Is your position that providing an atomic process tree
hierarchy snapshot is just not a capable the kernel should provide?
I'm writing trace analysis tools, and I'm saying that in order to be
confident in the results of the analysis, we need a way to be certain
about baseline system state, and without added robustness, there's
always going to be some doubt as to whether any particular observation
is real or an artifact. I'm open to various technical options for
providing this information, but I think it's reasonable to ask the
system "what is your state?" and somehow get back an answer that's
guaranteed not to be self-contradictory. Have you done much
retrospective long trace analysis?
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Andrew Morton @ 2018-11-22 1:29 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Mike Rapoport, Vlastimil Babka,
Roman Gushchin, Prashant Dhamdhere, Dennis Zhou (Facebook),
Eric W. Biederman, rostedt, tglx, mingo, linux, jpoimboe,
Ard Biesheuvel, Michal Hocko, Stephen Rothwell, ktsanaktsidis,
David Howells, open
In-Reply-To: <CAKOZuessQgmPuiReEO0caNqkss=r44o-XXnem12TOXSn=y6xkw@mail.gmail.com>
On Wed, 21 Nov 2018 17:08:08 -0800 Daniel Colascione <dancol@google.com> wrote:
> Have you done much
> retrospective long trace analysis?
No. Have you?
Of course you have, which is why I and others are dependent upon you to
explain why this change is worth adding to Linux. If this thing solves
a problem which we expect will not occur for anyone between now and the
heat death of the universe then this impacts our decisions.
> The patch already describes the problem, the solution, and the way in
> which this solution is provided. What more information do you want?
I want to know how useful the darn thing is! Why is this so hard?
And my thus-far-unanswered question regarding testing the feature
is also relevant to that understanding. What does a testcase look
like? If we're not actually able to construct one then what does that
mean?
^ permalink raw reply
* Re: [PATCH] Add /proc/pid_generation
From: Matthew Wilcox @ 2018-11-22 2:06 UTC (permalink / raw)
To: Daniel Colascione
Cc: linux-kernel, Linux API, Tim Murray, Primiano Tucci,
Joel Fernandes, Jonathan Corbet, Andrew Morton, Mike Rapoport,
Roman Gushchin, Vlastimil Babka, Dennis Zhou (Facebook),
Prashant Dhamdhere, Eric W. Biederman, rostedt, tglx, mingo,
linux, pasha.tatashin, jpoimboe, ard.biesheuvel, Michal Hocko,
David Howells, ktsanaktsidis
In-Reply-To: <CAKOZueuCYOsaHZzoz5nNPr8Jb-fEiHTR4MektaJx8vuUXcrKCQ@mail.gmail.com>
On Wed, Nov 21, 2018 at 12:38:20PM -0800, Daniel Colascione wrote:
> On Wed, Nov 21, 2018 at 12:31 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Nov 21, 2018 at 12:14:44PM -0800, Daniel Colascione wrote:
> > > This change adds a per-pid-namespace 64-bit generation number,
> > > incremented on PID rollover, and exposes it via a new proc file
> > > /proc/pid_generation. By examining this file before and after /proc
> > > enumeration, user code can detect the potential reuse of a PID and
> > > restart the task enumeration process, repeating until it gets a
> > > coherent snapshot.
> > >
> > > PID rollover ought to be rare, so in practice, scan repetitions will
> > > be rare.
> >
> > Then why does it need to be 64-bit?
>
> [Resending because of accidental HTML. I really need to switch to a
> better email client.]
>
> Because 64 bits is enough for anyone. :-) A u64 is big enough that
> we'll never observe an overflow on a running system, and PID
> namespaces are rare enough that we won't miss the four extra bytes we
> use by upgrading from a u32. And after reading about some security
> problems caused by too-clever handling of 32-bit rollover, I'd rather
> the code be obviously correct than save a trivial amount of space.
I don't think you understand how big 4 billion is. If it happens once a
second, it will take 136 years for a 2^32 count to roll over. How often
does a PID roll over happen?
^ permalink raw reply
* Re: [PATCH -next 1/2] mm/memfd: make F_SEAL_FUTURE_WRITE seal more robust
From: Andrew Morton @ 2018-11-22 2:27 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andy Lutomirski, Stephen Rothwell, Andy Lutomirski, LKML,
Hugh Dickins, Jann Horn, Khalid Aziz, Linux API,
open list:KERNEL SELFTEST FRAMEWORK, Linux-MM, marcandre.lureau,
Matthew Wilcox, Mike Kravetz, Shuah Khan
In-Reply-To: <20181120211335.GC22801@google.com>
On Tue, 20 Nov 2018 13:13:35 -0800 Joel Fernandes <joel@joelfernandes.org> wrote:
> > > I am Ok with whatever Andrew wants to do, if it is better to squash it with
> > > the original, then I can do that and send another patch.
> > >
> > >
> >
> > From experience, Andrew will food in fixups on request :)
>
> Andrew, could you squash this patch into the one titled ("mm: Add an
> F_SEAL_FUTURE_WRITE seal to memfd")?
Sure.
I could of course queue them separately but I rarely do so - I don't
think that the intermediate development states are useful in the
infinite-term, and I make them available via additional Link: tags in
the changelog footers anyway.
I think that the magnitude of these patches is such that John Stultz's
Reviewed-by is invalidated, so this series is now in the "unreviewed"
state.
So can we have a re-review please? For convenience, here's the
folded-together [1/1] patch, as it will go to Linus.
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
Android uses ashmem for sharing memory regions. We are looking forward to
migrating all usecases of ashmem to memfd so that we can possibly remove
the ashmem driver in the future from staging while also benefiting from
using memfd and contributing to it. Note staging drivers are also not ABI
and generally can be removed at anytime.
One of the main usecases Android has is the ability to create a region and
mmap it as writeable, then add protection against making any "future"
writes while keeping the existing already mmap'ed writeable-region active.
This allows us to implement a usecase where receivers of the shared
memory buffer can get a read-only view, while the sender continues to
write to the buffer. See CursorWindow documentation in Android for more
details:
https://developer.android.com/reference/android/database/CursorWindow
This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
which prevents any future mmap and write syscalls from succeeding while
keeping the existing mmap active. The following program shows the seal
working in action:
#include <stdio.h>
#include <errno.h>
#include <sys/mman.h>
#include <linux/memfd.h>
#include <linux/fcntl.h>
#include <asm/unistd.h>
#include <unistd.h>
#define F_SEAL_FUTURE_WRITE 0x0010
#define REGION_SIZE (5 * 1024 * 1024)
int memfd_create_region(const char *name, size_t size)
{
int ret;
int fd = syscall(__NR_memfd_create, name, MFD_ALLOW_SEALING);
if (fd < 0) return fd;
ret = ftruncate(fd, size);
if (ret < 0) { close(fd); return ret; }
return fd;
}
int main() {
int ret, fd;
void *addr, *addr2, *addr3, *addr1;
ret = memfd_create_region("test_region", REGION_SIZE);
printf("ret=%d\n", ret);
fd = ret;
// Create map
addr = mmap(0, REGION_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
if (addr == MAP_FAILED)
printf("map 0 failed\n");
else
printf("map 0 passed\n");
if ((ret = write(fd, "test", 4)) != 4)
printf("write failed even though no future-write seal "
"(ret=%d errno =%d)\n", ret, errno);
else
printf("write passed\n");
addr1 = mmap(0, REGION_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
if (addr1 == MAP_FAILED)
perror("map 1 prot-write failed even though no seal\n");
else
printf("map 1 prot-write passed as expected\n");
ret = fcntl(fd, F_ADD_SEALS, F_SEAL_FUTURE_WRITE |
F_SEAL_GROW |
F_SEAL_SHRINK);
if (ret == -1)
printf("fcntl failed, errno: %d\n", errno);
else
printf("future-write seal now active\n");
if ((ret = write(fd, "test", 4)) != 4)
printf("write failed as expected due to future-write seal\n");
else
printf("write passed (unexpected)\n");
addr2 = mmap(0, REGION_SIZE, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
if (addr2 == MAP_FAILED)
perror("map 2 prot-write failed as expected due to seal\n");
else
printf("map 2 passed\n");
addr3 = mmap(0, REGION_SIZE, PROT_READ, MAP_SHARED, fd, 0);
if (addr3 == MAP_FAILED)
perror("map 3 failed\n");
else
printf("map 3 prot-read passed as expected\n");
}
The output of running this program is as follows:
ret=3
map 0 passed
write passed
map 1 prot-write passed as expected
future-write seal now active
write failed as expected due to future-write seal
map 2 prot-write failed as expected due to seal
: Permission denied
map 3 prot-read passed as expected
[joel@joelfernandes.org: make F_SEAL_FUTURE_WRITE seal more robust]
Link: http://lkml.kernel.org/r/20181120052137.74317-1-joel@joelfernandes.org
Link: http://lkml.kernel.org/r/20181108041537.39694-1-joel@joelfernandes.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Cc: John Stultz <john.stultz@linaro.org>
Cc: John Reck <jreck@google.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Daniel Colascione <dancol@google.com>
Cc: J. Bruce Fields <bfields@fieldses.org>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Khalid Aziz <khalid.aziz@oracle.com>
Cc: Lei Yang <Lei.Yang@windriver.com>
Cc: Marc-Andr Lureau <marcandre.lureau@redhat.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Valdis Kletnieks <valdis.kletnieks@vt.edu>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
--- a/include/uapi/linux/fcntl.h~mm-add-an-f_seal_future_write-seal-to-memfd
+++ a/include/uapi/linux/fcntl.h
@@ -41,6 +41,7 @@
#define F_SEAL_SHRINK 0x0002 /* prevent file from shrinking */
#define F_SEAL_GROW 0x0004 /* prevent file from growing */
#define F_SEAL_WRITE 0x0008 /* prevent writes */
+#define F_SEAL_FUTURE_WRITE 0x0010 /* prevent future writes while mapped */
/* (1U << 31) is reserved for signed error codes */
/*
--- a/mm/memfd.c~mm-add-an-f_seal_future_write-seal-to-memfd
+++ a/mm/memfd.c
@@ -131,7 +131,8 @@ static unsigned int *memfd_file_seals_pt
#define F_ALL_SEALS (F_SEAL_SEAL | \
F_SEAL_SHRINK | \
F_SEAL_GROW | \
- F_SEAL_WRITE)
+ F_SEAL_WRITE | \
+ F_SEAL_FUTURE_WRITE)
static int memfd_add_seals(struct file *file, unsigned int seals)
{
--- a/fs/hugetlbfs/inode.c~mm-add-an-f_seal_future_write-seal-to-memfd
+++ a/fs/hugetlbfs/inode.c
@@ -530,7 +530,7 @@ static long hugetlbfs_punch_hole(struct
inode_lock(inode);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
inode_unlock(inode);
return -EPERM;
}
--- a/mm/shmem.c~mm-add-an-f_seal_future_write-seal-to-memfd
+++ a/mm/shmem.c
@@ -2119,6 +2119,23 @@ out_nomem:
static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
{
+ struct shmem_inode_info *info = SHMEM_I(file_inode(file));
+
+ /*
+ * New PROT_READ and MAP_SHARED mmaps are not allowed when "future
+ * write" seal active.
+ */
+ if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE) &&
+ (info->seals & F_SEAL_FUTURE_WRITE))
+ return -EPERM;
+
+ /*
+ * Since the F_SEAL_FUTURE_WRITE seals allow for a MAP_SHARED read-only
+ * mapping, take care to not allow mprotect to revert protections.
+ */
+ if (info->seals & F_SEAL_FUTURE_WRITE)
+ vma->vm_flags &= ~(VM_MAYWRITE);
+
file_accessed(file);
vma->vm_ops = &shmem_vm_ops;
if (IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) &&
@@ -2344,8 +2361,9 @@ shmem_write_begin(struct file *file, str
pgoff_t index = pos >> PAGE_SHIFT;
/* i_mutex is held by caller */
- if (unlikely(info->seals & (F_SEAL_WRITE | F_SEAL_GROW))) {
- if (info->seals & F_SEAL_WRITE)
+ if (unlikely(info->seals & (F_SEAL_GROW |
+ F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))
return -EPERM;
if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
return -EPERM;
@@ -2608,7 +2626,7 @@ static long shmem_fallocate(struct file
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(shmem_falloc_waitq);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
error = -EPERM;
goto out;
}
_
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Tim Murray @ 2018-11-22 2:35 UTC (permalink / raw)
To: Andrew Morton
Cc: Daniel Colascione, LKML, linux-api, Primiano Tucci,
Joel Fernandes, corbet, rppt, vbabka, guro, pdhamdhe, dennisszhou,
ebiederm, rostedt, tglx, mingo, linux, jpoimboe, ard.biesheuvel,
mhocko, sfr, ktsanaktsidis, dhowells, linux-doc
In-Reply-To: <20181121172919.1b6585e17770d0be7dd8c4d9@linux-foundation.org>
On Wed, Nov 21, 2018 at 5:29 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 21 Nov 2018 17:08:08 -0800 Daniel Colascione <dancol@google.com> wrote:
>
> > Have you done much
> > retrospective long trace analysis?
>
> No. Have you?
>
> Of course you have, which is why I and others are dependent upon you to
> explain why this change is worth adding to Linux. If this thing solves
> a problem which we expect will not occur for anyone between now and the
> heat death of the universe then this impacts our decisions.
I use ftrace the most on Android, so let me take a shot.
In addition to the normal "debug a slow thing" use cases for ftrace,
Android has started exploring two other ways of using ftrace:
1. "Flight recorder" mode: trigger ftrace for some amount of time when
a particular anomaly is detected to make debugging those cases easier.
2. Long traces: let a trace stream to disk for hours or days, then
postprocess it to get some deeper insights about system behavior.
We've used this very successfully to debug and optimize power
consumption.
Knowing the initial state of the system is a pain for both of these
cases. For example, one of the things I'd like to know in some of my
current use cases for long traces is the current oom_score_adj of
every process in the system, but similar to PID reuse, that can change
very quickly due to userspace behavior. There's also a race between
reading that value in userspace and writing it to trace_marker:
1. Userspace daemon X reads oom_score_adj for a process Y.
2. Process Y gets a new oom_score_adj value, triggering the
oom/oom_score_adj_update tracepoint.
3. Daemon X writes the old oom_score_adj value to trace_marker.
As I was writing this, though, I realized that the race doesn't matter
so long as our tools follow the same basic practice (for PID reuse,
oom_score_adj, or anything else we need):
1. Daemon enables all requested tracepoints and resets the trace clock.
2. Daemon enables tracing.
3. Daemon dumps initial state for any tracepoint we care about.
4. When postprocessing, a tool must consider the initial state of a
value (eg, oom_score_adj of pid X) to be either the initial state as
reported by the daemon or the first ftrace event reporting that value.
If there is an ftrace event in the trace before the report from the
daemon, the report from the daemon should be ignored.
The key here is that initial state as reported by userspace needs to
provable from ftrace events. For example, if we stream ps -AT to
trace_marker from userspace, we should be able to prove that pid 5000
in that ps -AT is actually the same process that shows up as pid 5000
later on in the trace and that it has not been replaced by some other
pid 5000. That requires that any event that could break that
assumption be available from the trace itself. Accordingly, I think a
PID reuse tracepoint would work better than an atomic dump of all PIDs
because I'd rather have tracepoints for anything where the initial
state of the system matters than relying on different atomic dumps to
be sure of the initial state. (in this case, we'd have to combine a
PID reuse tracepoint with sched_process_fork and task_rename or
something like that to know what's actually running, but that's a
tractable problem)
The PID reuse tracepoint requires more intelligence in postprocessing
and it still has a race where the state of these values can be
indeterminate at the beginning of a trace if those values change
quickly, but I don't think we can get to a point where we can generate
a full snapshot of every tracepoint we care about in the system at the
start of a trace. For Android's use cases, that short race at the
beginning of a trace isn't a big deal (or at least I can't think of a
case where it would be).
^ 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