* Re: RFC: userspace exception fixups
From: Sean Christopherson @ 2018-11-02 16:30 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Rich Felker, Jann Horn, Dave Hansen,
Jethro Beekman, Jarkko Sakkinen, Florian Weimer, Linux API,
X86 ML, linux-arch, LKML, Peter Zijlstra, nhorman, npmccallum,
Ayoun, Serge, shay.katz-zamir, linux-sgx, Andy Shevchenko,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Carlos O'Donell
In-Reply-To: <CALCETrWe4+apXJNswHAKVVqajGS3jTEKxdd2r3iu-MzGK1v0DA@mail.gmail.com>
On Thu, Nov 01, 2018 at 04:22:55PM -0700, Andy Lutomirski wrote:
> On Thu, Nov 1, 2018 at 2:24 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Thu, Nov 1, 2018 at 12:31 PM Rich Felker <dalias@libc.org> wrote:
> > >
> > > See my other emails in this thread. You would register the *address*
> > > (in TLS) of a function pointer object pointing to the handler, rather
> > > than the function address of the handler. Then switching handler is
> > > just a single store in userspace, no syscalls involved.
> >
> > Yes.
> >
> > And for just EENTER, maybe that's the right model.
> >
> > If we want to generalize it to other thread-synchronous faults, it
> > needs way more information and a list of handlers, but if we limit the
> > thing to _only_ EENTER getting an SGX fault, then a single "this is
> > the fault handler" address is probably the right thing to do.
>
> It sounds like you're saying that the kernel should know, *before*
> running any user fixup code, whether the fault in question is one that
> wants a fixup. Sounds reasonable.
>
> I think it would be nice, but not absolutely necessary, if user code
> didn't need to poke some value into TLS each time it ran a function
> that had a fixup. With the poke-into-TLS approach, it looks a lot
> like rseq, and rseq doesn't nest very nicely. I think we really want
> this mechanism to Just Work. So we could maybe have a syscall that
> associates a list of fixups with a given range of text addresses. We
> might want the kernel to automatically zap the fixups when the text in
> question is unmapped.
If this is EENTER specific then nesting isn't an issue. But I don't
see a simple way to restrict the mechanism to EENTER.
What if rather than having userspace register an address for fixup the
kernel instead unconditionally does fixup on the ENCLU opcode? For
example, skip the instruction and put fault info into some combination
of RDX/RSI/RDI (they're cleared on asynchronous enclave exits).
The decode logic is straightforward since ENCLU doesn't have operands,
we'd just have to eat any ignored prefixes. The intended convention
for EENTER is to have an ENCLU at the AEX target (to automatically do
ERESUME after INTR, etc...), so this would work regardless of whether
the fault happened on EENTER or in the enclave. EENTER/ERESUME are
the only ENCLU functions that are allowed outside of an enclave so
there's no danger of accidentally crushing something else.
This way we wouldn't need a VDSO blob and we'd enforce the kernel's
ABI, e.g. a library that tried to use signal handling would go off the
rails when the kernel mucked with the registers. We could even have
the SGX EPC fault handler return VM_FAULT_SIGBUS if the faulting
instruction isn't ENCLU, e.g. to further enforce that the AEX target
needs to be ENCLU.
Userspace would look something like this:
mov tcs, %xbx /* Thread Control Structure address */
leaq async_exit(%rip), %rcx /* AEX target for EENTER/RESUME */
mov $SGX_EENTER, %rax /* EENTER leaf */
async_exit:
ENCLU
fault_handler:
<handle fault>
enclave_exit: /* EEXIT target */
<handle enclave request>
^ permalink raw reply
* Re: Supporting core-specific instruction sets (e.g. big.LITTLE) with restartable sequences
From: Mark Rutland @ 2018-11-02 16:08 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Richard Henderson, Will Deacon, linux-kernel, libc-alpha,
Carlos O'Donell, Florian Weimer, Joseph Myers, Szabolcs Nagy,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Dave Watson, Paul Turner, linux-api
In-Reply-To: <313542172.8.1541171544337.JavaMail.zimbra@efficios.com>
Hi Mathieu, Richard,
On Fri, Nov 02, 2018 at 11:12:24AM -0400, Mathieu Desnoyers wrote:
> Hi Richard,
>
> I stumbled on these articles:
>
> - https://medium.com/@jadr2ddude/a-big-little-problem-a-tale-of-big-little-gone-wrong-e7778ce744bb
> - https://www.mono-project.com/news/2016/09/12/arm64-icache/
>
> and discussed them with Will Deacon. He told me you were looking into
> gcc atomics and it might be worthwhile to discuss the possible use of
> the new rseq system call that has been added in Linux 4.18 for those
> use-cases.
>
> Basically, the use-cases targeted are those where some cores on the
> system support a larger instruction set than others. So for instance,
> some cores could use a faster atomic add instruction than others,
> which should rely on a slower fallback. This is also the same story
> for reading the performance monitoring unit counters from user-space:
> it depends on the feature-set supported by the CPU on which the
> instruction is issued. Same applies to cores having different
> cache-line sizes.
Please note that upstream arm64 Linux does not expose mismatched ISA
feature to userspace. We go to great pains to expose a uniform set of
supported features.
The two issues referenced above are both handled by the kernel, and no
userspace changes are required to handle them.
We do not intend or expect to expose mismatched features to userspace.
Correctly-written userspace should not use optional instructions unless
the kernel has advertised their presence via a hwcap (or via ID register
emulation).
> The main problem is that the kernel can migrate a thread at any point
> between user-space reading the current cpu number and issuing the
> instruction. This is where rseq can help.
>
> The core idea to solve the instruction set issue is to set a mask of
> cpus supporting the new instruction in a library constructor, and then
> load cpu_id, use it with the mask, and branch to either the new or old
> instruction, all with a rseq critical section. If the kernel needs to
> abort due to preemption or signal delivery, the abort behavior would
> be to issue the fallback (slow) atomic operation, which guarantees
> progress even if single-stepping.
>
> As long as the load, test and branch is faster than the performance
> delta between the old and new atomic instruction, it would be worth
> it.
Specifically w.r.t. the atomics, the kernel will only expose the
presence of the ARMv8.1 atomic instructions when supported by all CPUs
in the system.
> In the case of PMU read from user-space, using rseq to figure out how
> to issue the PMU read enables a use-case which is not otherwise
> possible to do on big.LITTLE. On rseq abort, it would fallback to a
> system call to read the PMU counter. This abort behavior guarantees
> forward progress.
We do not currently expose any PMU registers to userspace. If we were to
expose them for big.LITTLE, rseq may be of use, but no-one has done the
groundwork to investigate this.
> The second article is about cache line size discrepancy between CPUs.
> Here again, doing the cacheline flushing in a rseq critical section
> could allow tuning it to characteristics of the actual core it is
> running on. The fast-path would use a stride fitting the current core
> characteristics, and if rseq needs to abort, the slow-path would
> fall-back to a conservative value which would fit all cores (smaller
> cache line size on the overall system).
This is already handled by the kernel, and the proposed rseq approach is
not correct -- cache maintenance must *always* use the system-wide
minimum cacheline size, or stale entries will be left on some CPUs,
which will result in later failures.
Thanks,
Mark.
^ permalink raw reply
* Re: [RFC PATCH 1/2] glibc: Perform rseq(2) registration at nptl init and thread creation (v3)
From: Mathieu Desnoyers @ 2018-11-02 15:33 UTC (permalink / raw)
To: Andy Lutomirski
Cc: libc-alpha, carlos, Florian Weimer, Joseph Myers, Szabolcs Nagy,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <CALCETrWG4j7R7jzuoPW6ifYVzwuQGMEHBp7Pb_k66_2hn1wUHg@mail.gmail.com>
----- On Nov 2, 2018, at 4:20 PM, Andy Lutomirski luto@amacapital.net wrote:
> On Fri, Nov 2, 2018 at 4:53 AM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Here is a third round of prototype registering rseq(2) TLS for each
>> thread (including main), and unregistering for each thread (excluding
>> main). "rseq" stands for Restartable Sequences.
>>
>> Remaining open questions:
>>
>> - How early do we want to register rseq and how late do we want to
>> unregister it ? It's important to consider if we expect rseq to
>> be used by the memory allocator and within destructor callbacks.
>> However, we want to be sure the TLS (__thread) area is properly
>> allocated across its entire use by rseq.
>>
>> - We do not need an atomic increment/decrement for the refcount per
>> se. Just being atomic with respect to the current thread (and nested
>> signals) would be enough. What is the proper API to use there ?
>>
>> See the rseq(2) man page proposed here:
>> https://lkml.org/lkml/2018/9/19/647
>>
>
> Merely having rseq registered carries some small but nonzero overhead,
> right?
There is indeed a small overhead at thread creation/exit (total of 2
system calls) and one system call in nptl init. Once registered, there
is very small, infrequent, a hard to measure overhead at thread preemption
and signal delivery.
> Should this perhaps live in a librseq.so or similar (possibly
> built as part of libc) to avoid the overhead for programs that don't
> use it?
My second patch modifies sched_getcpu() to use rseq. Another use-case
glibc guys want is to use rseq for malloc(). Once that is done, there
will be pretty much no program left using glibc facilities that won't
use rseq when available.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH 1/2] glibc: Perform rseq(2) registration at nptl init and thread creation (v3)
From: Andy Lutomirski @ 2018-11-02 15:20 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: libc-alpha, Carlos O'Donell, Florian Weimer, Joseph S. Myers,
szabolcs.nagy, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, LKML, Linux API
In-Reply-To: <20181102115259.11383-1-mathieu.desnoyers@efficios.com>
On Fri, Nov 2, 2018 at 4:53 AM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> Here is a third round of prototype registering rseq(2) TLS for each
> thread (including main), and unregistering for each thread (excluding
> main). "rseq" stands for Restartable Sequences.
>
> Remaining open questions:
>
> - How early do we want to register rseq and how late do we want to
> unregister it ? It's important to consider if we expect rseq to
> be used by the memory allocator and within destructor callbacks.
> However, we want to be sure the TLS (__thread) area is properly
> allocated across its entire use by rseq.
>
> - We do not need an atomic increment/decrement for the refcount per
> se. Just being atomic with respect to the current thread (and nested
> signals) would be enough. What is the proper API to use there ?
>
> See the rseq(2) man page proposed here:
> https://lkml.org/lkml/2018/9/19/647
>
Merely having rseq registered carries some small but nonzero overhead,
right? Should this perhaps live in a librseq.so or similar (possibly
built as part of libc) to avoid the overhead for programs that don't
use it?
^ permalink raw reply
* Supporting core-specific instruction sets (e.g. big.LITTLE) with restartable sequences
From: Mathieu Desnoyers @ 2018-11-02 15:12 UTC (permalink / raw)
To: Richard Henderson
Cc: Will Deacon, linux-kernel, libc-alpha, Carlos O'Donell,
Florian Weimer, Joseph Myers, Szabolcs Nagy, Thomas Gleixner,
Ben Maurer, Peter Zijlstra, Paul E. McKenney, Boqun Feng
Hi Richard,
I stumbled on these articles:
- https://medium.com/@jadr2ddude/a-big-little-problem-a-tale-of-big-little-gone-wrong-e7778ce744bb
- https://www.mono-project.com/news/2016/09/12/arm64-icache/
and discussed them with Will Deacon. He told me you were looking into gcc atomics and it might be
worthwhile to discuss the possible use of the new rseq system call that has been added in Linux 4.18
for those use-cases.
Basically, the use-cases targeted are those where some cores on the system support a larger instruction
set than others. So for instance, some cores could use a faster atomic add instruction than others, which
should rely on a slower fallback. This is also the same story for reading the performance monitoring
unit counters from user-space: it depends on the feature-set supported by the CPU on which the instruction
is issued. Same applies to cores having different cache-line sizes.
The main problem is that the kernel can migrate a thread at any point between user-space reading the
current cpu number and issuing the instruction. This is where rseq can help.
The core idea to solve the instruction set issue is to set a mask of cpus supporting the new instruction
in a library constructor, and then load cpu_id, use it with the mask, and branch to either the new or
old instruction, all with a rseq critical section. If the kernel needs to abort due to preemption or
signal delivery, the abort behavior would be to issue the fallback (slow) atomic operation, which
guarantees progress even if single-stepping.
As long as the load, test and branch is faster than the performance delta between the old and new atomic
instruction, it would be worth it.
In the case of PMU read from user-space, using rseq to figure out how to issue the PMU read enables a
use-case which is not otherwise possible to do on big.LITTLE. On rseq abort, it would fallback to a
system call to read the PMU counter. This abort behavior guarantees forward progress.
The second article is about cache line size discrepancy between CPUs. Here again, doing the cacheline
flushing in a rseq critical section could allow tuning it to characteristics of the actual core it is
running on. The fast-path would use a stride fitting the current core characteristics, and if rseq
needs to abort, the slow-path would fall-back to a conservative value which would fit all cores (smaller
cache line size on the overall system). Once again, this abort behavior guarantees forward progress.
This would only work, of course, if cacheline invalidation done on a big core end up being propagated
to other cores in a way that clears all the cache lines corresponding to the one targeted on the big
core.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-11-02 13:50 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181102112903.GB12360@redhat.com>
On Fri, Nov 02, 2018 at 12:29:03PM +0100, Oleg Nesterov wrote:
> On 11/01, Tycho Andersen wrote:
> >
> > On Thu, Nov 01, 2018 at 03:48:05PM +0100, Oleg Nesterov wrote:
> > >
> > > > > But my main concern is that either way wait_for_completion_killable() allows
> > > > > to trivially create a process which doesn't react to SIGSTOP, not good...
> > > > >
> > > > > Note also that this can happen if, say, both the tracer and tracee run in the
> > > > > same process group and SIGSTOP is sent to their pgid, if the tracer gets the
> > > > > signal first the tracee won't stop.
> > > > >
> > > > > Of freezer. try_to_freeze_tasks() can fail if it freezes the tracer before
> > > > > it does SECCOMP_IOCTL_NOTIF_SEND.
> > > >
> > > > I think in general the way this is intended to be used these things
> > > > wouldn't happen.
> > >
> > > Why?
> >
> > The intent is to run the tracer on the host and have it trace
> > containers, which would live in a different freezer cgroup, process
> > group, etc.
>
> I didn't mean the freezer cgroup, suspend can fail, it does the "global" freeze.
> Nevermind.
>
> > > Yes I think it would be nice to avoid wait_for_completion_killable().
> > >
> > > So please help me to understand the problem. Once again, why can not
> > > seccomp_do_user_notification() use wait_for_completion_interruptible() only?
> > >
> > > This is called before the task actually starts the syscall, so
> > > -ERESTARTNOINTR if signal_pending() can't hurt.
> >
> > The idea was that when the tracee gets a signal, it notifies the
> > tracer exactly once, and then waits for the tracer to decide what to
> > do. So if we use another wait_for_completion_interruptible(), doesn't
> > it just get re-woken immediately because the signal is still pending?
>
> Hmm. I meant that we should use a single wait_for_completion_interruptible().
Yes, but if we can use a second _interruptible(), then we can avoid
the SIGSTOP issue and still perhaps preserve the SIGNALED bit if we
decide it's worth it at the conclusion of this thread.
> > > Now lets suppose seccomp_do_user_notification() simply does
> > >
> > > err = wait_for_completion_interruptible(&n.ready);
> > >
> > > if (err < 0 && state != SECCOMP_NOTIFY_REPLIED) {
> > > syscall_set_return_value(ERESTARTNOINTR);
> > > list_del(&n.list);
> > > return -1;
> > > }
> > >
> > > (I am ignoring the locking/etc). Now the obvious problem is that the listener
> > > doing SECCOMP_IOCTL_NOTIF_SEND can't distinguish -ENOENT from the case when the
> > > tracee was killed, yes?
> > >
> > > Is it that important?
> >
> > The answer to this question depends on how we want the listener to be
> > able to react. For example, if the listener is in the middle of doing
> > a mount() on behalf of the task and it gets a signal and we return
> > immediately, the listener will complete the mount(), try to respond
> > with success and get -ENOENT.
>
> Yes. Should we undo the mount if the tracee is killed?
>
> > If the task handles the signal and
> > restarts the mount(), it'll happen twice unless the listener undoes
> > it when it sees the -ENOENT.
>
> Yes. But note that we know that if the same tracee sends another notification
> it must be the same syscall.
>
> So. If the listener needs to undo mount when the tracee is killed, it should
> undo it if it was interrupted too.
>
> If no, the listener can simply "ignore" the next notification and do
> SECCOMP_IOCTL_NOTIF_SEND(val = 0, error = 0).
>
> I see no real difference...
Well, doesn't it seem like a hack? What if the tracee really makes the
same syscall twice? How do we tell the difference between one that was
restarted and a real second call?
> > If we send another notification with the
> > SIGNALED flag, the listener has a better picture of what's going on,
> > which might be nice.
>
> Yes, but this returns us to my my question: Is it that important?
>
> What exactly the listener can do if it gets SECCOMP_NOTIF_FLAG_SIGNALED?
>
> Undo the mount? No. This doesn't differ from the case when the tracee gets
> the non-fatal signal right after SECCOMP_NOTIFY_REPLIED.
I guess that in do_user_notification(), even if we get -EINTER from
the wait, we should check to see that the state is REPLIED. If it is,
we can use the err and val from it, and complete the syscall as
normal, since it did.
Tycho
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-11-02 13:38 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181102100234.GA12360@redhat.com>
On Fri, Nov 02, 2018 at 11:02:35AM +0100, Oleg Nesterov wrote:
> On 11/01, Tycho Andersen wrote:
> >
> > On Thu, Nov 01, 2018 at 02:40:02PM +0100, Oleg Nesterov wrote:
> > >
> > > Somehow I no longer understand why do you need to take all locks. Isn't
> > > the first filter's notify_lock enough? IOW,
> > >
> > > for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> > > if (cur->notif)
> > > return ERR_PTR(-EBUSY);
> > > first = cur;
> > > }
> > >
> > > if (first)
> > > mutex_lock(&first->notify_lock);
> > >
> > > ... initialize filter->notif ...
> > >
> > > out:
> > > if (first)
> > > mutex_unlock(&first->notify_lock);
> > >
> > > return ret;
> >
> > The idea here is to prevent people from "nesting" notify filters. So
> > if any filter in the chain has a listener attached, it refuses to
> > install another filter with a listener.
>
> Yes, I understand, so we need to check cur->notif. My point was, we do not
> need to take all the locks in the ->prev chain, we need only one:
> first->notify_lock.
>
> But you know what? today I think that we do not need any locking at all,
> all we need is the lockless
>
> for (cur = current->seccomp.filter; cur; cur = cur->prev)
> if (cur->notif)
> return ERR_PTR(-EBUSY);
>
> at the start, nothing more.
Hmm, you're right. The locking is residual from when the old ptrace()
API was also a thing: with that, you could attach a filter at any
point in the tree, so we needed to lock to prevent that. But now
that it's only possible to do it at the bottom, we don't need to lock.
> > But it just occurred to me that we don't handle the TSYNC case
> > correctly by doing it this way,
>
> Why? Perhaps I missed your point, but TSYNC case looks fine. I mean, if 2
> threads do seccomp_set_mode_filter(NEW_LISTENER | TSYNC) then only one can
> win the race and succeed, but this has nothing to do with init_listener(),
> we rely on ->siglock and is_ancestor() check.
Yes, agreed.
Thanks!
Tycho
^ permalink raw reply
* [RFC PATCH 2/2] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux
From: Mathieu Desnoyers @ 2018-11-02 11:52 UTC (permalink / raw)
To: libc-alpha
Cc: Mathieu Desnoyers, Carlos O'Donell, Florian Weimer,
Joseph Myers, Szabolcs Nagy, Thomas Gleixner, Ben Maurer,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Will Deacon,
Dave Watson, Paul Turner, linux-kernel, linux-api
In-Reply-To: <20181102115259.11383-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
* [RFC PATCH 1/2] glibc: Perform rseq(2) registration at nptl init and thread creation (v3)
From: Mathieu Desnoyers @ 2018-11-02 11:52 UTC (permalink / raw)
To: libc-alpha
Cc: Mathieu Desnoyers, Carlos O'Donell, Florian Weimer,
Joseph Myers, Szabolcs Nagy, Thomas Gleixner, Ben Maurer,
Peter Zijlstra, Paul E. McKenney, Boqun Feng, Will Deacon,
Dave Watson, Paul Turner, linux-kernel, linux-api
Here is a third round of prototype registering rseq(2) TLS for each
thread (including main), and unregistering for each thread (excluding
main). "rseq" stands for Restartable Sequences.
Remaining open questions:
- How early do we want to register rseq and how late do we want to
unregister it ? It's important to consider if we expect rseq to
be used by the memory allocator and within destructor callbacks.
However, we want to be sure the TLS (__thread) area is properly
allocated across its entire use by rseq.
- We do not need an atomic increment/decrement for the refcount per
se. Just being atomic with respect to the current thread (and nested
signals) would be enough. What is the proper API to use there ?
See the rseq(2) man page proposed here:
https://lkml.org/lkml/2018/9/19/647
This patch is based on glibc 2.28.
To try it out, refer to the following kernel and librseq development
branches:
* rseq and cpu_opv:
https://github.com/compudj/linux-percpu-dev branch: rseq/dev-local
* librseq:
https://github.com/compudj/librseq branch: master
TODO:
- Add documentation, tests and a NEWS entry.
- Update ABI test baselines.
- Update abilist for non-x86-64 architectures.
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
---
Changes since v1:
- Move __rseq_refcount to an extra field at the end of __rseq_abi to
eliminate one symbol.
All libraries/programs which try to register rseq (glibc,
early-adopter applications, early-adopter libraries) should use the
rseq refcount. It becomes part of the ABI within a user-space
process, but it's not part of the ABI shared with the kernel per se.
- Restructure how this code is organized so glibc keeps building on
non-Linux targets.
- Use non-weak symbol for __rseq_abi.
- Move rseq registration/unregistration implementation into its own
nptl/rseq.c compile unit.
- Move __rseq_abi symbol under GLIBC_2.29.
Changes since v2:
- Move __rseq_refcount to its own symbol, which is less ugly than
trying to play tricks with the rseq uapi.
- Move __rseq_abi from nptl to csu (C start up), so it can be used
across glibc, including memory allocator and sched_getcpu(). The
__rseq_refcount symbol is kept in nptl, because there is no reason
to use it elsewhere in glibc.
---
csu/Makefile | 2 +-
csu/Versions | 3 +
csu/rseq.c | 38 ++++++++++
nptl/Makefile | 2 +-
nptl/Versions | 4 ++
nptl/nptl-init.c | 3 +
nptl/pthreadP.h | 3 +
nptl/pthread_create.c | 8 +++
nptl/rseq.c | 42 +++++++++++
sysdeps/nptl/rseq-internal.h | 34 +++++++++
sysdeps/unix/sysv/linux/rseq-internal.h | 72 +++++++++++++++++++
.../unix/sysv/linux/x86_64/64/libc.abilist | 1 +
.../sysv/linux/x86_64/64/libpthread.abilist | 1 +
13 files changed, 211 insertions(+), 2 deletions(-)
create mode 100644 csu/rseq.c
create mode 100644 nptl/rseq.c
create mode 100644 sysdeps/nptl/rseq-internal.h
create mode 100644 sysdeps/unix/sysv/linux/rseq-internal.h
diff --git a/csu/Makefile b/csu/Makefile
index 88fc77662e..81d471587f 100644
--- a/csu/Makefile
+++ b/csu/Makefile
@@ -28,7 +28,7 @@ include ../Makeconfig
routines = init-first libc-start $(libc-init) sysdep version check_fds \
libc-tls elf-init dso_handle
-aux = errno
+aux = errno rseq
elide-routines.os = libc-tls
static-only-routines = elf-init
csu-dummies = $(filter-out $(start-installed-name),crt1.o Mcrt1.o)
diff --git a/csu/Versions b/csu/Versions
index 43010c3443..0f44ebf991 100644
--- a/csu/Versions
+++ b/csu/Versions
@@ -7,6 +7,9 @@ libc {
# New special glibc functions.
gnu_get_libc_release; gnu_get_libc_version;
}
+ GLIBC_2.29 {
+ __rseq_abi;
+ }
GLIBC_PRIVATE {
errno;
}
diff --git a/csu/rseq.c b/csu/rseq.c
new file mode 100644
index 0000000000..17d553324d
--- /dev/null
+++ b/csu/rseq.c
@@ -0,0 +1,38 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdint.h>
+
+enum libc_rseq_cpu_id_state {
+ LIBC_RSEQ_CPU_ID_UNINITIALIZED = -1,
+ LIBC_RSEQ_CPU_ID_REGISTRATION_FAILED = -2,
+};
+
+/* linux/rseq.h defines struct rseq as aligned on 32 bytes. The kernel ABI
+ size is 20 bytes. */
+struct libc_rseq {
+ uint32_t cpu_id_start;
+ uint32_t cpu_id;
+ uint64_t rseq_cs;
+ uint32_t flags;
+} __attribute__((aligned(4 * sizeof(uint64_t))));
+
+__attribute__((weak))
+__thread volatile struct libc_rseq __rseq_abi = {
+ .cpu_id = LIBC_RSEQ_CPU_ID_UNINITIALIZED,
+};
diff --git a/nptl/Makefile b/nptl/Makefile
index be8066524c..9def8b3f13 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -145,7 +145,7 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \
mtx_destroy mtx_init mtx_lock mtx_timedlock \
mtx_trylock mtx_unlock call_once cnd_broadcast \
cnd_destroy cnd_init cnd_signal cnd_timedwait cnd_wait \
- tss_create tss_delete tss_get tss_set
+ tss_create tss_delete tss_get tss_set rseq
# pthread_setuid pthread_seteuid pthread_setreuid \
# pthread_setresuid \
# pthread_setgid pthread_setegid pthread_setregid \
diff --git a/nptl/Versions b/nptl/Versions
index e7f691da7a..f7890f73fc 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -277,6 +277,10 @@ libpthread {
cnd_timedwait; cnd_wait; tss_create; tss_delete; tss_get; tss_set;
}
+ GLIBC_2.29 {
+ __rseq_refcount;
+ }
+
GLIBC_PRIVATE {
__pthread_initialize_minimal;
__pthread_clock_gettime; __pthread_clock_settime;
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 907411d5bc..ab17bbb6e4 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -279,6 +279,9 @@ __pthread_initialize_minimal_internal (void)
THREAD_SETMEM (pd, cpuclock_offset, GL(dl_cpuclock_offset));
#endif
+ /* Register rseq ABI to the kernel. */
+ (void) __rseq_register_current_thread ();
+
/* Initialize the robust mutex data. */
{
#if __PTHREAD_MUTEX_HAVE_PREV
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 13bdb11133..aba641c170 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -605,6 +605,9 @@ extern void __shm_directory_freeres (void) attribute_hidden;
extern void __wait_lookup_done (void) attribute_hidden;
+extern int __rseq_register_current_thread (void) attribute_hidden;
+extern int __rseq_unregister_current_thread (void) attribute_hidden;
+
#ifdef SHARED
# define PTHREAD_STATIC_FN_REQUIRE(name)
#else
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index fe75d04113..a5233cdf2f 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -378,6 +378,7 @@ __free_tcb (struct pthread *pd)
START_THREAD_DEFN
{
struct pthread *pd = START_THREAD_SELF;
+ bool has_rseq = false;
#if HP_TIMING_AVAIL
/* Remember the time when the thread was started. */
@@ -396,6 +397,9 @@ START_THREAD_DEFN
if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
+ /* Register rseq TLS to the kernel. */
+ has_rseq = !__rseq_register_current_thread ();
+
#ifdef __NR_set_robust_list
# ifndef __ASSUME_SET_ROBUST_LIST
if (__set_robust_list_avail >= 0)
@@ -573,6 +577,10 @@ START_THREAD_DEFN
}
#endif
+ /* 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);
diff --git a/nptl/rseq.c b/nptl/rseq.c
new file mode 100644
index 0000000000..415674964f
--- /dev/null
+++ b/nptl/rseq.c
@@ -0,0 +1,42 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include "pthreadP.h"
+
+__attribute__((weak))
+__thread volatile uint32_t __rseq_refcount;
+
+#ifdef __NR_rseq
+#include <sysdeps/unix/sysv/linux/rseq-internal.h>
+#else
+#include <sysdeps/nptl/rseq-internal.h>
+#endif /* __NR_rseq. */
+
+int
+attribute_hidden
+__rseq_register_current_thread (void)
+{
+ return sysdep_rseq_register_current_thread ();
+}
+
+int
+attribute_hidden
+__rseq_unregister_current_thread (void)
+{
+ return sysdep_rseq_register_current_thread ();
+}
diff --git a/sysdeps/nptl/rseq-internal.h b/sysdeps/nptl/rseq-internal.h
new file mode 100644
index 0000000000..96422ebd57
--- /dev/null
+++ b/sysdeps/nptl/rseq-internal.h
@@ -0,0 +1,34 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef RSEQ_INTERNAL_H
+#define RSEQ_INTERNAL_H
+
+static inline int
+sysdep_rseq_register_current_thread (void)
+{
+ return -1;
+}
+
+static inline int
+sysdep_rseq_unregister_current_thread (void)
+{
+ return -1;
+}
+
+#endif /* rseq-internal.h */
diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h
new file mode 100644
index 0000000000..a7d59c8a2a
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/rseq-internal.h
@@ -0,0 +1,72 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef RSEQ_INTERNAL_H
+#define RSEQ_INTERNAL_H
+
+#include <stdint.h>
+#include <linux/rseq.h>
+
+#define RSEQ_SIG 0x53053053
+
+extern __thread volatile struct rseq __rseq_abi
+__attribute__ ((tls_model ("initial-exec")));
+
+extern __thread volatile uint32_t __rseq_refcount
+__attribute__ ((tls_model ("initial-exec")));
+
+static inline int
+sysdep_rseq_register_current_thread (void)
+{
+ int rc, ret = 0;
+ INTERNAL_SYSCALL_DECL (err);
+
+ if (__rseq_abi.cpu_id == RSEQ_CPU_ID_REGISTRATION_FAILED)
+ return -1;
+ if (atomic_increment_val (&__rseq_refcount) - 1)
+ goto end;
+ rc = INTERNAL_SYSCALL_CALL (rseq, err, &__rseq_abi, sizeof (struct rseq),
+ 0, RSEQ_SIG);
+ if (!rc)
+ goto end;
+ if (INTERNAL_SYSCALL_ERRNO (rc, err) != EBUSY)
+ __rseq_abi.cpu_id = RSEQ_CPU_ID_REGISTRATION_FAILED;
+ ret = -1;
+ atomic_decrement (&__rseq_refcount);
+end:
+ return ret;
+}
+
+static inline int
+sysdep_rseq_unregister_current_thread (void)
+{
+ int rc, ret = 0;
+ INTERNAL_SYSCALL_DECL (err);
+
+ if (atomic_decrement_val (&__rseq_refcount))
+ goto end;
+ rc = INTERNAL_SYSCALL_CALL (rseq, err, &__rseq_abi, sizeof (struct rseq),
+ RSEQ_FLAG_UNREGISTER, RSEQ_SIG);
+ if (!rc)
+ goto end;
+ ret = -1;
+end:
+ return ret;
+}
+
+#endif /* rseq-internal.h */
diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
index 816e4a7426..6ef92778fc 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
@@ -1895,6 +1895,7 @@ GLIBC_2.28 thrd_current F
GLIBC_2.28 thrd_equal F
GLIBC_2.28 thrd_sleep F
GLIBC_2.28 thrd_yield F
+GLIBC_2.29 __rseq_abi D
GLIBC_2.3 __ctype_b_loc F
GLIBC_2.3 __ctype_tolower_loc F
GLIBC_2.3 __ctype_toupper_loc F
diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist
index 931c8277a8..2cbb8882eb 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist
@@ -219,6 +219,7 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.29 __rseq_refcount D
GLIBC_2.3.2 pthread_cond_broadcast F
GLIBC_2.3.2 pthread_cond_destroy F
GLIBC_2.3.2 pthread_cond_init F
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-11-02 11:29 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181101203328.GI2180@cisco>
On 11/01, Tycho Andersen wrote:
>
> On Thu, Nov 01, 2018 at 03:48:05PM +0100, Oleg Nesterov wrote:
> >
> > > > But my main concern is that either way wait_for_completion_killable() allows
> > > > to trivially create a process which doesn't react to SIGSTOP, not good...
> > > >
> > > > Note also that this can happen if, say, both the tracer and tracee run in the
> > > > same process group and SIGSTOP is sent to their pgid, if the tracer gets the
> > > > signal first the tracee won't stop.
> > > >
> > > > Of freezer. try_to_freeze_tasks() can fail if it freezes the tracer before
> > > > it does SECCOMP_IOCTL_NOTIF_SEND.
> > >
> > > I think in general the way this is intended to be used these things
> > > wouldn't happen.
> >
> > Why?
>
> The intent is to run the tracer on the host and have it trace
> containers, which would live in a different freezer cgroup, process
> group, etc.
I didn't mean the freezer cgroup, suspend can fail, it does the "global" freeze.
Nevermind.
> > Yes I think it would be nice to avoid wait_for_completion_killable().
> >
> > So please help me to understand the problem. Once again, why can not
> > seccomp_do_user_notification() use wait_for_completion_interruptible() only?
> >
> > This is called before the task actually starts the syscall, so
> > -ERESTARTNOINTR if signal_pending() can't hurt.
>
> The idea was that when the tracee gets a signal, it notifies the
> tracer exactly once, and then waits for the tracer to decide what to
> do. So if we use another wait_for_completion_interruptible(), doesn't
> it just get re-woken immediately because the signal is still pending?
Hmm. I meant that we should use a single wait_for_completion_interruptible().
> > Now lets suppose seccomp_do_user_notification() simply does
> >
> > err = wait_for_completion_interruptible(&n.ready);
> >
> > if (err < 0 && state != SECCOMP_NOTIFY_REPLIED) {
> > syscall_set_return_value(ERESTARTNOINTR);
> > list_del(&n.list);
> > return -1;
> > }
> >
> > (I am ignoring the locking/etc). Now the obvious problem is that the listener
> > doing SECCOMP_IOCTL_NOTIF_SEND can't distinguish -ENOENT from the case when the
> > tracee was killed, yes?
> >
> > Is it that important?
>
> The answer to this question depends on how we want the listener to be
> able to react. For example, if the listener is in the middle of doing
> a mount() on behalf of the task and it gets a signal and we return
> immediately, the listener will complete the mount(), try to respond
> with success and get -ENOENT.
Yes. Should we undo the mount if the tracee is killed?
> If the task handles the signal and
> restarts the mount(), it'll happen twice unless the listener undoes
> it when it sees the -ENOENT.
Yes. But note that we know that if the same tracee sends another notification
it must be the same syscall.
So. If the listener needs to undo mount when the tracee is killed, it should
undo it if it was interrupted too.
If no, the listener can simply "ignore" the next notification and do
SECCOMP_IOCTL_NOTIF_SEND(val = 0, error = 0).
I see no real difference...
> If we send another notification with the
> SIGNALED flag, the listener has a better picture of what's going on,
> which might be nice.
Yes, but this returns us to my my question: Is it that important?
What exactly the listener can do if it gets SECCOMP_NOTIF_FLAG_SIGNALED?
Undo the mount? No. This doesn't differ from the case when the tracee gets
the non-fatal signal right after SECCOMP_NOTIFY_REPLIED.
Oleg.
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Oleg Nesterov @ 2018-11-02 10:02 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181101195635.GG2180@cisco>
On 11/01, Tycho Andersen wrote:
>
> On Thu, Nov 01, 2018 at 02:40:02PM +0100, Oleg Nesterov wrote:
> >
> > Somehow I no longer understand why do you need to take all locks. Isn't
> > the first filter's notify_lock enough? IOW,
> >
> > for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> > if (cur->notif)
> > return ERR_PTR(-EBUSY);
> > first = cur;
> > }
> >
> > if (first)
> > mutex_lock(&first->notify_lock);
> >
> > ... initialize filter->notif ...
> >
> > out:
> > if (first)
> > mutex_unlock(&first->notify_lock);
> >
> > return ret;
>
> The idea here is to prevent people from "nesting" notify filters. So
> if any filter in the chain has a listener attached, it refuses to
> install another filter with a listener.
Yes, I understand, so we need to check cur->notif. My point was, we do not
need to take all the locks in the ->prev chain, we need only one:
first->notify_lock.
But you know what? today I think that we do not need any locking at all,
all we need is the lockless
for (cur = current->seccomp.filter; cur; cur = cur->prev)
if (cur->notif)
return ERR_PTR(-EBUSY);
at the start, nothing more.
> But it just occurred to me that we don't handle the TSYNC case
> correctly by doing it this way,
Why? Perhaps I missed your point, but TSYNC case looks fine. I mean, if 2
threads do seccomp_set_mode_filter(NEW_LISTENER | TSYNC) then only one can
win the race and succeed, but this has nothing to do with init_listener(),
we rely on ->siglock and is_ancestor() check.
No?
Oleg.
^ permalink raw reply
* RE: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: y-goto-LMvhtfratI1BDgjK7y7TUQ @ 2018-11-02 1:43 UTC (permalink / raw)
To: 'Dave Chinner'
Cc: linux-nvdimm, Linux API, linux-xfs, Linux MM, linux-fsdevel,
Jan Kara, linux-ext4
In-Reply-To: <20181101230012.GC19305@dastard>
> > > MAP_DIRECT is an access hint.
> > >
> > > MAP_SYNC provides a data integrity model guarantee.
> > >
> > > MAP_SYNC may imply MAP_DIRECT for specific implementations,
> > > but it does not require or guarantee MAP_DIRECT.
> > >
> > > Let's compare that with O_DIRECT:
> > >
> > > O_DIRECT in an access hint.
> > >
> > > O_DSYNC provides a data integrity model guarantee.
> > >
> > > O_DSYNC may imply O_DIRECT for specific implementations, but
> > > it does not require or guarantee O_DIRECT.
> > >
> > > Consistency in access and data integrity models is a good thing. DAX
> > > and pmem is not an exception. We need to use a model we know works
> > > and has proven itself over a long period of time.
> >
> > Hmmm, then, I would like to know all of the reasons of breakage of MAP_DIRECT.
> > (I'm not opposed to your opinion, but I need to know it.)
> >
> > In O_DIRECT case, in my understanding, the reason of breakage of O_DIRECT is
> > "wrong alignment is specified by application", right?
>
> O_DIRECT has defined memory and offset alignment restrictions, and
> will return an error to userspace when they are violated. It does
> not fall back to buffered IO in this case. MAP_DIRECT has no
> equivalent restriction, so IO alignment of O_DIRECT is largely
> irrelevant here.
>
> What we are talking about here is that some filesystems can only do
> certain operations through buffered IO, such as block allocation or
> file extension, and so silently fall back to doing them via buffered
> IO even when O_DIRECT is specified. The old direct IO code used to
> be full of conditionals to allow this - I think DIO_SKIP_HOLES is
> only one remaining:
>
> /*
> * For writes that could fill holes inside i_size on a
> * DIO_SKIP_HOLES filesystem we forbid block creations: only
> * overwrites are permitted. We will return early to the caller
> * once we see an unmapped buffer head returned, and the caller
> * will fall back to buffered I/O.
> *
> * Otherwise the decision is left to the get_blocks method,
> * which may decide to handle it or also return an unmapped
> * buffer head.
> */
> create = dio->op == REQ_OP_WRITE;
> if (dio->flags & DIO_SKIP_HOLES) {
> if (fs_startblk <= ((i_size_read(dio->inode) - 1) >>
> i_blkbits))
> create = 0;
> }
>
> Other cases like file extension cases are caught by the filesystems
> before calling into the DIO code itself, so there's multiple avenues
> for O_DIRECT transparently falling back to buffered IO.
>
> This means the applications don't fail just because the filesystem
> can't do a specific operation via O_DIRECT. The data writes still
> succeed because they fall back to buffered IO, and the application
> is blissfully unaware that the filesystem behaved that way.
>
> > When filesystem can not use O_DIRECT and it uses page cache instead,
> > then system uses more memory resource than user's expectation.
>
> That's far better than failing unexpectedly because the app
> unexpectedly came across a hole in the file (e.g. someone ran
> sparsify across the filesystem).
>
> > So, there is a side effect, and it may cause other trouble.
> > (memory pressure, expected performance can not be gained, and so on ..)
>
> Which is why people are supposed to test their systems before they
> put them into production.
>
> I've lost count of the number of times I've heard "but O_DIRECT is
> supposed to make things faster!" because people don't understand
> exactly what it does or means. Bypassing the page cache does not
> magically make applications go faster - it puts the responsibility
> for doing optimal IO on the application, not the kernel.
>
> MAP_DIRECT will be no different. It's no guarantee that it will make
> things faster, or that everything will just work as users expect
> them to. It specifically places the responsibility for performing IO
> in an optimal fashion on the application and the user for making
> sure that it is fit for their purposes. Like O_DIRECT, using
> MAP_DIRECT means "I, the application, know exactly what I'm doing,
> so get out of the way as much as possible because I'm taking
> responsibility for issuing IO in the most optimal manner now".
>
> > In such case its administrator (or technical support engineer) needs to struggle to
> > investigate what is the reason.
>
> That's no different to performance problems that arise from
> inappropriate use of O_DIRECT. It requires a certain level of
> expertise to be able to understand and diagnose such issues.
>
> > So, I would like to know in MAP_DIRECT case, what is the reasons?
> > I think it will be helpful for users.
> > Only splice?
>
> The filesystem can ignore MAP_DIRECT for any reason it needs to. I'm
> certain that filesystem developers will try to maintain MAP_DIRECT
> semantics as much as possible, but it's not going to be possible in
> /all situations/ on XFS and ext4 because they simply haven't been
> designed with DAX in mind. Filesystems designed specifically for
> pmem and DAX might be able to provide MAP_DIRECT in all situations,
> but those filesystems don't really exist yet.
>
> This is no different to the early days of O_DIRECT. e.g. ext3
> couldn't do O_DIRECT for all operations when it was first
> introduced, but over time the functionality improved as the
> underlying issues were solved. If O_DIRECT was a guarantee, then
> ext3 would have never supported O_DIRECT at all...
Hmm, Ok. I see.
Thank you very much for your detail explanation.
>
> > (Maybe such document will be necessary....)
>
> The semantics will need to be documented in the relevant man pages.
I agree.
Thanks, again.
----
Yasunori Goto
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Andy Lutomirski @ 2018-11-01 23:22 UTC (permalink / raw)
To: Linus Torvalds
Cc: Rich Felker, Jann Horn, Andrew Lutomirski, Dave Hansen,
Christopherson, Sean J, Jethro Beekman, Jarkko Sakkinen,
Florian Weimer, Linux API, X86 ML, linux-arch, LKML,
Peter Zijlstra, nhorman, npmccallum, Ayoun, Serge,
shay.katz-zamir, linux-sgx, Andy Shevchenko, Thomas Gleixner,
Ingo Molnar, Borislav
In-Reply-To: <CAHk-=wiYSpmDOfpi9n7ETsxK2UrUKfT4kM=Y3yqRSaZuFFPY1A@mail.gmail.com>
On Thu, Nov 1, 2018 at 2:24 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Nov 1, 2018 at 12:31 PM Rich Felker <dalias@libc.org> wrote:
> >
> > See my other emails in this thread. You would register the *address*
> > (in TLS) of a function pointer object pointing to the handler, rather
> > than the function address of the handler. Then switching handler is
> > just a single store in userspace, no syscalls involved.
>
> Yes.
>
> And for just EENTER, maybe that's the right model.
>
> If we want to generalize it to other thread-synchronous faults, it
> needs way more information and a list of handlers, but if we limit the
> thing to _only_ EENTER getting an SGX fault, then a single "this is
> the fault handler" address is probably the right thing to do.
It sounds like you're saying that the kernel should know, *before*
running any user fixup code, whether the fault in question is one that
wants a fixup. Sounds reasonable.
I think it would be nice, but not absolutely necessary, if user code
didn't need to poke some value into TLS each time it ran a function
that had a fixup. With the poke-into-TLS approach, it looks a lot
like rseq, and rseq doesn't nest very nicely. I think we really want
this mechanism to Just Work. So we could maybe have a syscall that
associates a list of fixups with a given range of text addresses. We
might want the kernel to automatically zap the fixups when the text in
question is unmapped.
^ permalink raw reply
* Re: Problems with VM_MIXEDMAP removal from /proc/<pid>/smaps
From: Dave Chinner @ 2018-11-01 23:00 UTC (permalink / raw)
To: y-goto-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org
Cc: linux-nvdimm, Linux API, linux-xfs, Linux MM, linux-fsdevel,
Jan Kara, linux-ext4
In-Reply-To: <TYAPR01MB32619CCA488DD0DA86EDB17E90CD0-0qPFe2CvyVSThio5wsivRXcolHNk5qUtvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
On Wed, Oct 31, 2018 at 05:59:17AM +0000, y-goto-LMvhtfratI1BDgjK7y7TUQ@public.gmane.org wrote:
> > On Mon, Oct 29, 2018 at 11:30:41PM -0700, Dan Williams wrote:
> > > On Thu, Oct 18, 2018 at 5:58 PM Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote:
> > In summary:
> >
> > MAP_DIRECT is an access hint.
> >
> > MAP_SYNC provides a data integrity model guarantee.
> >
> > MAP_SYNC may imply MAP_DIRECT for specific implementations,
> > but it does not require or guarantee MAP_DIRECT.
> >
> > Let's compare that with O_DIRECT:
> >
> > O_DIRECT in an access hint.
> >
> > O_DSYNC provides a data integrity model guarantee.
> >
> > O_DSYNC may imply O_DIRECT for specific implementations, but
> > it does not require or guarantee O_DIRECT.
> >
> > Consistency in access and data integrity models is a good thing. DAX
> > and pmem is not an exception. We need to use a model we know works
> > and has proven itself over a long period of time.
>
> Hmmm, then, I would like to know all of the reasons of breakage of MAP_DIRECT.
> (I'm not opposed to your opinion, but I need to know it.)
>
> In O_DIRECT case, in my understanding, the reason of breakage of O_DIRECT is
> "wrong alignment is specified by application", right?
O_DIRECT has defined memory and offset alignment restrictions, and
will return an error to userspace when they are violated. It does
not fall back to buffered IO in this case. MAP_DIRECT has no
equivalent restriction, so IO alignment of O_DIRECT is largely
irrelevant here.
What we are talking about here is that some filesystems can only do
certain operations through buffered IO, such as block allocation or
file extension, and so silently fall back to doing them via buffered
IO even when O_DIRECT is specified. The old direct IO code used to
be full of conditionals to allow this - I think DIO_SKIP_HOLES is
only one remaining:
/*
* For writes that could fill holes inside i_size on a
* DIO_SKIP_HOLES filesystem we forbid block creations: only
* overwrites are permitted. We will return early to the caller
* once we see an unmapped buffer head returned, and the caller
* will fall back to buffered I/O.
*
* Otherwise the decision is left to the get_blocks method,
* which may decide to handle it or also return an unmapped
* buffer head.
*/
create = dio->op == REQ_OP_WRITE;
if (dio->flags & DIO_SKIP_HOLES) {
if (fs_startblk <= ((i_size_read(dio->inode) - 1) >>
i_blkbits))
create = 0;
}
Other cases like file extension cases are caught by the filesystems
before calling into the DIO code itself, so there's multiple avenues
for O_DIRECT transparently falling back to buffered IO.
This means the applications don't fail just because the filesystem
can't do a specific operation via O_DIRECT. The data writes still
succeed because they fall back to buffered IO, and the application
is blissfully unaware that the filesystem behaved that way.
> When filesystem can not use O_DIRECT and it uses page cache instead,
> then system uses more memory resource than user's expectation.
That's far better than failing unexpectedly because the app
unexpectedly came across a hole in the file (e.g. someone ran
sparsify across the filesystem).
> So, there is a side effect, and it may cause other trouble.
> (memory pressure, expected performance can not be gained, and so on ..)
Which is why people are supposed to test their systems before they
put them into production.
I've lost count of the number of times I've heard "but O_DIRECT is
supposed to make things faster!" because people don't understand
exactly what it does or means. Bypassing the page cache does not
magically make applications go faster - it puts the responsibility
for doing optimal IO on the application, not the kernel.
MAP_DIRECT will be no different. It's no guarantee that it will make
things faster, or that everything will just work as users expect
them to. It specifically places the responsibility for performing IO
in an optimal fashion on the application and the user for making
sure that it is fit for their purposes. Like O_DIRECT, using
MAP_DIRECT means "I, the application, know exactly what I'm doing,
so get out of the way as much as possible because I'm taking
responsibility for issuing IO in the most optimal manner now".
> In such case its administrator (or technical support engineer) needs to struggle to
> investigate what is the reason.
That's no different to performance problems that arise from
inappropriate use of O_DIRECT. It requires a certain level of
expertise to be able to understand and diagnose such issues.
> So, I would like to know in MAP_DIRECT case, what is the reasons?
> I think it will be helpful for users.
> Only splice?
The filesystem can ignore MAP_DIRECT for any reason it needs to. I'm
certain that filesystem developers will try to maintain MAP_DIRECT
semantics as much as possible, but it's not going to be possible in
/all situations/ on XFS and ext4 because they simply haven't been
designed with DAX in mind. Filesystems designed specifically for
pmem and DAX might be able to provide MAP_DIRECT in all situations,
but those filesystems don't really exist yet.
This is no different to the early days of O_DIRECT. e.g. ext3
couldn't do O_DIRECT for all operations when it was first
introduced, but over time the functionality improved as the
underlying issues were solved. If O_DIRECT was a guarantee, then
ext3 would have never supported O_DIRECT at all...
> (Maybe such document will be necessary....)
The semantics will need to be documented in the relevant man pages.
Cheers,
Dave.
--
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
^ permalink raw reply
* Re: [RFC PATCH for 4.21 03/16] mm: Replace BUG_ON() by WARN_ON() in vm_unmap_ram()
From: Mathieu Desnoyers @ 2018-11-01 22:17 UTC (permalink / raw)
To: Linus Torvalds
Cc: rostedt, Thomas Gleixner, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, linux-kernel, linux-api, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Josh Triplett, Catalin Marinas, Will Deacon,
Michael Kerrisk <mtk.ma>
In-Reply-To: <CAHk-=wiOm8x5KnBQH+FYN76sSjCeP2_9cxgzu4WArR3jP0MD8g@mail.gmail.com>
----- On Nov 1, 2018, at 11:00 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Thu, Nov 1, 2018 at 12:57 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> > I think the graceful recovery is to simply return:
>> >
>> > if (WARN_ON(cond))
>> > return;
>> >
>> > is better than just
>> >
>> > BUG_ON(cond);
>> >
>> > As that's what Linus made pretty clear at the Maintainer's Summit.
>>
>> That's it. For an unmap function, this basically boils down to
>> print a warning and leak the memory on internal unmap error.
>>
>> I will update the commit message describing this behavior.
>
> It might be even better to use WARN_ON_ONCE().
>
> If it's a "this shouldn't happen" situation, the advantage of
> WARN_ON_ONCE() is that it will still show the backtrace of the "how
> the heck did it happen after all" situation, but if it turns ouit to
> be user-triggerable (or simply triggerable by some odd hw situation),
> it won't spam your logs forever.
>
> Obviously, things like rate limiting etc can also be good ideas, but
> that's just overkill for "this really should never happen" cases.
>
> (Side note: WARN_ON_ONCE() will _warn_ just once, but will always
> return the condition that it warns for, so the return value is _not_
> "I have warned", but "I have seen the condition that I should warn
> about". Just in case people are worried about it).
Allright, I'll update this patch (and the following one implementing
vm_{map,unmap}_user_ram) to use WARN_ON_ONCE().
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Richard Guy Briggs @ 2018-11-01 22:07 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-kernel, ebiederm, luto,
dhowells, viro, simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhQFqs1SyvximK+8XJG5Fk3p9WsWhEV5sY6kksN9tc6eKw@mail.gmail.com>
On 2018-10-19 19:15, Paul Moore wrote:
> On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > The audit-related parameters in struct task_struct should ideally be
> > collected together and accessed through a standard audit API.
> >
> > Collect the existing loginuid, sessionid and audit_context together in a
> > new struct audit_task_info called "audit" in struct task_struct.
> >
> > Use kmem_cache to manage this pool of memory.
> > Un-inline audit_free() to be able to always recover that memory.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/81
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > include/linux/sched.h | 5 +----
> > init/init_task.c | 3 +--
> > init/main.c | 2 ++
> > kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
> > kernel/fork.c | 4 +++-
> > 6 files changed, 73 insertions(+), 26 deletions(-)
>
> ...
>
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 9334fbe..8964332 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab,
> >
> > /* These are defined in auditsc.c */
> > /* Public API */
> > +struct audit_task_info {
> > + kuid_t loginuid;
> > + unsigned int sessionid;
> > + struct audit_context *ctx;
> > +};
>
> ...
>
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 87bf02d..e117272 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -873,10 +872,8 @@ struct task_struct {
> >
> > struct callback_head *task_works;
> >
> > - struct audit_context *audit_context;
> > #ifdef CONFIG_AUDITSYSCALL
> > - kuid_t loginuid;
> > - unsigned int sessionid;
> > + struct audit_task_info *audit;
> > #endif
> > struct seccomp seccomp;
>
> Prior to this patch audit_context was available regardless of
> CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
> is only available when CONFIG_AUDITSYSCALL is defined.
This was intentional since audit_context is not used when AUDITSYSCALL is
disabled. audit_alloc() was stubbed in that case to return 0. audit_context()
returned NULL.
The fact that audit_context was still present in struct task_struct was an
oversight in the two patches already accepted:
("audit: use inline function to get audit context")
("audit: use inline function to get audit context")
that failed to hide or remove it from struct task_struct when it was no longer
relevant.
The 0-day kbuildbot was happy and it tests many configs.
On further digging, loginuid and sessionid (and audit_log_session_info) should
be part of CONFIG_AUDIT scope and not CONFIG_AUDITSYSCALL since it is used in
CONFIG_CHANGE, ANOM_LINK, FEATURE_CHANGE(, INTEGRITY_RULE), none of which are
otherwise dependent on AUDITSYSCALL.
Looking ahead, contid should be treated like loginuid and sessionid, which are
currently only available when syscall auditting is.
Converting records from standalone to syscall and checking audit_dummy_context
changes the nature of CONFIG_AUDIT/!CONFIG_AUDITSYSCALL separation.
eg: ANOM_LINK accompanied by PATH record (which needed CWD addition to be
complete anyways)
> > diff --git a/init/main.c b/init/main.c
> > index 3b4ada1..6aba171 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -92,6 +92,7 @@
> > #include <linux/rodata_test.h>
> > #include <linux/jump_label.h>
> > #include <linux/mem_encrypt.h>
> > +#include <linux/audit.h>
> >
> > #include <asm/io.h>
> > #include <asm/bugs.h>
> > @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void)
> > nsfs_init();
> > cpuset_init();
> > cgroup_init();
> > + audit_task_init();
> > taskstats_init_early();
> > delayacct_init();
>
> It seems like we would need either init_struct_audit or
> audit_task_init(), but not both, yes?
One sets initial values of init task via an included struct, other makes a call
to create the kmem cache. Both seem appropriate to me unless we move the
initialization from a struct to assignments in audit_task_init(), but I'm not
that comfortable separating the audit init values from the rest of the
task_struct init task initializers (though there are other subsystems that need
to do so dynamically).
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index fb20746..88779a7 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
> > int return_valid,
> > long return_code)
> > {
> > - struct audit_context *context = tsk->audit_context;
> > + struct audit_context *context = tsk->audit->ctx;
> >
> > if (!context)
> > return NULL;
> > @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
> > return context;
> > }
> >
> > +static struct kmem_cache *audit_task_cache;
> > +
> > +void __init audit_task_init(void)
> > +{
> > + audit_task_cache = kmem_cache_create("audit_task",
> > + sizeof(struct audit_task_info),
> > + 0, SLAB_PANIC, NULL);
> > +}
>
> This is somewhat related to the CONFIG_AUDITSYSCALL comment above, but
> since the audit_task_info contains generic audit state (not just
> syscall related state), it seems like this, and the audit_task_info
> accessors/helpers, should live in kernel/audit.c.
Well, in fact it was only containing syscall related state.
> There are probably a few other things that should move to
> kernel/audit.c too, e.g. audit_alloc(). Have you verified that this
> builds/runs correctly on architectures that define CONFIG_AUDIT but
> not CONFIG_AUDITSYSCALL?
I was under the mistaken impression that all this went away and wondered why
not just rip out the AUDITSYSCALL config option, but that was not completely
solved by cb74ed278f80 ("audit: always enable syscall auditing when supported
and audit is enabled").
I vaguely knew that AUDITSYSCALL was not implemented on all platforms but that
a number were expunged recently from mainline. It turns out that 5-10+ remain.
> > /**
> > * audit_alloc - allocate an audit context block for a task
> > * @tsk: task
> > @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk)
> > struct audit_context *context;
> > enum audit_state state;
> > char *key = NULL;
> > + struct audit_task_info *info;
> > +
> > + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > + info->loginuid = audit_get_loginuid(current);
> > + info->sessionid = audit_get_sessionid(current);
> > + tsk->audit = info;
> >
> > if (likely(!audit_ever_enabled))
> > return 0; /* Return if not auditing. */
>
> I don't view this as necessary for initial acceptance, and
> synchronization/locking might render this undesirable, but it would be
> curious to see if we could do something clever with refcnts and
> copy-on-write to minimize the number of kmem_cache objects in use in
> the !audit_ever_enabled (and possibly the AUDIT_DISABLED) case.
>
> > state = audit_filter_task(tsk, &key);
> > if (state == AUDIT_DISABLED) {
> > + audit_set_context(tsk, NULL);
>
> It's already NULL, isn't it?
Yes, holdover from copying audit_task_info as a struct from the parent task.
Fixed.
> > clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> > return 0;
> > }
> >
> > if (!(context = audit_alloc_context(state))) {
> > + tsk->audit = NULL;
> > + kmem_cache_free(audit_task_cache, info);
> > kfree(key);
> > audit_log_lost("out of memory in audit_alloc");
> > return -ENOMEM;
> > @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk)
> > return 0;
> > }
> >
> > +struct audit_task_info init_struct_audit = {
> > + .loginuid = INVALID_UID,
> > + .sessionid = AUDIT_SID_UNSET,
> > + .ctx = NULL,
> > +};
> > +
> > static inline void audit_free_context(struct audit_context *context)
> > {
> > audit_free_names(context);
>
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [RFC PATCH for 4.21 03/16] mm: Replace BUG_ON() by WARN_ON() in vm_unmap_ram()
From: Linus Torvalds @ 2018-11-01 22:00 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: rostedt, tglx, Peter Zijlstra, Paul McKenney, boqun.feng,
Linux Kernel Mailing List, linux-api, luto, davejwatson,
Paul Turner, Andrew Morton, linux, Ingo Molnar, Peter Anvin, andi,
Christoph Lameter, bmaurer, josh, Catalin Marinas, will.deacon,
Michael Kerrisk-manpages, Joel Fernandes, Sergey Senozhatsky,
mawilcox
In-Reply-To: <278477314.3995.1541102273591.JavaMail.zimbra@efficios.com>
On Thu, Nov 1, 2018 at 12:57 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> > I think the graceful recovery is to simply return:
> >
> > if (WARN_ON(cond))
> > return;
> >
> > is better than just
> >
> > BUG_ON(cond);
> >
> > As that's what Linus made pretty clear at the Maintainer's Summit.
>
> That's it. For an unmap function, this basically boils down to
> print a warning and leak the memory on internal unmap error.
>
> I will update the commit message describing this behavior.
It might be even better to use WARN_ON_ONCE().
If it's a "this shouldn't happen" situation, the advantage of
WARN_ON_ONCE() is that it will still show the backtrace of the "how
the heck did it happen after all" situation, but if it turns ouit to
be user-triggerable (or simply triggerable by some odd hw situation),
it won't spam your logs forever.
Obviously, things like rate limiting etc can also be good ideas, but
that's just overkill for "this really should never happen" cases.
(Side note: WARN_ON_ONCE() will _warn_ just once, but will always
return the condition that it warns for, so the return value is _not_
"I have warned", but "I have seen the condition that I should warn
about". Just in case people are worried about it).
Linus
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Linus Torvalds @ 2018-11-01 21:24 UTC (permalink / raw)
To: dalias
Cc: Jann Horn, luto, dave.hansen, sean.j.christopherson, jethro,
jarkko.sakkinen, fweimer, linux-api, x86, linux-arch,
Linux Kernel Mailing List, Peter Zijlstra, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, linux-sgx, andriy.shevchenko, tglx,
Ingo Molnar, bp, carlos, adhemerval.zanella
In-Reply-To: <20181101193107.GE5150@brightrain.aerifal.cx>
On Thu, Nov 1, 2018 at 12:31 PM Rich Felker <dalias@libc.org> wrote:
>
> See my other emails in this thread. You would register the *address*
> (in TLS) of a function pointer object pointing to the handler, rather
> than the function address of the handler. Then switching handler is
> just a single store in userspace, no syscalls involved.
Yes.
And for just EENTER, maybe that's the right model.
If we want to generalize it to other thread-synchronous faults, it
needs way more information and a list of handlers, but if we limit the
thing to _only_ EENTER getting an SGX fault, then a single "this is
the fault handler" address is probably the right thing to do.
Linus
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-11-01 20:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181101144804.GD23232@redhat.com>
On Thu, Nov 01, 2018 at 03:48:05PM +0100, Oleg Nesterov wrote:
> On 10/30, Tycho Andersen wrote:
> >
> > > I am not sure I understand the value of signaled/SECCOMP_NOTIF_FLAG_SIGNALED...
> > > I mean, why it is actually useful?
> > >
> > > Sorry if this was already discussed.
> >
> > :) no problem, many people have complained about this. This is an
> > implementation of Andy's suggestion here:
> > https://lkml.org/lkml/2018/3/15/1122
> >
> > You can see some more detailed discussion here:
> > https://lkml.org/lkml/2018/9/21/138
>
> Cough, sorry, I simply can't understand what are you talking about ;)
> It seems that I need to read all the previous emails... So let me ask
> a stupid question below.
>
> > > But my main concern is that either way wait_for_completion_killable() allows
> > > to trivially create a process which doesn't react to SIGSTOP, not good...
> > >
> > > Note also that this can happen if, say, both the tracer and tracee run in the
> > > same process group and SIGSTOP is sent to their pgid, if the tracer gets the
> > > signal first the tracee won't stop.
> > >
> > > Of freezer. try_to_freeze_tasks() can fail if it freezes the tracer before
> > > it does SECCOMP_IOCTL_NOTIF_SEND.
> >
> > I think in general the way this is intended to be used these things
> > wouldn't happen.
>
> Why?
The intent is to run the tracer on the host and have it trace
containers, which would live in a different freezer cgroup, process
group, etc. Of course you could use it in a situation where they would
be, so the concern is still valid, but I'm not sure why you'd do that.
> > was malicious and had the ability to create a user namespace to
> > exhaust pids this way,
>
> Not sure I understand how this connects to my question... nevermind.
>
> > so perhaps we should drop this part of the
> > patch. I have no real need for it, but perhaps Andy can elaborate?
>
> Yes I think it would be nice to avoid wait_for_completion_killable().
>
> So please help me to understand the problem. Once again, why can not
> seccomp_do_user_notification() use wait_for_completion_interruptible() only?
>
> This is called before the task actually starts the syscall, so
> -ERESTARTNOINTR if signal_pending() can't hurt.
The idea was that when the tracee gets a signal, it notifies the
tracer exactly once, and then waits for the tracer to decide what to
do. So if we use another wait_for_completion_interruptible(), doesn't
it just get re-woken immediately because the signal is still pending?
...actually I just tested it, and it doesn't. So it seems we could use
_interruptible() here and achieve the same thing.
> Now lets suppose seccomp_do_user_notification() simply does
>
> err = wait_for_completion_interruptible(&n.ready);
>
> if (err < 0 && state != SECCOMP_NOTIFY_REPLIED) {
> syscall_set_return_value(ERESTARTNOINTR);
> list_del(&n.list);
> return -1;
> }
>
> (I am ignoring the locking/etc). Now the obvious problem is that the listener
> doing SECCOMP_IOCTL_NOTIF_SEND can't distinguish -ENOENT from the case when the
> tracee was killed, yes?
>
> Is it that important?
The answer to this question depends on how we want the listener to be
able to react. For example, if the listener is in the middle of doing
a mount() on behalf of the task and it gets a signal and we return
immediately, the listener will complete the mount(), try to respond
with success and get -ENOENT. If the task handles the signal and
restarts the mount(), it'll happen twice unless the listener undoes
it when it sees the -ENOENT. If we send another notification with the
SIGNALED flag, the listener has a better picture of what's going on,
which might be nice.
Tycho
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-11-01 19:58 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181101135633.GB23232@redhat.com>
On Thu, Nov 01, 2018 at 02:56:34PM +0100, Oleg Nesterov wrote:
> On 10/29, Tycho Andersen wrote:
> >
> > +static int seccomp_notify_release(struct inode *inode, struct file *file)
> > +{
> > + struct seccomp_filter *filter = file->private_data;
> > + struct seccomp_knotif *knotif;
> > +
> > + mutex_lock(&filter->notify_lock);
> > +
> > + /*
> > + * If this file is being closed because e.g. the task who owned it
> > + * died, let's wake everyone up who was waiting on us.
> > + */
> > + list_for_each_entry(knotif, &filter->notif->notifications, list) {
> > + if (knotif->state == SECCOMP_NOTIFY_REPLIED)
> > + continue;
> > +
> > + knotif->state = SECCOMP_NOTIFY_REPLIED;
> > + knotif->error = -ENOSYS;
> > + knotif->val = 0;
> > +
> > + complete(&knotif->ready);
> > + }
> > +
> > + wake_up_all(&filter->notif->wqh);
>
> Why? __fput() is not possible if there is another user of this file sleeping
> in seccomp_notify_poll().
Yes, I was just trying to be extra defensive. But I can drop it.
> > + kfree(filter->notif);
>
> Hmm, this looks wrong... we can't kfree ->notif if its ->notifications list
> is not empty, otherwise seccomp_do_user_notification()->list_del(&n.list)
> can write to the freed memory.
>
> I think _release() should do list_for_each_entry_safe() + list_del_init()
> and seccomp_do_user_notification() should use list_del_init() too.
>
> Or, simpler, seccomp_do_user_notification() should do
>
> if (!match->notif)
> goto out;
>
> instead of "goto remove_list".
Yes, and we need another such check in this case after we re-acquire
the lock from the signal send. Thanks for catching this!
Tycho
^ permalink raw reply
* Re: [RFC PATCH for 4.21 03/16] mm: Replace BUG_ON() by WARN_ON() in vm_unmap_ram()
From: Mathieu Desnoyers @ 2018-11-01 19:57 UTC (permalink / raw)
To: rostedt
Cc: Thomas Gleixner, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
linux-kernel, linux-api, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Josh Triplett, Linus Torvalds, Catalin Marinas, Will Deacon,
Michae
In-Reply-To: <20181101144623.61d43102@gandalf.local.home>
----- On Nov 1, 2018, at 7:46 PM, rostedt rostedt@goodmis.org wrote:
> On Thu, 1 Nov 2018 13:21:12 +0100 (CET)
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> Mathieu,
>>
>> On Thu, 1 Nov 2018, Mathieu Desnoyers wrote:
>>
>> > It is encouraged to warn and return rather than use BUG_ON() when
>> > the condition can be recovered from in ways that are more graceful than
>> > halting the whole system.
>>
>> You're failing to desribe how that graceful recovery works.
>>
>
> I think the graceful recovery is to simply return:
>
> if (WARN_ON(cond))
> return;
>
> is better than just
>
> BUG_ON(cond);
>
> As that's what Linus made pretty clear at the Maintainer's Summit.
That's it. For an unmap function, this basically boils down to
print a warning and leak the memory on internal unmap error.
I will update the commit message describing this behavior.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-11-01 19:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Andy Lutomirski, Eric W . Biederman, Serge E . Hallyn,
Christian Brauner, Tyler Hicks, Akihiro Suda, Aleksa Sarai,
linux-kernel, containers, linux-api
In-Reply-To: <20181101134001.GA23232@redhat.com>
On Thu, Nov 01, 2018 at 02:40:02PM +0100, Oleg Nesterov wrote:
> On 10/29, Tycho Andersen wrote:
> >
> > +static struct file *init_listener(struct seccomp_filter *filter)
> > +{
> > + struct file *ret = ERR_PTR(-EBUSY);
> > + struct seccomp_filter *cur, *last_locked = NULL;
> > + int filter_nesting = 0;
> > +
> > + for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> > + mutex_lock_nested(&cur->notify_lock, filter_nesting);
> > + filter_nesting++;
> > + last_locked = cur;
> > + if (cur->notif)
> > + goto out;
> > + }
>
> Somehow I no longer understand why do you need to take all locks. Isn't
> the first filter's notify_lock enough? IOW,
>
> for (cur = current->seccomp.filter; cur; cur = cur->prev) {
> if (cur->notif)
> return ERR_PTR(-EBUSY);
> first = cur;
> }
>
> if (first)
> mutex_lock(&first->notify_lock);
>
> ... initialize filter->notif ...
>
> out:
> if (first)
> mutex_unlock(&first->notify_lock);
>
> return ret;
The idea here is to prevent people from "nesting" notify filters. So
if any filter in the chain has a listener attached, it refuses to
install another filter with a listener.
But it just occurred to me that we don't handle the TSYNC case
correctly by doing it this way, and it's not necessarily obvious to me
how we can :). So let me look into that.
Tycho
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Rich Felker @ 2018-11-01 19:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, luto, dave.hansen, sean.j.christopherson, jethro,
jarkko.sakkinen, fweimer, linux-api, x86, linux-arch,
Linux Kernel Mailing List, Peter Zijlstra, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, linux-sgx, andriy.shevchenko, tglx,
Ingo Molnar, bp, carlos, adhemerval.zanella
In-Reply-To: <CAHk-=wgFo8tuZX5gzCfWGdAwGyn_Ar9JFnnMFNuwr9=vtfDV1g@mail.gmail.com>
On Thu, Nov 01, 2018 at 12:10:35PM -0700, Linus Torvalds wrote:
> On Thu, Nov 1, 2018 at 11:52 AM Rich Felker <dalias@libc.org> wrote:
> >
> > There's no need to chain if the handler is specific to the context
> > where the fault happens. You just replace the handler with the one
> > relevant to the code you're about to run before you run it.
>
> That's much too expensive to do as a system call.
See my other emails in this thread. You would register the *address*
(in TLS) of a function pointer object pointing to the handler, rather
than the function address of the handler. Then switching handler is
just a single store in userspace, no syscalls involved.
Rich
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Linus Torvalds @ 2018-11-01 19:10 UTC (permalink / raw)
To: dalias
Cc: Jann Horn, luto, dave.hansen, sean.j.christopherson, jethro,
jarkko.sakkinen, fweimer, linux-api, x86, linux-arch,
Linux Kernel Mailing List, Peter Zijlstra, nhorman, npmccallum,
serge.ayoun, shay.katz-zamir, linux-sgx, andriy.shevchenko, tglx,
Ingo Molnar, bp, carlos, adhemerval.zanella
In-Reply-To: <20181101185225.GC5150@brightrain.aerifal.cx>
On Thu, Nov 1, 2018 at 11:52 AM Rich Felker <dalias@libc.org> wrote:
>
> There's no need to chain if the handler is specific to the context
> where the fault happens. You just replace the handler with the one
> relevant to the code you're about to run before you run it.
That's much too expensive to do as a system call.
Maybe an rseq-like "register an area where exception information will
be found" and then you can just swap in a pointer there (and nest with
previous pointers).
But even that doesn't work. Maybe some library wants to capture page
faults because they write-protected some area and want to log writes
and then emulate them (or just enable them after logging - statistical
logging is a thing).
And then another library (or just nested code) wants to handle the
eenter fault, so it overwrites the page handler fault. What do you do
if you now get a page fault before you even do the eenter?
The whole "one global error handler" model is broken. It's broken even
if the "global" one is just per-thread. Don't do it.
Even signals didn't make *that* bad a mistake, and signals are horrible.
Linus
^ permalink raw reply
* Re: RFC: userspace exception fixups
From: Linus Torvalds @ 2018-11-01 19:06 UTC (permalink / raw)
To: luto
Cc: dave.hansen, sean.j.christopherson, jethro, jarkko.sakkinen,
fweimer, linux-api, Jann Horn, x86, linux-arch,
Linux Kernel Mailing List, Peter Zijlstra, dalias, nhorman,
npmccallum, serge.ayoun, shay.katz-zamir, linux-sgx,
andriy.shevchenko, tglx, Ingo Molnar, bp
In-Reply-To: <CALCETrWdpoDkbZjkucKL91GWpDPG9p=VqYrULade2pFDR7S=GQ@mail.gmail.com>
On Thu, Nov 1, 2018 at 10:53 AM Andy Lutomirski <luto@kernel.org> wrote:
>
> There's been some discussion of adding a vDSO entry point to wrap
> EENTER and do something sensible with the exceptions,
I think that's likely the right thing to do, and would be similar to sysenter.
> The basic idea would be to allow libc, or maybe even any library, to
> register a handler that gets a chance to act on an exception caused by
> a user instruction before a signal is delivered. As a straw-man
> example for how this could work, there could be a new syscall:
>
> long register_exception_handler(void (*handler)(int, siginfo_t *, void *));
I'm not a huge fan of signals, but the above is an abomination.
It has all the problems of signals _and_ then some.
And it in absolutely no way fixes the problem with libraires. In fact,
it arguably makes it much much worse, since now there's only one
single library that can register it.
Yes yes, maybe a library would then expose _another_ interface to
other libraries and act as some kind of dispatch point, but on the
whole the above is just crazy and fundamentally broken.
If you want to register an exception, you need to make it clear
(a) which _thread_ the exception registration is valid for
(b) which _range_ the exception registration is valid for
(c) which _fault_ the exception registration is valid for (page
fault, div-by-zero, whatever)
(d) which save area (aka stack) and exception handler point.
Note that (b) might be more than just an exception IP range. It might
well be interesting to register the exception by page fault address
(in addition to code range).
If you do something that does all of (a)-(d), and you allow some
limited number of exception registrations, then maybe. Because at that
point, you have something that is actually actively more powerful than
signal handling is.
But your suggested "just register a broken form of signal handling for
a special case" is just wrong. Don't do it.
Linus
^ 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