* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Rich Felker @ 2018-11-22 15:44 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Florian Weimer, carlos, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <1306224240.10055.1542900799576.JavaMail.zimbra@efficios.com>
On Thu, Nov 22, 2018 at 10:33:19AM -0500, Mathieu Desnoyers wrote:
> ----- On Nov 22, 2018, at 10:21 AM, Florian Weimer fweimer@redhat.com wrote:
>
> > * Rich Felker:
> >
> >> On Thu, Nov 22, 2018 at 04:11:45PM +0100, Florian Weimer wrote:
> >>> * Mathieu Desnoyers:
> >>>
> >>> > Thoughts ?
> >>> >
> >>> > /* Unregister rseq TLS from kernel. */
> >>> > if (has_rseq && __rseq_unregister_current_thread ())
> >>> > abort();
> >>> >
> >>> > advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
> >>> > pd->guardsize);
> >>> >
> >>> > /* If the thread is detached free the TCB. */
> >>> > if (IS_DETACHED (pd))
> >>> > /* Free the TCB. */
> >>> > __free_tcb (pd);
> >>>
> >>> Considering that we proceed to free the TCB, I really hope that all
> >>> signals are blocked at this point. (I have not checked this, though.)
> >>>
> >>> Wouldn't this address your concern about access to the rseq area?
> >>
> >> I'm not familiar with glibc's logic here, but for other reasons, I
> >> don't think freeing it is safe until the kernel task exit futex (set
> >> via clone or set_tid_address) has fired. I would guess __free_tcb just
> >> sets up for it to be reclaimable when this happens rather than
> >> immediately freeing it for reuse.
> >
> > Right, but in case of user-supplied stacks, we actually free TLS memory
> > at this point, so signals need to be blocked because the TCB is
> > (partially) gone after that.
>
> Unfortuntately, disabling signals is not enough.
>
> With rseq registered, the kernel accesses the rseq TLS area when returning to
> user-space after _preemption_ of user-space, which can be triggered at any
> point by an interrupt or a fault, even if signals are blocked.
>
> So if there are cases where the TLS memory is freed while the thread is still
> running, we _need_ to explicitly unregister rseq beforehand.
OK, that makes sense. I was wrongly under the impression that the TLS
memory could not be reused until the task exit futex fired, but in
glibc that's not the case with caller-provided stacks.
I still don't understand the need for a reference count though.
Rich
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Mathieu Desnoyers @ 2018-11-22 15:33 UTC (permalink / raw)
To: Florian Weimer
Cc: Rich Felker, carlos, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <875zwpyw81.fsf@oldenburg.str.redhat.com>
----- On Nov 22, 2018, at 10:21 AM, Florian Weimer fweimer@redhat.com wrote:
> * Rich Felker:
>
>> On Thu, Nov 22, 2018 at 04:11:45PM +0100, Florian Weimer wrote:
>>> * Mathieu Desnoyers:
>>>
>>> > Thoughts ?
>>> >
>>> > /* Unregister rseq TLS from kernel. */
>>> > if (has_rseq && __rseq_unregister_current_thread ())
>>> > abort();
>>> >
>>> > advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
>>> > pd->guardsize);
>>> >
>>> > /* If the thread is detached free the TCB. */
>>> > if (IS_DETACHED (pd))
>>> > /* Free the TCB. */
>>> > __free_tcb (pd);
>>>
>>> Considering that we proceed to free the TCB, I really hope that all
>>> signals are blocked at this point. (I have not checked this, though.)
>>>
>>> Wouldn't this address your concern about access to the rseq area?
>>
>> I'm not familiar with glibc's logic here, but for other reasons, I
>> don't think freeing it is safe until the kernel task exit futex (set
>> via clone or set_tid_address) has fired. I would guess __free_tcb just
>> sets up for it to be reclaimable when this happens rather than
>> immediately freeing it for reuse.
>
> Right, but in case of user-supplied stacks, we actually free TLS memory
> at this point, so signals need to be blocked because the TCB is
> (partially) gone after that.
Unfortuntately, disabling signals is not enough.
With rseq registered, the kernel accesses the rseq TLS area when returning to
user-space after _preemption_ of user-space, which can be triggered at any
point by an interrupt or a fault, even if signals are blocked.
So if there are cases where the TLS memory is freed while the thread is still
running, we _need_ to explicitly unregister rseq beforehand.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Mathieu Desnoyers @ 2018-11-22 15:27 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,
Thomas Gleixner, Ingo Molnar, linux, Josh Poimboeuf,
Ard Biesheuvel, Michal Hocko
In-Reply-To: <CAKOZueuV=X4eqc+-8Jh3Z-2iQO+4fp2otEOs03yQoaa88C9zUA@mail.gmail.com>
----- On Nov 21, 2018, at 7:30 PM, 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.
>> >
[...]
I've just been CC'd on this thread for some reason, so I'll add my 2 cents.
WHIW, I think using /proc to add stateful information to a time-based
trace is the wrong way to do things. Here, the fact that you need to
add a generation counter struct pid_namespace and expose it via /proc
just highlights its limitations when it comes to dealing with state
that changes over time. Your current issue is with PID re-use, but
you will eventually face the same issue for re-use of all other resources
you are trying to model. For instance, a file descriptor may be associated
to a path as some point in time, but that is not true anymore after a
sequence of close/open which re-uses that file descriptor. Does that
mean we will eventually end up needing per-file-descriptor generation
counters as well ?
LTTng solves this by dumping the system state as events within the
trace [1], which associates time-stamps with the state being dumped.
It is recorded while the rest of the system is being traced, so tools
can reconstruct full system state by combining this statedump with the
rest of the events recording state transitions.
So while I agree that it's important to have a way to reconstruct
system state that is aware of PID re-use, I think trying to extend
/proc for this is the wrong approach. It adds extra fields to struct
pid_namespace that seem to be only useful for tracing, whereas using
the time-stamp at which the thread/process was first seen in the trace
(either fork or statedump) as secondary key should suffice to uniquely
identify a thread/process. I would recommend extending tracing
facilities to dump the data you need rather than /proc.
Thanks,
Mathieu
[1] http://git.lttng.org/?p=lttng-modules.git;a=blob;f=lttng-statedump-impl.c;h=dc037508c055b7f61b8c758d581bd0178e26552a;hb=HEAD
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Florian Weimer @ 2018-11-22 15:21 UTC (permalink / raw)
To: Rich Felker
Cc: Mathieu Desnoyers, carlos, Joseph Myers, Szabolcs Nagy,
libc-alpha, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, linux-kernel, linux-api
In-Reply-To: <20181122151710.GF23599@brightrain.aerifal.cx>
* Rich Felker:
> On Thu, Nov 22, 2018 at 04:11:45PM +0100, Florian Weimer wrote:
>> * Mathieu Desnoyers:
>>
>> > Thoughts ?
>> >
>> > /* Unregister rseq TLS from kernel. */
>> > if (has_rseq && __rseq_unregister_current_thread ())
>> > abort();
>> >
>> > advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
>> > pd->guardsize);
>> >
>> > /* If the thread is detached free the TCB. */
>> > if (IS_DETACHED (pd))
>> > /* Free the TCB. */
>> > __free_tcb (pd);
>>
>> Considering that we proceed to free the TCB, I really hope that all
>> signals are blocked at this point. (I have not checked this, though.)
>>
>> Wouldn't this address your concern about access to the rseq area?
>
> I'm not familiar with glibc's logic here, but for other reasons, I
> don't think freeing it is safe until the kernel task exit futex (set
> via clone or set_tid_address) has fired. I would guess __free_tcb just
> sets up for it to be reclaimable when this happens rather than
> immediately freeing it for reuse.
Right, but in case of user-supplied stacks, we actually free TLS memory
at this point, so signals need to be blocked because the TCB is
(partially) gone after that.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Rich Felker @ 2018-11-22 15:17 UTC (permalink / raw)
To: Florian Weimer
Cc: Mathieu Desnoyers, carlos, Joseph Myers, Szabolcs Nagy,
libc-alpha, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, linux-kernel, linux-api
In-Reply-To: <87a7m1ywni.fsf@oldenburg.str.redhat.com>
On Thu, Nov 22, 2018 at 04:11:45PM +0100, Florian Weimer wrote:
> * Mathieu Desnoyers:
>
> > Thoughts ?
> >
> > /* Unregister rseq TLS from kernel. */
> > if (has_rseq && __rseq_unregister_current_thread ())
> > abort();
> >
> > advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
> > pd->guardsize);
> >
> > /* If the thread is detached free the TCB. */
> > if (IS_DETACHED (pd))
> > /* Free the TCB. */
> > __free_tcb (pd);
>
> Considering that we proceed to free the TCB, I really hope that all
> signals are blocked at this point. (I have not checked this, though.)
>
> Wouldn't this address your concern about access to the rseq area?
I'm not familiar with glibc's logic here, but for other reasons, I
don't think freeing it is safe until the kernel task exit futex (set
via clone or set_tid_address) has fired. I would guess __free_tcb just
sets up for it to be reclaimable when this happens rather than
immediately freeing it for reuse.
Rich
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Rich Felker @ 2018-11-22 15:14 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: carlos, Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <782067422.9852.1542899056778.JavaMail.zimbra@efficios.com>
On Thu, Nov 22, 2018 at 10:04:16AM -0500, Mathieu Desnoyers wrote:
> ----- On Nov 22, 2018, at 9:36 AM, Rich Felker dalias@libc.org wrote:
>
> > On Wed, Nov 21, 2018 at 01:39:32PM -0500, Mathieu Desnoyers wrote:
> >> Register rseq(2) TLS for each thread (including main), and unregister
> >> for each thread (excluding main). "rseq" stands for Restartable
> >> Sequences.
> >
> > Maybe I'm missing something obvious, but "unregister" does not seem to
> > be a meaningful operation. Can you clarify what it's for?
>
> There are really two ways rseq TLS can end up being unregistered: either
> through an explicit call to the rseq "unregister", or when the OS frees the
> thread's task struct.
>
> You bring an interesting point here: do we need to explicitly unregister
> rseq at thread exit, or can we leave that to the OS ?
>
> The key thing to look for here is whether it's valid to access the
> TLS area of the thread from preemption or signal delivery happening
> at the very end of START_THREAD_DEFN. If it's OK to access it until
> the very end of the thread lifetime, then we could do without an
> explicit unregistration. However, if at any given point of the late
> thread lifetime we end up in a situation where reading or writing to
> that TLS area can cause corruption, then we need to carefully
> unregister it before that memory is reclaimed/reused.
The thread memory cannot be reused until after kernel task exit,
reported via the set_tid_address futex. Also, assuming signals are
blocked (which is absolutely necessary for other reasons) nothing in
userspace can touch the rseq state after this point anyway.
I was more confused about the need for reference counting, though.
Where would anything be able to observe a state other than "refcnt>0"?
-- in which case tracking it makes no sense. If the goal is to make an
ABI thatsupports environments where libc doesn't have rseq support,
and a third-party library is providing a compatible ABI, it seems all
that would be needed it a boolean thread-local "is_initialized" flag.
There does not seem to be any safe way such a library could be
dynamically unloaded (which would require unregistration in all
threads) and thus no need for a count.
Rich
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Florian Weimer @ 2018-11-22 15:11 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Rich Felker, carlos, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <782067422.9852.1542899056778.JavaMail.zimbra@efficios.com>
* Mathieu Desnoyers:
> Thoughts ?
>
> /* Unregister rseq TLS from kernel. */
> if (has_rseq && __rseq_unregister_current_thread ())
> abort();
>
> advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
> pd->guardsize);
>
> /* If the thread is detached free the TCB. */
> if (IS_DETACHED (pd))
> /* Free the TCB. */
> __free_tcb (pd);
Considering that we proceed to free the TCB, I really hope that all
signals are blocked at this point. (I have not checked this, though.)
Wouldn't this address your concern about access to the rseq area?
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Mathieu Desnoyers @ 2018-11-22 15:04 UTC (permalink / raw)
To: Rich Felker
Cc: carlos, Florian Weimer, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <20181122143603.GD23599@brightrain.aerifal.cx>
----- On Nov 22, 2018, at 9:36 AM, Rich Felker dalias@libc.org wrote:
> On Wed, Nov 21, 2018 at 01:39:32PM -0500, Mathieu Desnoyers wrote:
>> Register rseq(2) TLS for each thread (including main), and unregister
>> for each thread (excluding main). "rseq" stands for Restartable
>> Sequences.
>
> Maybe I'm missing something obvious, but "unregister" does not seem to
> be a meaningful operation. Can you clarify what it's for?
There are really two ways rseq TLS can end up being unregistered: either
through an explicit call to the rseq "unregister", or when the OS frees the
thread's task struct.
You bring an interesting point here: do we need to explicitly unregister
rseq at thread exit, or can we leave that to the OS ?
The key thing to look for here is whether it's valid to access the
TLS area of the thread from preemption or signal delivery happening
at the very end of START_THREAD_DEFN. If it's OK to access it until
the very end of the thread lifetime, then we could do without an
explicit unregistration. However, if at any given point of the late
thread lifetime we end up in a situation where reading or writing to
that TLS area can cause corruption, then we need to carefully
unregister it before that memory is reclaimed/reused.
What we have below the current location for the __rseq_unregister_current_thread ()
call is as follows. I'm not all that convinced that it's valid to access the TLS
area up until __exit_thread () at the very end, especially after setting
setxid_futex back to 0.
Thoughts ?
/* Unregister rseq TLS from kernel. */
if (has_rseq && __rseq_unregister_current_thread ())
abort();
advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
pd->guardsize);
/* If the thread is detached free the TCB. */
if (IS_DETACHED (pd))
/* Free the TCB. */
__free_tcb (pd);
else if (__glibc_unlikely (pd->cancelhandling & SETXID_BITMASK))
{
/* Some other thread might call any of the setXid functions and expect
us to reply. In this case wait until we did that. */
do
/* XXX This differs from the typical futex_wait_simple pattern in that
the futex_wait condition (setxid_futex) is different from the
condition used in the surrounding loop (cancelhandling). We need
to check and document why this is correct. */
futex_wait_simple (&pd->setxid_futex, 0, FUTEX_PRIVATE);
while (pd->cancelhandling & SETXID_BITMASK);
/* Reset the value so that the stack can be reused. */
pd->setxid_futex = 0;
}
/* We cannot call '_exit' here. '_exit' will terminate the process.
The 'exit' implementation in the kernel will signal when the
process is really dead since 'clone' got passed the CLONE_CHILD_CLEARTID
flag. The 'tid' field in the TCB will be set to zero.
The exit code is zero since in case all threads exit by calling
'pthread_exit' the exit status must be 0 (zero). */
__exit_thread ();
/* NOTREACHED */
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Andy Lutomirski @ 2018-11-22 14:55 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Andrew Lutomirski, Elvira Khabirova, Kees Cook, Sasha Levin,
Linux API, Jann Horn, Oleg Nesterov, Steven Rostedt, Ingo Molnar,
LKML, Eugene Syromiatnikov, strace-devel
In-Reply-To: <20181121235634.GA14146@altlinux.org>
On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > Please cc linux-api@vger.kernel.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.
I'm not sure there's any material difference.
>
> 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.
Hmm. PTRACE_GET_SYSCALL_INFO should fail for PTRACE_EVENT_EXIT, I think.
>
> > > __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.
Agreed. Although if we ever start using those high bits, things will
get confusing.
>
> > > __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.
Makes sense to me. Also, it's possible in principle to extend
seccomp_data with other fields that are only generated if they're
read, so passing struct seccomp_data to userspace as a struct may be
the wrong thing to do.
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Rich Felker @ 2018-11-22 14:36 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Carlos O'Donell, Florian Weimer, Joseph Myers, Szabolcs Nagy,
libc-alpha, 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>
On Wed, Nov 21, 2018 at 01:39:32PM -0500, Mathieu Desnoyers wrote:
> Register rseq(2) TLS for each thread (including main), and unregister
> for each thread (excluding main). "rseq" stands for Restartable
> Sequences.
Maybe I'm missing something obvious, but "unregister" does not seem to
be a meaningful operation. Can you clarify what it's for?
Rich
^ permalink raw reply
* [PATCH v3 4/4] selftests: add a test for bpf_prog_test_run_xattr
From: Lorenz Bauer @ 2018-11-22 14:09 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181122140910.1079-1-lmb@cloudflare.com>
Make sure that bpf_prog_test_run_xattr returns the correct length
and that the kernel respects the output size hint. Also check
that errno indicates ENOSPC if there is a short output buffer given.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/testing/selftests/bpf/test_progs.c | 49 ++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c1e688f61061..f9f5b1dbcc83 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -124,6 +124,54 @@ static void test_pkt_access(void)
bpf_object__close(obj);
}
+static void test_prog_run_xattr(void)
+{
+ const char *file = "./test_pkt_access.o";
+ __u32 duration, retval, size_out;
+ struct bpf_object *obj;
+ char buf[10];
+ int err;
+ struct bpf_prog_test_run_attr tattr = {
+ .repeat = 1,
+ .data = &pkt_v4,
+ .size = sizeof(pkt_v4),
+ .data_out = buf,
+ .size_out = 5,
+ };
+
+ err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj,
+ &tattr.prog_fd);
+ if (CHECK(err, "load", "err %d errno %d\n", err, errno))
+ return;
+
+ memset(buf, 0, sizeof(buf));
+
+ err = bpf_prog_test_run_xattr(&tattr, &size_out, &retval, &duration);
+ CHECK(err != -1 || errno != ENOSPC || retval, "run",
+ "err %d errno %d retval %d\n", err, errno, retval);
+
+ CHECK(size_out != sizeof(pkt_v4), "output_size",
+ "incorrect output size, want %lu have %u\n",
+ sizeof(pkt_v4), size_out);
+
+ CHECK(buf[5] != 0, "overflow",
+ "BPF_PROG_TEST_RUN ignored size hint\n");
+
+ tattr.data_out = NULL;
+ tattr.size_out = 0;
+ errno = 0;
+
+ err = bpf_prog_test_run_xattr(&tattr, NULL, &retval, &duration);
+ CHECK(err || errno || retval, "run_no_output",
+ "err %d errno %d retval %d\n", err, errno, retval);
+
+ tattr.size_out = 1;
+ err = bpf_prog_test_run_xattr(&tattr, NULL, NULL, &duration);
+ CHECK(err != -EINVAL, "run_wrong_size_out", "err %d\n", err);
+
+ bpf_object__close(obj);
+}
+
static void test_xdp(void)
{
struct vip key4 = {.protocol = 6, .family = AF_INET};
@@ -1837,6 +1885,7 @@ int main(void)
jit_enabled = is_jit_enabled();
test_pkt_access();
+ test_prog_run_xattr();
test_xdp();
test_xdp_adjust_tail();
test_l4lb_all();
--
2.17.1
^ permalink raw reply related
* [PATCH v3 3/4] libbpf: add bpf_prog_test_run_xattr
From: Lorenz Bauer @ 2018-11-22 14:09 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181122140910.1079-1-lmb@cloudflare.com>
Add a new function, which encourages safe usage of the test interface.
bpf_prog_test_run continues to work as before, but should be considered
unsafe.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/lib/bpf/bpf.c | 27 +++++++++++++++++++++++++++
tools/lib/bpf/bpf.h | 13 +++++++++++++
2 files changed, 40 insertions(+)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 961e1b9fc592..f8518bef6886 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -424,6 +424,33 @@ int bpf_prog_test_run(int prog_fd, int repeat, void *data, __u32 size,
return ret;
}
+int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+ __u32 *size_out, __u32 *retval, __u32 *duration)
+{
+ union bpf_attr attr;
+ int ret;
+
+ if (!test_attr->data_out && test_attr->size_out > 0)
+ return -EINVAL;
+
+ bzero(&attr, sizeof(attr));
+ attr.test.prog_fd = test_attr->prog_fd;
+ attr.test.data_in = ptr_to_u64(test_attr->data);
+ attr.test.data_out = ptr_to_u64(test_attr->data_out);
+ attr.test.data_size_in = test_attr->size;
+ attr.test.data_size_out = test_attr->size_out;
+ attr.test.repeat = test_attr->repeat;
+
+ ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, sizeof(attr));
+ if (size_out)
+ *size_out = attr.test.data_size_out;
+ if (retval)
+ *retval = attr.test.retval;
+ if (duration)
+ *duration = attr.test.duration;
+ return ret;
+}
+
int bpf_prog_get_next_id(__u32 start_id, __u32 *next_id)
{
union bpf_attr attr;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 26a51538213c..570f19f77f42 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -110,6 +110,19 @@ LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
LIBBPF_API int bpf_prog_detach(int attachable_fd, enum bpf_attach_type type);
LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd,
enum bpf_attach_type type);
+
+struct bpf_prog_test_run_attr {
+ int prog_fd;
+ int repeat;
+ const void *data;
+ __u32 size;
+ void *data_out; /* optional */
+ __u32 size_out;
+};
+
+LIBBPF_API int bpf_prog_test_run_xattr(const struct bpf_prog_test_run_attr *test_attr,
+ __u32 *size_out, __u32 *retval,
+ __u32 *duration);
LIBBPF_API int bpf_prog_test_run(int prog_fd, int repeat, void *data,
__u32 size, void *data_out, __u32 *size_out,
__u32 *retval, __u32 *duration);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 2/4] tools: sync uapi/linux/bpf.h
From: Lorenz Bauer @ 2018-11-22 14:09 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181122140910.1079-1-lmb@cloudflare.com>
Pull changes from "bpf: respect size hint to BPF_PROG_TEST_RUN if present".
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
tools/include/uapi/linux/bpf.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
--
2.17.1
^ permalink raw reply related
* [PATCH v3 1/4] bpf: respect size hint to BPF_PROG_TEST_RUN if present
From: Lorenz Bauer @ 2018-11-22 14:09 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181122140910.1079-1-lmb@cloudflare.com>
Use data_size_out as a size hint when copying test output to user space.
ENOSPC is returned if the output buffer is too small.
Callers which so far did not set data_size_out are not affected.
Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
include/uapi/linux/bpf.h | 7 +++++--
net/bpf/test_run.c | 15 +++++++++++++--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 05d95290b848..7f5a7d032cd1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -356,8 +356,11 @@ union bpf_attr {
struct { /* anonymous struct used by BPF_PROG_TEST_RUN command */
__u32 prog_fd;
__u32 retval;
- __u32 data_size_in;
- __u32 data_size_out;
+ __u32 data_size_in; /* input: len of data_in */
+ __u32 data_size_out; /* input/output: len of data_out
+ * returns ENOSPC if data_out
+ * is too small.
+ */
__aligned_u64 data_in;
__aligned_u64 data_out;
__u32 repeat;
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index c89c22c49015..7663e6a57280 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -74,8 +74,18 @@ static int bpf_test_finish(const union bpf_attr *kattr,
{
void __user *data_out = u64_to_user_ptr(kattr->test.data_out);
int err = -EFAULT;
+ u32 copy_size = size;
- if (data_out && copy_to_user(data_out, data, size))
+ /* Clamp copy if the user has provided a size hint, but copy the full
+ * buffer if not to retain old behaviour.
+ */
+ if (kattr->test.data_size_out &&
+ copy_size > kattr->test.data_size_out) {
+ copy_size = kattr->test.data_size_out;
+ err = -ENOSPC;
+ }
+
+ if (data_out && copy_to_user(data_out, data, copy_size))
goto out;
if (copy_to_user(&uattr->test.data_size_out, &size, sizeof(size)))
goto out;
@@ -83,7 +93,8 @@ static int bpf_test_finish(const union bpf_attr *kattr,
goto out;
if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration)))
goto out;
- err = 0;
+ if (err != -ENOSPC)
+ err = 0;
out:
return err;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 0/4] Fix unsafe BPF_PROG_TEST_RUN interface
From: Lorenz Bauer @ 2018-11-22 14:09 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-api, ys114321, Lorenz Bauer
In-Reply-To: <20181116125329.3974-1-lmb@cloudflare.com>
Right now, there is no safe way to use BPF_PROG_TEST_RUN with data_out.
This is because bpf_test_finish copies the output buffer to user space
without checking its size. This can lead to the kernel overwriting
data in user space after the buffer if xdp_adjust_head and friends are
in play.
Changes in v3:
* Introduce bpf_prog_test_run_xattr instead of modifying the existing
function
Changes in v2:
* Make the syscall return ENOSPC if data_size_out is too small
* Make bpf_prog_test_run return EINVAL if size_out is missing
* Document the new behaviour of data_size_out
Lorenz Bauer (4):
bpf: respect size hint to BPF_PROG_TEST_RUN if present
tools: sync uapi/linux/bpf.h
libbpf: add bpf_prog_test_run_xattr
selftests: add a test for bpf_prog_test_run_xattr
include/uapi/linux/bpf.h | 7 +++-
net/bpf/test_run.c | 15 +++++++-
tools/include/uapi/linux/bpf.h | 7 +++-
tools/lib/bpf/bpf.c | 27 +++++++++++++
tools/lib/bpf/bpf.h | 13 +++++++
tools/testing/selftests/bpf/test_progs.c | 49 ++++++++++++++++++++++++
6 files changed, 112 insertions(+), 6 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Cyrill Gorcunov @ 2018-11-22 13:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, 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
In-Reply-To: <37255927-1A93-4B8B-A916-B5A3983D56B6@amacapital.net>
On Wed, Nov 21, 2018 at 04:35:34PM -0700, Andy Lutomirski wrote:
> >
> > 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?
FWIW we've had similar problem in criu for memory snapshotting,
because memory data is PID-driven and snapshots are rather
discrete events. So we use task_struct::real_start_time as
a second guard agains pid reuse.
^ permalink raw reply
* Re: [PATCH v2] UAPI: move RENAME_* definitions to separated file
From: Yury Norov @ 2018-11-22 12:16 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro, Florian Weimer
Cc: Norov, Yuri, linux-kernel@vger.kernel.org,
linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
libc-alpha@sourceware.org
In-Reply-To: <20181114072559.GA16695@yury-thinkpad>
Ping?
On Wed, Nov 14, 2018 at 07:26:07AM +0000, Yury Norov wrote:
> Hi all,
>
> It seems that I forgot to rename guards in v2. This is the correct
> version. My apologize for noise.
>
> Yury
>
> >From 61215b1cb559e69fb57f2a82864e72e3a1ef4b12 Mon Sep 17 00:00:00 2001
> From: Yury Norov <ynorov@caviumnetworks.com>
> Date: Wed, 14 Nov 2018 09:51:49 +0300
> To: Andrew Morton <akpm@linux-foundation.org>,
> Alexander Viro <viro@zeniv.linux.org.uk>,
> Florian Weimer <fweimer@redhat.com>
> CC: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
> linux-api@vger.kernel.org, libc-alpha@sourceware.org
> Subject: [PATCH v2] UAPI: move RENAME_* definitions to separated file
>
> Discussion:
> https://lore.kernel.org/lkml/20180702084622.GA15274@yury-thinkpad/
>
> Although RENAME_* macros are exposed in kernel headers, they are not
> used by glibc. That's because linux/fs.h which hosts RENAME_* is
> considered unsuitable by glibc developers:
>
> As Florian Weimer wrote:
>
> > <linux/fs.h> undefines and defines macros not mentioned in the standards
> > (and it even contains a few unrelated structs), so we cannot include it
> > without _GNU_SOURCE.
> >
> > It might be possible to include it only for _GNU_SOURCE, but there are a
> > lot of things in <linux/fs.h>, so that does not seem to be particularly
> > advisable.
>
> In this patch RENAME_* is removed to a separated header file that
> conforms the standards and therefore is suitable for inclusion by glibc.
>
> v2:
> - rename dedicated file to 'rename-flags.h', as requested by Florian.
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
> ---
> include/uapi/linux/fs.h | 4 +---
> include/uapi/linux/rename-flags.h | 12 ++++++++++++
> 2 files changed, 13 insertions(+), 3 deletions(-)
> create mode 100644 include/uapi/linux/rename-flags.h
>
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 73e01918f996..34c308203746 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -39,9 +39,7 @@
> #define SEEK_HOLE 4 /* seek to the next hole */
> #define SEEK_MAX SEEK_HOLE
>
> -#define RENAME_NOREPLACE (1 << 0) /* Don't overwrite target */
> -#define RENAME_EXCHANGE (1 << 1) /* Exchange source and dest */
> -#define RENAME_WHITEOUT (1 << 2) /* Whiteout source */
> +#include <linux/rename-flags.h>
>
> struct file_clone_range {
> __s64 src_fd;
> diff --git a/include/uapi/linux/rename-flags.h b/include/uapi/linux/rename-flags.h
> new file mode 100644
> index 000000000000..7178f0565657
> --- /dev/null
> +++ b/include/uapi/linux/rename-flags.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_RENAME_FLAGS_H
> +#define _UAPI_LINUX_RENAME_FLAGS_H
> +
> +/*
> + * Definitions for rename syscall family.
> + */
> +#define RENAME_NOREPLACE (1 << 0) /* Don't overwrite target */
> +#define RENAME_EXCHANGE (1 << 1) /* Exchange source and dest */
> +#define RENAME_WHITEOUT (1 << 2) /* Whiteout source */
> +
> +#endif /* _UAPI_LINUX_RENAME_FLAGS_H */
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH] Add /proc/pid_generation
From: Kevin Easton @ 2018-11-22 11:19 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, Matth
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.
I see downthread this patch has been withdrawn, but nonetheless I'm
still curious - does this actually solve the problem?
It seems to me that a PID could be reused within a scan even if the
generation number remains the same at the beginning and end of a scan:
Say you have a very long-lived task with PID 500 allocated in generation
0. The PID creation has since wrapped and we are now allocating from the
start of the range again, with generation 1. We begin a scan of /proc,
read the generation (1) and at this point, our task dies and PID 500 is
then reallocated to a new task. We finish our scan, generation is still
1 but PID 500 is now ambiguous.
Am I wrong?
- Kevin
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Aleksa Sarai @ 2018-11-22 8:23 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, viro,
linux-fsdevel, linux-api, dancol, timmurray, linux-man, Kees Cook
In-Reply-To: <20181120105124.14733-1-christian@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 13106 bytes --]
On 2018-11-20, Christian Brauner <christian@brauner.io> wrote:
> The kill() syscall operates on process identifiers. After a process has
> exited its pid can be reused by another process. If a caller sends a signal
> to a reused pid it will end up signaling the wrong process. This issue has
> often surfaced and there has been a push [1] to address this problem.
>
> This patch uses file descriptors from proc/<pid> as stable handles on
> struct pid. Even if a pid is recycled the handle will not change. The file
> descriptor can be used to send signals to the referenced process.
> Thus, the new syscall procfd_signal() is introduced to solve this problem.
> It operates on a process file descriptor.
> The syscall takes an additional siginfo_t and flags argument. If siginfo_t
> is NULL then procfd_signal() behaves like kill() if it is not NULL it
> behaves like rt_sigqueueinfo.
> The flags argument is added to allow for future extensions of this syscall.
> It currently needs to be passed as 0.
>
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <signal.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> int ret;
> char buf[1000];
>
> if (argc < 2)
> exit(EXIT_FAILURE);
>
> ret = snprintf(buf, sizeof(buf), "/proc/%s", argv[1]);
> if (ret < 0)
> exit(EXIT_FAILURE);
>
> int fd = open(buf, O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), buf);
> exit(EXIT_FAILURE);
> }
>
> ret = syscall(__NR_procfd_signal, fd, SIGKILL, NULL, 0);
> if (ret < 0) {
> printf("Failed to send SIGKILL \"%s\"\n", strerror(errno));
> close(fd);
> exit(EXIT_FAILURE);
> }
>
> close(fd);
>
> exit(EXIT_SUCCESS);
> }
>
> [1]: https://lkml.org/lkml/2018/11/18/130
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
Acked-by: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> Changelog:
> v2:
> - define __NR_procfd_signal in unistd.h
> - wire up compat syscall
> - s/proc_is_procfd/proc_is_tgid_procfd/g
> - provide stubs when CONFIG_PROC_FS=n
> - move proc_pid() to linux/proc_fs.h header
> - use proc_pid() to grab struct pid from /proc/<pid> fd
> v1:
> - patch introduced
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> fs/proc/base.c | 11 ++-
> fs/proc/internal.h | 5 -
> include/linux/proc_fs.h | 12 +++
> include/linux/syscalls.h | 2 +
> include/uapi/asm-generic/unistd.h | 4 +-
> kernel/signal.c | 127 +++++++++++++++++++++++--
> 8 files changed, 151 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..3f27ffd8ae87 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..8a30cde82450 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 64 procfd_signal __x64_sys_procfd_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> @@ -386,3 +387,4 @@
> 545 x32 execveat __x32_compat_sys_execveat/ptregs
> 546 x32 preadv2 __x32_compat_sys_preadv64v2
> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ce3465479447..771c6bd1cac6 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -716,7 +716,10 @@ static int proc_pid_permission(struct inode *inode, int mask)
> return generic_permission(inode, mask);
> }
>
> -
> +struct pid *proc_pid(const struct inode *inode)
> +{
> + return PROC_I(inode)->pid;
> +}
>
> static const struct inode_operations proc_def_inode_operations = {
> .setattr = proc_setattr,
> @@ -3038,6 +3041,12 @@ static const struct file_operations proc_tgid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tgid_base_operations);
> +}
> +
> static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> {
> return proc_pident_lookup(dir, dentry,
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 5185d7f6a51e..eb69afba83f3 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -113,11 +113,6 @@ static inline void *__PDE_DATA(const struct inode *inode)
> return PDE(inode)->data;
> }
>
> -static inline struct pid *proc_pid(const struct inode *inode)
> -{
> - return PROC_I(inode)->pid;
> -}
> -
> static inline struct task_struct *get_proc_task(const struct inode *inode)
> {
> return get_pid_task(proc_pid(inode), PIDTYPE_PID);
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..96df2fe6311d 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> int (*show)(struct seq_file *, void *),
> proc_write_t write,
> void *data);
> +extern bool proc_is_tgid_procfd(const struct file *file);
> +extern struct pid *proc_pid(const struct inode *inode);
>
> #else /* CONFIG_PROC_FS */
>
> @@ -114,6 +116,16 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
> #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
>
> +static inline bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return false;
> +}
> +
> +static inline struct pid *proc_pid(const struct inode *inode)
> +{
> + return NULL;
> +}
> +
> #endif /* CONFIG_PROC_FS */
>
> struct net;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..a5ca8cb84566 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -907,6 +907,8 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_procfd_signal(int fd, int sig, siginfo_t __user *info,
> + int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 538546edbfbd..4dc81a994ad1 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
> __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
> #define __NR_rseq 293
> __SYSCALL(__NR_rseq, sys_rseq)
> +#define __NR_procfd_signal 294
> +__SC_COMP(__NR_procfd_signal, sys_procfd_signal, compat_sys_procfd_signal)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 294
> +#define __NR_syscalls 295
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a32bc2088c9..13695342f150 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -19,7 +19,9 @@
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/sched/cputime.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/proc_fs.h>
> #include <linux/tty.h>
> #include <linux/binfmts.h>
> #include <linux/coredump.h>
> @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> }
> #endif
>
> +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +{
> + clear_siginfo(info);
> + info->si_signo = sig;
> + info->si_errno = 0;
> + info->si_code = SI_USER;
> + info->si_pid = task_tgid_vnr(current);
> + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +}
> +
> /**
> * sys_kill - send a signal to a process
> * @pid: the PID of the process
> @@ -3295,16 +3307,119 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> {
> struct kernel_siginfo info;
>
> - clear_siginfo(&info);
> - info.si_signo = sig;
> - info.si_errno = 0;
> - info.si_code = SI_USER;
> - info.si_pid = task_tgid_vnr(current);
> - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> + prepare_kill_siginfo(sig, &info);
>
> return kill_something_info(sig, &info, pid);
> }
>
> +/*
> + * Verify that the signaler and signalee either are in the same pid namespace
> + * or that the signaler's pid namespace is an ancestor of the signalee's pid
> + * namespace.
> + */
> +static bool may_signal_procfd(struct pid *pid)
> +{
> + struct pid_namespace *active = task_active_pid_ns(current);
> + struct pid_namespace *p = ns_of_pid(pid);
> +
> + for (;;) {
> + if (!p)
> + return false;
> + if (p == active)
> + break;
> + p = p->parent;
> + }
> +
> + return true;
> +}
> +
> +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t *kinfo, int flags,
> + bool had_siginfo)
> +{
> + int ret;
> + struct fd f;
> + struct pid *pid;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + /* Is this a process file descriptor? */
> + ret = -EINVAL;
> + if (!proc_is_tgid_procfd(f.file))
> + goto err;
> +
> + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> + pid = proc_pid(file_inode(f.file));
> + if (!pid)
> + goto err;
> +
> + if (!may_signal_procfd(pid))
> + goto err;
> +
> + if (had_siginfo) {
> + /*
> + * Not even root can pretend to send signals from the kernel.
> + * Nor can they impersonate a kill()/tgkill(), which adds
> + * source info.
> + */
> + ret = -EPERM;
> + if ((kinfo->si_code >= 0 || kinfo->si_code == SI_TKILL) &&
> + (task_pid(current) != pid))
> + goto err;
> + } else {
> + prepare_kill_siginfo(sig, kinfo);
> + }
> +
> + ret = kill_pid_info(sig, kinfo, pid);
> +
> +err:
> + fdput(f);
> + return ret;
> +}
> +
> +/**
> + * sys_procfd_signal - send a signal to a process through a process file
> + * descriptor
> + * @fd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + */
> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> + int, flags)
> +{
> + kernel_siginfo_t kinfo;
> +
> + if (info) {
> + int ret = __copy_siginfo_from_user(sig, &kinfo, info);
> + if (unlikely(ret))
> + return ret;
> + }
> +
> + return do_procfd_signal(fd, sig, &kinfo, flags, !!info);
> +}
> +
> +#ifdef CONFIG_COMPAT
> +COMPAT_SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig,
> + struct compat_siginfo __user *, info, int, flags)
> +{
> + kernel_siginfo_t kinfo;
> +
> + if (info) {
> + int ret = __copy_siginfo_from_user32(sig, &kinfo, info);
> + if (unlikely(ret))
> + return ret;
> + }
> +
> + return do_procfd_signal(fd, sig, &kinfo, flags, !!info);
> +}
> +#endif
> +
> static int
> do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> {
> --
> 2.19.1
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] signal: add procfd_signal() syscall
From: Serge E. Hallyn @ 2018-11-22 8:00 UTC (permalink / raw)
To: Christian Brauner
Cc: ebiederm, linux-kernel, serge, jannh, luto, akpm, oleg, cyphar,
viro, linux-fsdevel, linux-api, dancol, timmurray, linux-man,
Kees Cook
In-Reply-To: <20181120105124.14733-1-christian@brauner.io>
On Tue, Nov 20, 2018 at 11:51:23AM +0100, Christian Brauner wrote:
> The kill() syscall operates on process identifiers. After a process has
> exited its pid can be reused by another process. If a caller sends a signal
> to a reused pid it will end up signaling the wrong process. This issue has
> often surfaced and there has been a push [1] to address this problem.
>
> This patch uses file descriptors from proc/<pid> as stable handles on
> struct pid. Even if a pid is recycled the handle will not change. The file
> descriptor can be used to send signals to the referenced process.
> Thus, the new syscall procfd_signal() is introduced to solve this problem.
> It operates on a process file descriptor.
> The syscall takes an additional siginfo_t and flags argument. If siginfo_t
> is NULL then procfd_signal() behaves like kill() if it is not NULL it
> behaves like rt_sigqueueinfo.
> The flags argument is added to allow for future extensions of this syscall.
> It currently needs to be passed as 0.
>
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <signal.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> int ret;
> char buf[1000];
>
> if (argc < 2)
> exit(EXIT_FAILURE);
>
> ret = snprintf(buf, sizeof(buf), "/proc/%s", argv[1]);
> if (ret < 0)
|| ret > sizeof(buf) ? :-) I mean, you *are* passing the string...
> exit(EXIT_FAILURE);
>
> int fd = open(buf, O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), buf);
> exit(EXIT_FAILURE);
> }
>
> ret = syscall(__NR_procfd_signal, fd, SIGKILL, NULL, 0);
> if (ret < 0) {
> printf("Failed to send SIGKILL \"%s\"\n", strerror(errno));
> close(fd);
> exit(EXIT_FAILURE);
> }
>
> close(fd);
>
> exit(EXIT_SUCCESS);
> }
>
> [1]: https://lkml.org/lkml/2018/11/18/130
>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Serge Hallyn <serge@hallyn.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> Changelog:
> v2:
> - define __NR_procfd_signal in unistd.h
> - wire up compat syscall
> - s/proc_is_procfd/proc_is_tgid_procfd/g
> - provide stubs when CONFIG_PROC_FS=n
> - move proc_pid() to linux/proc_fs.h header
> - use proc_pid() to grab struct pid from /proc/<pid> fd
> v1:
> - patch introduced
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> fs/proc/base.c | 11 ++-
> fs/proc/internal.h | 5 -
> include/linux/proc_fs.h | 12 +++
> include/linux/syscalls.h | 2 +
> include/uapi/asm-generic/unistd.h | 4 +-
> kernel/signal.c | 127 +++++++++++++++++++++++--
> 8 files changed, 151 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..3f27ffd8ae87 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..8a30cde82450 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 64 procfd_signal __x64_sys_procfd_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> @@ -386,3 +387,4 @@
> 545 x32 execveat __x32_compat_sys_execveat/ptregs
> 546 x32 preadv2 __x32_compat_sys_preadv64v2
> 547 x32 pwritev2 __x32_compat_sys_pwritev64v2
> +548 x32 procfd_signal __x32_compat_sys_procfd_signal
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ce3465479447..771c6bd1cac6 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -716,7 +716,10 @@ static int proc_pid_permission(struct inode *inode, int mask)
> return generic_permission(inode, mask);
> }
>
> -
> +struct pid *proc_pid(const struct inode *inode)
> +{
> + return PROC_I(inode)->pid;
> +}
>
> static const struct inode_operations proc_def_inode_operations = {
> .setattr = proc_setattr,
> @@ -3038,6 +3041,12 @@ static const struct file_operations proc_tgid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tgid_base_operations);
> +}
> +
> static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> {
> return proc_pident_lookup(dir, dentry,
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 5185d7f6a51e..eb69afba83f3 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -113,11 +113,6 @@ static inline void *__PDE_DATA(const struct inode *inode)
> return PDE(inode)->data;
> }
>
> -static inline struct pid *proc_pid(const struct inode *inode)
> -{
> - return PROC_I(inode)->pid;
> -}
> -
> static inline struct task_struct *get_proc_task(const struct inode *inode)
> {
> return get_pid_task(proc_pid(inode), PIDTYPE_PID);
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..96df2fe6311d 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> int (*show)(struct seq_file *, void *),
> proc_write_t write,
> void *data);
> +extern bool proc_is_tgid_procfd(const struct file *file);
> +extern struct pid *proc_pid(const struct inode *inode);
>
> #else /* CONFIG_PROC_FS */
>
> @@ -114,6 +116,16 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
> #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
>
> +static inline bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return false;
> +}
> +
> +static inline struct pid *proc_pid(const struct inode *inode)
> +{
> + return NULL;
> +}
> +
> #endif /* CONFIG_PROC_FS */
>
> struct net;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..a5ca8cb84566 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -907,6 +907,8 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_procfd_signal(int fd, int sig, siginfo_t __user *info,
> + int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 538546edbfbd..4dc81a994ad1 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
> __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
> #define __NR_rseq 293
> __SYSCALL(__NR_rseq, sys_rseq)
> +#define __NR_procfd_signal 294
> +__SC_COMP(__NR_procfd_signal, sys_procfd_signal, compat_sys_procfd_signal)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 294
> +#define __NR_syscalls 295
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a32bc2088c9..13695342f150 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -19,7 +19,9 @@
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/sched/cputime.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/proc_fs.h>
> #include <linux/tty.h>
> #include <linux/binfmts.h>
> #include <linux/coredump.h>
> @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> }
> #endif
>
> +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +{
> + clear_siginfo(info);
> + info->si_signo = sig;
> + info->si_errno = 0;
> + info->si_code = SI_USER;
> + info->si_pid = task_tgid_vnr(current);
> + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +}
> +
> /**
> * sys_kill - send a signal to a process
> * @pid: the PID of the process
> @@ -3295,16 +3307,119 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> {
> struct kernel_siginfo info;
>
> - clear_siginfo(&info);
> - info.si_signo = sig;
> - info.si_errno = 0;
> - info.si_code = SI_USER;
> - info.si_pid = task_tgid_vnr(current);
> - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> + prepare_kill_siginfo(sig, &info);
>
> return kill_something_info(sig, &info, pid);
> }
>
> +/*
> + * Verify that the signaler and signalee either are in the same pid namespace
> + * or that the signaler's pid namespace is an ancestor of the signalee's pid
> + * namespace.
> + */
> +static bool may_signal_procfd(struct pid *pid)
> +{
> + struct pid_namespace *active = task_active_pid_ns(current);
> + struct pid_namespace *p = ns_of_pid(pid);
> +
> + for (;;) {
> + if (!p)
> + return false;
> + if (p == active)
> + break;
> + p = p->parent;
> + }
> +
> + return true;
> +}
> +
> +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t *kinfo, int flags,
> + bool had_siginfo)
> +{
> + int ret;
> + struct fd f;
> + struct pid *pid;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + /* Is this a process file descriptor? */
> + ret = -EINVAL;
> + if (!proc_is_tgid_procfd(f.file))
> + goto err;
> +
> + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> + pid = proc_pid(file_inode(f.file));
> + if (!pid)
> + goto err;
> +
> + if (!may_signal_procfd(pid))
> + goto err;
> +
> + if (had_siginfo) {
> + /*
> + * Not even root can pretend to send signals from the kernel.
> + * Nor can they impersonate a kill()/tgkill(), which adds
> + * source info.
> + */
> + ret = -EPERM;
> + if ((kinfo->si_code >= 0 || kinfo->si_code == SI_TKILL) &&
> + (task_pid(current) != pid))
> + goto err;
> + } else {
> + prepare_kill_siginfo(sig, kinfo);
> + }
> +
> + ret = kill_pid_info(sig, kinfo, pid);
> +
> +err:
> + fdput(f);
> + return ret;
> +}
> +
> +/**
> + * sys_procfd_signal - send a signal to a process through a process file
> + * descriptor
> + * @fd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + */
> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> + int, flags)
> +{
> + kernel_siginfo_t kinfo;
> +
> + if (info) {
> + int ret = __copy_siginfo_from_user(sig, &kinfo, info);
> + if (unlikely(ret))
> + return ret;
> + }
> +
> + return do_procfd_signal(fd, sig, &kinfo, flags, !!info);
> +}
> +
> +#ifdef CONFIG_COMPAT
> +COMPAT_SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig,
> + struct compat_siginfo __user *, info, int, flags)
> +{
> + kernel_siginfo_t kinfo;
> +
> + if (info) {
> + int ret = __copy_siginfo_from_user32(sig, &kinfo, info);
> + if (unlikely(ret))
> + return ret;
> + }
> +
> + return do_procfd_signal(fd, sig, &kinfo, flags, !!info);
> +}
> +#endif
> +
> static int
> do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> {
> --
> 2.19.1
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Serge E. Hallyn @ 2018-11-22 7:48 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, ebiederm, linux-kernel, serge, jannh, luto,
akpm, oleg, cyphar, viro, linux-fsdevel, linux-api, dancol,
timmurray, linux-man, Kees Cook
In-Reply-To: <20181119223954.GA4992@cisco>
On Mon, Nov 19, 2018 at 03:39:54PM -0700, Tycho Andersen wrote:
> On Mon, Nov 19, 2018 at 11:32:39AM +0100, Christian Brauner wrote:
> >
> > +/**
> > + * sys_procfd_signal - send a signal to a process through a process file
> > + * descriptor
> > + * @fd: the file descriptor of the process
> > + * @sig: signal to be sent
> > + * @info: the signal info
> > + * @flags: future flags to be passed
> > + */
> > +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info,
> > + int, flags)
> > +{
>
> Can I just register an objection here that I think using a syscall
> just for this is silly?
>
> My understanding is that the concern is that some code might do:
>
> unknown_fd = recv_fd();
> ioctl(unknown_fd, SOME_IOCTL, NULL); // where SOME_IOCTL == PROC_FD_KILL
> // whoops, unknown_fd was a procfd and we killed a task!
This could just be my own mental model, but for something like "kill a
task", an ioctl just seems wrong. Syscall seems more natural.
I'd ack either method.
-serge
^ permalink raw reply
* Re: [PATCH v1 2/2] signal: add procfd_signal() syscall
From: Serge E. Hallyn @ 2018-11-22 7:41 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, ebiederm, linux-kernel, serge, jannh, luto,
akpm, oleg, viro, linux-fsdevel, linux-api, dancol, timmurray,
linux-man, Kees Cook
In-Reply-To: <20181119212343.yikfoxob7f4hio7h@yavin>
On Tue, Nov 20, 2018 at 08:23:43AM +1100, Aleksa Sarai wrote:
> On 2018-11-20, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > On Tue, Nov 20, 2018 at 07:28:57AM +1100, Aleksa Sarai wrote:
> > > > On 2018-11-19, Christian Brauner <christian@brauner.io> wrote:
> > > > > + if (info) {
> > > > > + ret = __copy_siginfo_from_user(sig, &kinfo, info);
> > > > > + if (unlikely(ret))
> > > > > + goto err;
> > > > > + /*
> > > > > + * Not even root can pretend to send signals from the kernel.
> > > > > + * Nor can they impersonate a kill()/tgkill(), which adds
> > > > > + * source info.
> > > > > + */
> > > > > + ret = -EPERM;
> > > > > + if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
> > > > > + (task_pid(current) != pid))
> > > > > + goto err;
> > > > > + } else {
> > > > > + prepare_kill_siginfo(sig, &kinfo);
> > > > > + }
> > > >
> > > > I wonder whether we should also have a pidns restriction here, since
> > > > currently it isn't possible for a container process using a pidns to
> > > > signal processes outside its pidns. AFAICS, this isn't done through an
> > > > explicit check -- it's a side-effect of processes in a pidns not being
> > > > able to address non-descendant-pidns processes.
> > > >
> > > > But maybe it's reasonable to allow sending a procfd to a different pidns
> > > > and the same operations working on it? If we extend the procfd API to
> > >
> > > No, I don't think so. I really don't want any fancy semantics in here.
> > > Fancy doesn't get merged and fancy is hard to maintain. So we should do
> > > something like:
> > >
> > > if (proc_pid_ns() != current_pid_ns)
> > > return EINVAL
> >
> > This isn't quite sufficient. The key thing is that you have to be in an
> > *ancestor* (or same) pidns, not the *same* pidns. Ideally you can re-use
> > the check already in pidns_get_parent, and expose it. It would be
> > something as trivial as:
> >
> > bool pidns_is_descendant(struct pid_namespace *ns,
> > struct pid_namespace *ancestor)
> > {
> > for (;;) {
> > if (!ns)
> > return false;
> > if (ns == ancestor)
> > break;
> > ns = ns->parent;
> > }
> > return true;
> > }
> >
> > 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;
>
> Scratch the last bit, -EPERM is wrong here. I would argue that -EINVAL
> is *somewhat* wrong because arguable the more semantically consistent
> error (with kill(2)) would be -ESRCH -- but then you're mixing the "pid
> is dead" and "pid is not visible to you" cases. I'm not sure what the
> right errno would be here (I'm sure some of the LKML greybeards will
> have a better clue.) :P
Actually I like EXDEV for this. ERMOTE also works.
^ permalink raw reply
* Re: [PATCH v2] Add /proc/pid_gen
From: Daniel Colascione @ 2018-11-22 5:30 UTC (permalink / raw)
To: Tim Murray
Cc: Andrew Morton, linux-kernel, Linux API, 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
In-Reply-To: <CAEe=SxmNVHR_1ge6hM5636CNi4BZhLvyxoKW_28L9-XDV7x6aQ@mail.gmail.com>
On Wed, Nov 21, 2018 at 6:36 PM Tim Murray <timmurray@google.com> wrote:
>
> 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.
I was imagining periodic scans being the moral equivalent of I-frames
in an MPEG stream: known-good synchronization points that bound the
propagation of errors. To prevent loss of events in the *middle* of
one of these samples affecting the validity of the sample, I wanted
each one to be atomic and self-contained. I was initially imagining a
single proc file containing a mapping from thread ID list and a TGID
mapping for each one (as well as assorted other information),
generated atomically under task_list_lock and stamped with the trace
timestamp. The /proc/pid_gen thing was an effort to do most of that in
userspace. But maybe that's not really that important.
Another thing to consider: there are some properties which we can
enumerate *only* through /proc and that we can't yet receive in a
streaming, event-driven fashion. We read those by periodic sampling,
but the sampling isn't atomic. And as Tim notes, even properties that
we ordinarily receive in streaming form via trace event must be
sampled at trace start. But trace start isn't the only case where it
matters.
Suppose at T+50 we have a TID 601 (which we'll call TID 601_1) with
steady-state resource-consumption $COUNTER 100MB. Now suppose we start
sampling at T+50. At T+55, TID 601_1 dies. At T+60, due to unlikely be
possible TID reuse, we see a new task with TID 601, which we'll call
TID 601_2. At T+65, the sample reads TID 601_2's $COUNTER as 1GB. At
T+70, we finish the sample. From the perspective of trace analysis,
all the sample tells us is "at some point between T+50 and T+70, TID
601's $COUNTER was 1GB". We also know when TID 601_1 died and when TID
601_2 started. We don't know whether the sample's "TID 601" refers to
TID 601_1 or TID 601_2. So what do we do?
1) We could attribute the 1GB value of $COUNTER to TID 601_1, which is
what would happen by default if we deemed the sample to be a virtual
event with timestamp T+50, the time we started the sampling process.
But that would be incorrect here. In analysis, we'd see this
attribution as a mysterious spike in TID 601_1's resource consumption
immediately before its death. (The spike would be to a level
suspiciously similar to TID 601_2's next sample.) Imagine spending
hours trying to track down a mystery death-rattle resource spike
(something that could plausibly happen in the real world) that ended
up being a tracing artifact.
2) We could attribute the 1GB read of $COUNTER to TID 601_2. In the
timeline I gave above, this attribution would be correct. But what if
TID 601_1's $COUNTER really _had_ spiked to 1GB immediately before its
death? (What if its death had been caused by that spike?) Then, under
the "tag last" approach, we'd falsely tag TID 601_2 with TID 601_1's
excessive resource use, and this false attribution would last until
the next sample of TID 601_2, which might be quite far in the future.
And we might find ourselves wondering why TID 601_1 died, since (if we
blame 601_2 for the new $COUNTER value) TID 601_1 would have done
nothing wrong. And in the meantime, we'd see 1GB of $COUNTER
consumption that would appear to come out of nowhere.
3) We could discard the ambiguous sample. But in this case, we'd leave
TID 601_2's $COUNTER value completely unknown until the next sample,
which could be quite far in the future. And, if TID 601_2's $COUNTER
value really were 1GB, we might leave significant resource consumption
unaccounted-for until that next sample.
With my pid_gen patch, we could have used strategy #3 without having
to wait for the system to get around to sampling TID 601_2 again on
its own, because we'd have been able to detect the ambiguity and
collect a new, hopefully-unambiguous sample on the spot. Yes, we do
wait a sampling period between samples anyway, and it's possible for a
process to start, run, and die without having ever been sampled with
or without pid_gen, but at least we wouldn't have missed a perfectly
good sampling opportunity due to PID ambiguity.
You might say, "dancol@, you sound excessively fussy here. That's
never going to really matter under any realistic workload." And maybe
that'd be right. But like I said above, I think it's important to be
able to have deterministic bounds on uncertainty even in the case of
pathological behavior, and I'd rather guarantee correctness
structurally than wonder whether unlikely events are one-in-a-trace
unlikely or heat-death-of-the-universe unlikely.
To improve on the sampling model, we could have the kernel stamp
$COUNTER reads with the current trace timestamp. Then, the sample
would tell us, instead of "TID 601's $COUNTER was 1GB between T+50 and
T+70", it'd tell us "TID 601's $COUNTER was 1GB at exactly T+65". (The
sampling program itself might get around to logging this bit of
information at T+70 or T+700, but the embedded timestamp would be
accurate.) We could then look up the inferred process table at T+65
and attribute that 1GB to TID 601_2.
Ideally, we wouldn't bother with this sort of sampling at all, instead
relying on streaming $COUNTER updates as events. But we're not quite
there yet. I have a larger proposal in the works for that one. Under
that proposal, we wouldn't log every change to $COUNTER, but we would
log $COUNTER at process death (as well as at other times ---
basically, after large changes and after a maximum time after any
change), so trace analysis would still be able to correctly determine
TID 601_1's time-of-death resource use.
In another approach, the trace system *itself* would generate the
"I-frames" by automatically generating synthetic trace events at trace
startup in order to reflect pre-trace system state. (For example, if
you enabled sched_switch and started tracing, the ftrace ring buffer
would begin life with one synthetic sched_switch per CPU describing
each CPU's running task at the instant of trace startup, maybe
collected with on_all_cpus.)
But anyway, I didn't expect the pid_gen change to be controversial.
Given that we can eventually do something different altogether and
that it probably *is* excessively fussy, consider the patch withdrawn.
^ permalink raw reply
* Re: [PATCH -next 1/2] mm/memfd: make F_SEAL_FUTURE_WRITE seal more robust
From: Andy Lutomirski @ 2018-11-22 3:25 UTC (permalink / raw)
To: Andrew Morton
Cc: Joel Fernandes, Stephen Rothwell, Andrew 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: <20181121182701.0d8a775fda6af1f8d2be8f25@linux-foundation.org>
On Wed, Nov 21, 2018 at 6:27 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> 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
PROT_WRITE, perhaps?
> + * 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);
> +
This might all be clearer as:
if (info->seals & F_SEAL_FUTURE_WRITE) {
if (vma->vm_flags ...)
return -EPERM;
vma->vm_flags &= ~VM_MAYWRITE;
}
with appropriate comments inserted.
^ 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