* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Rich Felker @ 2018-11-23 14:28 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: <871s7cvt1l.fsf@oldenburg.str.redhat.com>
On Fri, Nov 23, 2018 at 02:10:14PM +0100, Florian Weimer wrote:
> * Rich Felker:
>
> >> I'm not entirely sure because the glibc terminology is confusing, but I
> >> think it places intial-exec TLS into the static TLS area (so that it has
> >> a fixed offset from the TCB). The static TLS area is placed on the
> >> user-supplied stack.
> >
> > This is an implementation detail that should not leak to applications,
> > and I believe it's still considered a bug, in that, with large static
> > TLS, it could overflow or leave unusably little space left on an
> > otherwise-plenty-large application-provided stack.
>
> Sure, but that does not matter in this context because right now, there
> is no fix for this bug, and when we fix it, we can take backwards
> compatibility into account.
>
> Any library that ends up using rseq will need to coordinate with the
> toolchain. I think that's unavoidable given the kernel interface.
Right. I don't agree with this. What I'm saying is that this behavior
(putting static TLS in the caller-provided stack) should not be
documented as a behavior applications can rely on or accounted as a
solution to the rseq problem, since doing so would preclude fixing the
"application doesn't have as much stack as it requested" bug.
> >> > One issue here is that early adopter libraries cannot always use
> >> > the IE model. I tried using it for other TLS variables in lttng-ust, and
> >> > it ended up hanging our CI tests when tracing a sample application with
> >> > lttng-ust under a Java virtual machine: being dlopen'd in a process that
> >> > possibly already exhausts the number of available backup TLS IE entries
> >> > seems to have odd effects. This is why I'm worried about using the IE model
> >> > within lttng-ust.
> >>
> >> You can work around this by preloading the library. I'm not sure if
> >> this is a compelling reason not to use initial-exec TLS memory.
> >
> > Use of IE model from a .so file (except possibly libc.so or something
> > else that inherently needs to be present at program startup for other
> > reasons) should be a considered a bug and unsupported usage.
> > Encouraging libraries to perpetuate this behavior is going backwards
> > on progress that's being made to end it.
>
> Why? Just because glibc's TCB allocation strategy is problematic?
> We can fix that, even with dlopen.
>
> If you are only concerned about the interactions with dlopen, then why
> do you think initial-exec TLS is the problem, and not dlopen?
The initial-exec model, *by design*, only works for TLS objects that
exist at initial execution time. That's why it's called initial-exec.
This is not an implementation flaw/limitation in glibc but
fundamental to the fact that you don't have an unlimited-size (or
practically unlimited) virtual address space range for each thread.
The global-dynamic model is the one that admits dynamic creation of
new TLS objects at runtime (thus the name).
> >> > The per-thread reference counter is a way to avoid issues that arise from
> >> > lack of destructor ordering. Is it an acceptable approach for you, or
> >> > you have something else in mind ?
> >>
> >> Only for the involved libraries. It will not help if other TLS
> >> destructors run and use these libraries.
> >
> > Presumably they should have registered their need for rseq too,
> > thereby incrementing the reference count. I'm not sure this is a good
> > idea, but I think I understand it now.
>
> They may have to increase the reference count from 0 to 1, though, so
> they have to re-register the rseq area. This tends to get rather messy.
>
> I still I think implicit destruction of the rseq area is preferable over
> this complexity.
Absolutely. As long as it's in libc, implicit destruction will happen.
Actually I think the glibc code shound unconditionally unregister the
rseq address at exit (after blocking signals, so no application code
can run) in case a third-party rseq library was linked and failed to
do so before thread exit (e.g. due to mismatched ref counts) rather
than respecting the reference count, since it knows it's the last
user. This would make potentially-buggy code safer.
Rich
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: David Newall @ 2018-11-23 14:11 UTC (permalink / raw)
To: Florian Weimer, Daniel Colascione
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <87efbbvrx9.fsf@oldenburg.str.redhat.com>
On 24/11/18 12:04 am, Florian Weimer wrote:
> But socketcall does not exist on all architectures. Neither does
> getpid, it's called getxpid on some architectures.
> ...
> I think it would be a poor approach to expose application developers to
> these portability issues. We need to abstract over these differences at
> a certain layer, and applications are too late.
Interesting. I think the opposite. I think exposing the OS's
interfaces is exactly what a c-library should do. It might also provide
alternative interfaces that work consistently across different
platforms, but in addition to, not instead of the OS interface.
^ permalink raw reply
* Re: [RFC PATCH 1/3] mm, proc: be more verbose about unstable VMA flags in /proc/<pid>/smaps
From: Vlastimil Babka @ 2018-11-23 13:47 UTC (permalink / raw)
To: Michal Hocko, linux-api
Cc: Andrew Morton, Alexey Dobriyan, linux-mm, LKML, Michal Hocko,
Jan Kara, Dan Williams, David Rientjes
In-Reply-To: <20181120103515.25280-2-mhocko@kernel.org>
On 11/20/18 11:35 AM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Even though vma flags exported via /proc/<pid>/smaps are explicitly
> documented to be not guaranteed for future compatibility the warning
> doesn't go far enough because it doesn't mention semantic changes to
> those flags. And they are important as well because these flags are
> a deep implementation internal to the MM code and the semantic might
> change at any time.
>
> Let's consider two recent examples:
> http://lkml.kernel.org/r/20181002100531.GC4135@quack2.suse.cz
> : commit e1fb4a086495 "dax: remove VM_MIXEDMAP for fsdax and device dax" has
> : removed VM_MIXEDMAP flag from DAX VMAs. Now our testing shows that in the
> : mean time certain customer of ours started poking into /proc/<pid>/smaps
> : and looks at VMA flags there and if VM_MIXEDMAP is missing among the VMA
> : flags, the application just fails to start complaining that DAX support is
> : missing in the kernel.
>
> http://lkml.kernel.org/r/alpine.DEB.2.21.1809241054050.224429@chino.kir.corp.google.com
> : Commit 1860033237d4 ("mm: make PR_SET_THP_DISABLE immediately active")
> : introduced a regression in that userspace cannot always determine the set
> : of vmas where thp is ineligible.
> : Userspace relies on the "nh" flag being emitted as part of /proc/pid/smaps
> : to determine if a vma is eligible to be backed by hugepages.
> : Previous to this commit, prctl(PR_SET_THP_DISABLE, 1) would cause thp to
> : be disabled and emit "nh" as a flag for the corresponding vmas as part of
> : /proc/pid/smaps. After the commit, thp is disabled by means of an mm
> : flag and "nh" is not emitted.
> : This causes smaps parsing libraries to assume a vma is eligible for thp
> : and ends up puzzling the user on why its memory is not backed by thp.
>
> In both cases userspace was relying on a semantic of a specific VMA
> flag. The primary reason why that happened is a lack of a proper
> internface. While this has been worked on and it will be fixed properly,
> it seems that our wording could see some refinement and be more vocal
> about semantic aspect of these flags as well.
>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Rientjes <rientjes@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Agreed, although no amount of docs will override the
do-not-break-userspace rule I'm afraid :)
Acked-by: Vlastimil Babka <vbabka@suse.cz>
On top of typos reported by Mike:
> ---
> Documentation/filesystems/proc.txt | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index 12a5e6e693b6..b1fda309f067 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -496,7 +496,9 @@ flags associated with the particular virtual memory area in two letter encoded
>
> Note that there is no guarantee that every flag and associated mnemonic will
> be present in all further kernel releases. Things get changed, the flags may
> -be vanished or the reverse -- new added.
> +be vanished or the reverse -- new added. Interpretatation of their meaning
^ interpretation
> +might change in future as well. So each consumnent of these flags have to
> +follow each specific kernel version for the exact semantic.
>
> This file is only present if the CONFIG_MMU kernel configuration option is
> enabled.
>
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Florian Weimer @ 2018-11-23 13:34 UTC (permalink / raw)
To: Daniel Colascione
Cc: Michael Kerrisk (man-pages), linux-kernel, Joel Fernandes,
Linux API, Willy Tarreau, Vlastimil Babka, Carlos O'Donell,
libc-alpha@sourceware.org
In-Reply-To: <CAKOZuetdgk1QYhx1538-98rFpogMin=8DkPnCtU9_=ip23Vk7w@mail.gmail.com>
* Daniel Colascione:
> On Mon, Nov 12, 2018 at 12:11 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> * Daniel Colascione:
>>
>>> If the kernel provides a system call, libc should provide a C wrapper
>>> for it, even if in the opinion of the libc maintainers, that system
>>> call is flawed.
>>
>> It's not that simple, I think. What about bdflush? socketcall?
>> getxpid? osf_gettimeofday? set_robust_list?
>
> What about them? Mentioning that these system calls exist is not in
> itself an argument.
But socketcall does not exist on all architectures. Neither does
getpid, it's called getxpid on some architectures.
>> There are quite a few irregularities
>
> So?
I think it would be a poor approach to expose application developers to
these portability issues. We need to abstract over these differences at
a certain layer, and applications are too late.
>> and some editorial discretion appears to be unavoidable.
>
> That's an assertion, not an argument, and I strongly disagree. *Why*
> do you think "editorial discretion" is unavoidable?
We do not want application authors to write code which uses socketcall,
however it is the right system call for the BSD sockets API if you need
compatibility back to Linux 2.6.32 and before. If we application
authors seitched to socketall, applications would not be portable (at
the source level) to new architectures which do not have socketcall.
We do not want to force application authors to call osf_gettimeofday
instead of gettimeofday on Alpha.
We do not want to encourage library authors to call set_robust_list
because doing so would break robust mutex support in any libc.
>> Even if we were to provide perfectly consistent system call wrappers
>> under separate names, we'd still expose different calling conventions
>> for things like off_t to applications, which would make using some of
>> the system calls quite difficult and surprisingly non-portable.
>
> We can learn something from how Windows does things. On that system,
> what we think of as "libc" is actually two parts. (More, actually, but
> I'm simplifying.) At the lowest level, you have the semi-documented
> ntdll.dll, which contains raw system call wrappers and arcane
> kernel-userland glue. On top of ntdll live the "real" libc
> (msvcrt.dll, kernel32.dll, etc.) that provide conventional
> application-level glue. The tight integration between ntdll.dll and
> the kernel allows Windows to do very impressive things.
> We should adopt a similar approach.
Most kernel developers claim that a stable userspace ABI is desirable.
With your proposal, we need to maintain three stable ABI layers instead
of two, without actually adding any functionality. That doesn't seem to
be a good way of using developer resources.
Thanks,
Florian
^ 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-23 13:29 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: <644835950.10383.1542907783295.JavaMail.zimbra@efficios.com>
* Mathieu Desnoyers:
>>>> I don't think you need unregistering if the memory is initial-exec TLS
>>>> memory. Initial-exec TLS memory is tied directly to the TCB and cannot
>>>> be freed while the thread is running, so it should be safe to put the
>>>> rseq area there even if glibc knows nothing about it.
>>>
>>> Is it true for user-supplied stacks as well ?
>>
>> I'm not entirely sure because the glibc terminology is confusing, but I
>> think it places intial-exec TLS into the static TLS area (so that it has
>> a fixed offset from the TCB). The static TLS area is placed on the
>> user-supplied stack.
>
> You said earlier in the email thread that user-supplied stack can be
> reclaimed by __free_tcb () while the thread still runs, am I correct ?
> If so, then we really want to unregister the rseq TLS before that.
No, dynamic TLS can be reclaimed. Static TLS (which I assume includes
initial-exec TLS) is not deallocated.
> I notice that __free_tcb () calls __deallocate_stack (), which invokes
> _dl_deallocate_tls (). Accessing the TLS from the kernel upon preemption
> would appear fragile after this call.
_dl_deallocate_tls only covers dynamic TLS.
Thanks,
Florian
^ 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-23 13:10 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: <20181122171010.GH23599@brightrain.aerifal.cx>
* Rich Felker:
>> I'm not entirely sure because the glibc terminology is confusing, but I
>> think it places intial-exec TLS into the static TLS area (so that it has
>> a fixed offset from the TCB). The static TLS area is placed on the
>> user-supplied stack.
>
> This is an implementation detail that should not leak to applications,
> and I believe it's still considered a bug, in that, with large static
> TLS, it could overflow or leave unusably little space left on an
> otherwise-plenty-large application-provided stack.
Sure, but that does not matter in this context because right now, there
is no fix for this bug, and when we fix it, we can take backwards
compatibility into account.
Any library that ends up using rseq will need to coordinate with the
toolchain. I think that's unavoidable given the kernel interface.
>> > One issue here is that early adopter libraries cannot always use
>> > the IE model. I tried using it for other TLS variables in lttng-ust, and
>> > it ended up hanging our CI tests when tracing a sample application with
>> > lttng-ust under a Java virtual machine: being dlopen'd in a process that
>> > possibly already exhausts the number of available backup TLS IE entries
>> > seems to have odd effects. This is why I'm worried about using the IE model
>> > within lttng-ust.
>>
>> You can work around this by preloading the library. I'm not sure if
>> this is a compelling reason not to use initial-exec TLS memory.
>
> Use of IE model from a .so file (except possibly libc.so or something
> else that inherently needs to be present at program startup for other
> reasons) should be a considered a bug and unsupported usage.
> Encouraging libraries to perpetuate this behavior is going backwards
> on progress that's being made to end it.
Why? Just because glibc's TCB allocation strategy is problematic?
We can fix that, even with dlopen.
If you are only concerned about the interactions with dlopen, then why
do you think initial-exec TLS is the problem, and not dlopen?
>> > The per-thread reference counter is a way to avoid issues that arise from
>> > lack of destructor ordering. Is it an acceptable approach for you, or
>> > you have something else in mind ?
>>
>> Only for the involved libraries. It will not help if other TLS
>> destructors run and use these libraries.
>
> Presumably they should have registered their need for rseq too,
> thereby incrementing the reference count. I'm not sure this is a good
> idea, but I think I understand it now.
They may have to increase the reference count from 0 to 1, though, so
they have to re-register the rseq area. This tends to get rather messy.
I still I think implicit destruction of the rseq area is preferable over
this complexity.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v4 2/4] namei: O_BENEATH-style path resolution flags
From: Jürg Billeter @ 2018-11-23 12:10 UTC (permalink / raw)
To: Aleksa Sarai, Al Viro, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, David Howells
Cc: Eric Biederman, Christian Brauner, linux-api, Andy Lutomirski,
Jann Horn, David Drysdale, Aleksa Sarai, containers,
linux-fsdevel, linux-kernel, linux-arch
In-Reply-To: <20181112142654.341-3-cyphar@cyphar.com>
Hi Aleksa,
On Tue, 2018-11-13 at 01:26 +1100, Aleksa Sarai wrote:
> * O_BENEATH: Disallow "escapes" from the starting point of the
> filesystem tree during resolution (you must stay "beneath" the
> starting point at all times). Currently this is done by disallowing
> ".." and absolute paths (either in the given path or found during
> symlink resolution) entirely, as well as all "magic link" jumping.
With open_tree(2) and OPEN_TREE_CLONE, will O_BENEATH still be
necessary? As I understand it, O_BENEATH could be replaced by a much
simpler flag that only disallows absolute paths (incl. absolute
symlinks). And it would have the benefit that you can actually pass the
tree/directory fd to another process and escaping would not be possible
even if that other process doesn't use O_BENEATH (after calling
mount_setattr(2) to make sure it's locked down).
This approach would also make it easy to restrict writes via a cloned
tree/directory fd by marking it read-only via mount_setattr(2) (and
locking down the read-only flag). This would again be especially useful
when passing tree/directory fds across processes, or for voluntary
self-lockdown within a process for robustness against security bugs.
This wouldn't affect any of the other flags in this patch. And for full
equivalence to O_BENEATH you'd have to use O_NOMAGICLINKS in addition
to O_NOABSOLUTE, or whatever that new flag would be called.
Or is OPEN_TREE_CLONE too expensive for this use case? Or is there
anything else I'm missing?
Jürg
^ permalink raw reply
* RE: [PATCH] Add /proc/pid_generation
From: David Laight @ 2018-11-23 11:14 UTC (permalink / raw)
To: 'Kevin Easton', Daniel Colascione
Cc: linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
timmurray@google.com, primiano@google.com, joelaf@google.com,
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
In-Reply-To: <20181122111930.GA7164@ip-172-31-15-78>
From: Kevin Easton
> Sent: 22 November 2018 11:20
>
> 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:
Why not allocate a 48bit generation number to each 16bit pid?
Then you have a 64bit 'extended-pid' that can be assumed to never be reused.
Provided enough interfaces are enhanced to support 'extended-pid' values
you'll never get reused values.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2018-11-23 4:01 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
Oleg Nesterov, Steven Rostedt, LKML, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <CALCETrXvXzRzoEqwEY_VZ7Vpt-sLwaF+rZPg+y_eG2xyzubXtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 2300 bytes --]
On Thu, Nov 22, 2018 at 04:19:10PM -0800, Andy Lutomirski wrote:
> On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org> wrote:
> >
> > On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > > > >
> > > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > > >
> > > > > > struct ptrace_syscall_info {
> > > > > > __u8 op; /* 0 for entry, 1 for exit */
> > > > >
> > > > > Can you add proper defines, like:
> > > > >
> > > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > > #define PTRACE_SYSCALL_EXIT 1
> > > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > > >
> > > > > and make seccomp work from the start? I'd rather we don't merge an
> > > > > implementation that doesn't work for seccomp and then have to rework
> > > > > it later.
> > > >
> > > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > > with regards to PTRACE_GET_SYSCALL_INFO request? At least they have the
> > > > same entry_info to return.
> > >
> > > I'm not sure there's any material difference.
> >
> > In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> > describes the structure inside the union to use, not the ptrace stop.
>
> Unless we think the structures might diverge in the future.
If these structures ever diverge, then a seccomp structure will be added
to the union, and a portable userspace code will likely look this way:
#include <linux/ptrace.h>
...
struct ptrace_syscall_info info;
long rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid, (void *) sizeof(info), &info);
...
switch (info.op) {
case PTRACE_SYSCALL_INFO_ENTRY:
/* handle info.entry */
case PTRACE_SYSCALL_INFO_EXIT:
/* handle info.exit */
#ifdef PTRACE_SYSCALL_INFO_SECCOMP
case PTRACE_SYSCALL_INFO_SECCOMP:
/* handle info.seccomp */
#endif
default:
/* handle unknown info.op */
}
In other words, it would be better if PTRACE_SYSCALL_INFO_* selector
constants were introduced along with corresponding structures in the
union.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Andy Lutomirski @ 2018-11-23 0:19 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
Oleg Nesterov, Steven Rostedt, LKML, Andrew Lutomirski,
Ingo Molnar, strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <20181122191504.GB27204-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org> wrote:
>
> On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > > >
> > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > >
> > > > > struct ptrace_syscall_info {
> > > > > __u8 op; /* 0 for entry, 1 for exit */
> > > >
> > > > Can you add proper defines, like:
> > > >
> > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > #define PTRACE_SYSCALL_EXIT 1
> > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > >
> > > > and make seccomp work from the start? I'd rather we don't merge an
> > > > implementation that doesn't work for seccomp and then have to rework
> > > > it later.
> > >
> > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > with regards to PTRACE_GET_SYSCALL_INFO request? At least they have the
> > > same entry_info to return.
> >
> > I'm not sure there's any material difference.
>
> In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> describes the structure inside the union to use, not the ptrace stop.
Unless we think the structures might diverge in the future.
>
> > > 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.
>
> Unless we can change PTRACE_EVENT_SECCOMP to set some higher bits of
> ptrace_message (beyond SECCOMP_RET_DATA) which is very unlikely because
> it would qualify as an ABI change, this would require an additional field
> in struct task_struct because ptrace_message wouldn't be enough
> to distinguish PTRACE_EVENT_SECCOMP from PTRACE_EVENT_EXIT.
At the risk of making the patch more complicated, there's room to
massively clean up the ptrace state. We could add a struct
ptrace_tracee and put a struct ptrace_tracee *ptrace_tracee into
task_struct. The struct would contain a pointer to the task_struct as
well as ptrace (the flag field, I think), ptrace_entry, ptracer_cred,
ptrace_message, and last_siginfo. And then we could add a field for
the ptrace stop state that would indicate the actual reason for the
current stop. We'd only allocate ptrace_tracee when someone attaches
with ptrace, thus saving quite a few bytes for each task.
It's a bit unfortunate if we allow PTRACE_GET_SYSCALL_INFO to success
if the event is PTRACE_EVENT_EXIT. I'd also be a bit nervous about
info leaks if we start calling the syscall accessors for tasks that
aren't in syscalls.
--Andy
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [PATCH -next 2/2] selftests/memfd: modify tests for F_SEAL_FUTURE_WRITE seal
From: Joel Fernandes @ 2018-11-22 23:21 UTC (permalink / raw)
To: linux-kernel
Cc: Jann Horn, Andrew Morton, Andy Lutomirski, Hugh Dickins,
Khalid Aziz, linux-api, linux-kselftest, linux-mm,
Marc-André Lureau, Matthew Wilcox, Mike Kravetz, Shuah Khan,
Stephen Rothwell
In-Reply-To: <20181120052137.74317-2-joel@joelfernandes.org>
On Mon, Nov 19, 2018 at 09:21:37PM -0800, Joel Fernandes (Google) wrote:
> Modify the tests for F_SEAL_FUTURE_WRITE based on the changes
> introduced in previous patch.
>
> Also add a test to make sure the reopen issue pointed by Jann Horn [1]
> is fixed.
>
> [1] https://lore.kernel.org/lkml/CAG48ez1h=v-JYnDw81HaYJzOfrNhwYksxmc2r=cJvdQVgYM+NA@mail.gmail.com/
>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> tools/testing/selftests/memfd/memfd_test.c | 88 +++++++++++-----------
> 1 file changed, 44 insertions(+), 44 deletions(-)
Since we squashed [1] the mm/memfd patch modifications suggested by Andy into
the original patch, I also squashed the selftests modifications and appended
the patch inline below if you want to take this instead:
[1] https://lore.kernel.org/lkml/20181122230906.GA198127@google.com/T/#m8ba68f67f3ec24913a977b62bcaeafc4b194b8c8
---8<-----------------------
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH v4] selftests/memfd: add tests for F_SEAL_FUTURE_WRITE seal
Add tests to verify sealing memfds with the F_SEAL_FUTURE_WRITE works as
expected.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
tools/testing/selftests/memfd/memfd_test.c | 74 ++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 10baa1652fc2..c67d32eeb668 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -54,6 +54,22 @@ static int mfd_assert_new(const char *name, loff_t sz, unsigned int flags)
return fd;
}
+static int mfd_assert_reopen_fd(int fd_in)
+{
+ int r, fd;
+ char path[100];
+
+ sprintf(path, "/proc/self/fd/%d", fd_in);
+
+ fd = open(path, O_RDWR);
+ if (fd < 0) {
+ printf("re-open of existing fd %d failed\n", fd_in);
+ abort();
+ }
+
+ return fd;
+}
+
static void mfd_fail_new(const char *name, unsigned int flags)
{
int r;
@@ -255,6 +271,25 @@ static void mfd_assert_read(int fd)
munmap(p, mfd_def_size);
}
+/* Test that PROT_READ + MAP_SHARED mappings work. */
+static void mfd_assert_read_shared(int fd)
+{
+ void *p;
+
+ /* verify PROT_READ and MAP_SHARED *is* allowed */
+ p = mmap(NULL,
+ mfd_def_size,
+ PROT_READ,
+ MAP_SHARED,
+ fd,
+ 0);
+ if (p == MAP_FAILED) {
+ printf("mmap() failed: %m\n");
+ abort();
+ }
+ munmap(p, mfd_def_size);
+}
+
static void mfd_assert_write(int fd)
{
ssize_t l;
@@ -692,6 +727,44 @@ static void test_seal_write(void)
close(fd);
}
+/*
+ * Test SEAL_FUTURE_WRITE
+ * Test whether SEAL_FUTURE_WRITE actually prevents modifications.
+ */
+static void test_seal_future_write(void)
+{
+ int fd, fd2;
+ void *p;
+
+ printf("%s SEAL-FUTURE-WRITE\n", memfd_str);
+
+ fd = mfd_assert_new("kern_memfd_seal_future_write",
+ mfd_def_size,
+ MFD_CLOEXEC | MFD_ALLOW_SEALING);
+
+ p = mfd_assert_mmap_shared(fd);
+
+ mfd_assert_has_seals(fd, 0);
+
+ mfd_assert_add_seals(fd, F_SEAL_FUTURE_WRITE);
+ mfd_assert_has_seals(fd, F_SEAL_FUTURE_WRITE);
+
+ /* read should pass, writes should fail */
+ mfd_assert_read(fd);
+ mfd_assert_read_shared(fd);
+ mfd_fail_write(fd);
+
+ fd2 = mfd_assert_reopen_fd(fd);
+ /* read should pass, writes should still fail */
+ mfd_assert_read(fd2);
+ mfd_assert_read_shared(fd2);
+ mfd_fail_write(fd2);
+
+ munmap(p, mfd_def_size);
+ close(fd2);
+ close(fd);
+}
+
/*
* Test SEAL_SHRINK
* Test whether SEAL_SHRINK actually prevents shrinking
@@ -945,6 +1018,7 @@ int main(int argc, char **argv)
test_basic();
test_seal_write();
+ test_seal_future_write();
test_seal_shrink();
test_seal_grow();
test_seal_resize();
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [PATCH -next 1/2] mm/memfd: make F_SEAL_FUTURE_WRITE seal more robust
From: Joel Fernandes @ 2018-11-22 23:09 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Morton, Stephen Rothwell, 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: <CALCETrUGyhqi+M3cTdqJNNOPfTWn-R-ekM_R5heq2mbdVqPUAw@mail.gmail.com>
On Wed, Nov 21, 2018 at 07:25:26PM -0800, Andy Lutomirski wrote:
> 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.
Sure, I removed the old tags and also provide an updated patch below inline.
> > 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.
[...]
> > --- 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?
Yes, fixed.
> > + * 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.
Agreed, its simpler. Updated patch is below. I squashed it with all the
earlier ones. Andy, could you provide Acks and/or Reviewed-by tag as well?
---8<-----------------------
>From b5a4960e755af67e9f6f9e65db5113e712cf338e Mon Sep 17 00:00:00 2001
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Date: Sat, 10 Nov 2018 22:21:31 -0800
Subject: [PATCH v4] mm/memfd: 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.
A better way to do F_SEAL_FUTURE_WRITE seal was discussed [1] last week
where we don't need to modify core VFS structures to get the same
behavior of the seal. This solves several side-effects pointed by Andy.
self-tests are provided in later patch to verify the expected semantics.
[1] https://lore.kernel.org/lkml/20181111173650.GA256781@google.com/
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
fs/hugetlbfs/inode.c | 2 +-
include/uapi/linux/fcntl.h | 1 +
mm/memfd.c | 3 ++-
mm/shmem.c | 26 +++++++++++++++++++++++---
4 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 762028994f47..5b54bf893a67 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -558,7 +558,7 @@ static long hugetlbfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
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;
}
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 594b85f7cb86..1d338357df8a 100644
--- a/include/uapi/linux/fcntl.h
+++ b/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 */
/*
diff --git a/mm/memfd.c b/mm/memfd.c
index 97264c79d2cd..650e65a46b9c 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -131,7 +131,8 @@ static unsigned int *memfd_file_seals_ptr(struct file *file)
#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)
{
diff --git a/mm/shmem.c b/mm/shmem.c
index 32eb29bd72c6..f5069e8225cc 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2121,6 +2121,25 @@ int shmem_lock(struct file *file, int lock, struct user_struct *user)
static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
{
+ struct shmem_inode_info *info = SHMEM_I(file_inode(file));
+
+
+ if (info->seals & F_SEAL_FUTURE_WRITE) {
+ /*
+ * New PROT_WRITE and MAP_SHARED mmaps are not allowed when
+ * "future write" seal active.
+ */
+ if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_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.
+ */
+ vma->vm_flags &= ~(VM_MAYWRITE);
+ }
+
file_accessed(file);
vma->vm_ops = &shmem_vm_ops;
if (IS_ENABLED(CONFIG_TRANSPARENT_HUGE_PAGECACHE) &&
@@ -2346,8 +2365,9 @@ shmem_write_begin(struct file *file, struct address_space *mapping,
pgoff_t index = pos >> PAGE_SHIFT;
/* i_mutex is held by caller */
- if (unlikely(info->seals & (F_SEAL_WRITE | F_SEAL_GROW))) {
- if (info->seals & F_SEAL_WRITE)
+ if (unlikely(info->seals & (F_SEAL_GROW |
+ F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE))
return -EPERM;
if ((info->seals & F_SEAL_GROW) && pos + len > inode->i_size)
return -EPERM;
@@ -2610,7 +2630,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,
DECLARE_WAIT_QUEUE_HEAD_ONSTACK(shmem_falloc_waitq);
/* protected by i_mutex */
- if (info->seals & F_SEAL_WRITE) {
+ if (info->seals & (F_SEAL_WRITE | F_SEAL_FUTURE_WRITE)) {
error = -EPERM;
goto out;
}
--
2.19.1.1215.g8438c0b245-goog
^ permalink raw reply related
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Mathieu Desnoyers @ 2018-11-22 19:36 UTC (permalink / raw)
To: Rich Felker
Cc: Szabolcs Nagy, Florian Weimer, nd, carlos, Joseph Myers,
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: <20181122190143.GI23599@brightrain.aerifal.cx>
----- On Nov 22, 2018, at 2:01 PM, Rich Felker dalias@libc.org wrote:
> On Thu, Nov 22, 2018 at 01:35:44PM -0500, Mathieu Desnoyers wrote:
>> ----- On Nov 22, 2018, at 11:24 AM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>>
>> > On 22/11/18 15:33, Mathieu Desnoyers wrote:
>> >> ----- On Nov 22, 2018, at 10:21 AM, Florian Weimer fweimer@redhat.com wrote:
>> >>> 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.
>> >
>> > i think the man page should point this out.
>>
>> Yes, I should add this to the proposed rseq(2) man page.
>>
>> >
>> > the memory of a registered rseq object must not be freed
>> > before thread exit. (either unregister it or free later)
>> >
>> > and ideally also point out that c language thread storage
>> > duration does not provide this guarantee: it may be freed
>> > by the implementation before thread exit (which is currently
>> > not observable, but with the rseq syscall it is).
>>
>> How about the following wording ?
>>
>> Memory of a registered rseq object must not be freed before the
>> thread exits. Reclaim of rseq object's memory must only be
>> done after either an explicit rseq unregistration is performed
>> or after the thread exit. Keep in mind that the implementation
>> of the Thread-Local Storage (C language __thread) lifetime does
>> not guarantee existence of the TLS area up until the thread exits.
>
> This is all really ugly for application/library code to have to deal
> with. Maybe if the man page is considered as documenting the syscall
> only, and not something you can use, it's okay,
This is indeed for the rseq(2) manpage targeting the man-pages project,
which documents system calls.
> but "until the thread
> exits" is not well-defined in the sense you want it here. It's more
> like "until the kernel task for the thread exits", and the whole point
> is that there is some interval in time between the abstract thread
> exit and the kernel task exit that is not observable without rseq but
> is observable if the rseq is wrongly left installed.
It's important to clear a possible misunderstanding here: from the
point where the thread issues the "exit" system call, the kernel won't
touch the registered rseq TLS area anymore.
So the point where the thread exits is actually well defined, even from
a user-space perspective.
The problematic scenario arises when glibc frees the TLS memory
before invoking exit() when the thread terminates. In this kind of
scenario, we need to explicitly invoke rseq unregister before TLS
memory reclaim.
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: Dmitry V. Levin @ 2018-11-22 19:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
Oleg Nesterov, Steven Rostedt, LKML, Ingo Molnar,
strace-devel-3+4lAyCyj6AWlMsSdNXQLw
In-Reply-To: <CALCETrXea_uRAqw_srW5CWgOzeM=RubaDbjnxZ=cUMy5Zv1TsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1928 bytes --]
On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > >
> > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > >
> > > > struct ptrace_syscall_info {
> > > > __u8 op; /* 0 for entry, 1 for exit */
> > >
> > > Can you add proper defines, like:
> > >
> > > #define PTRACE_SYSCALL_ENTRY 0
> > > #define PTRACE_SYSCALL_EXIT 1
> > > #define PTRACE_SYSCALL_SECCOMP 2
> > >
> > > and make seccomp work from the start? I'd rather we don't merge an
> > > implementation that doesn't work for seccomp and then have to rework
> > > it later.
> >
> > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > with regards to PTRACE_GET_SYSCALL_INFO request? At least they have the
> > same entry_info to return.
>
> I'm not sure there's any material difference.
In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
describes the structure inside the union to use, not the ptrace stop.
> > 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.
Unless we can change PTRACE_EVENT_SECCOMP to set some higher bits of
ptrace_message (beyond SECCOMP_RET_DATA) which is very unlikely because
it would qualify as an ABI change, this would require an additional field
in struct task_struct because ptrace_message wouldn't be enough
to distinguish PTRACE_EVENT_SECCOMP from PTRACE_EVENT_EXIT.
--
ldv
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 137 bytes --]
--
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Rich Felker @ 2018-11-22 19:01 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Szabolcs Nagy, Florian Weimer, nd, carlos, Joseph Myers,
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: <1602745030.10426.1542911744479.JavaMail.zimbra@efficios.com>
On Thu, Nov 22, 2018 at 01:35:44PM -0500, Mathieu Desnoyers wrote:
> ----- On Nov 22, 2018, at 11:24 AM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>
> > On 22/11/18 15:33, Mathieu Desnoyers wrote:
> >> ----- On Nov 22, 2018, at 10:21 AM, Florian Weimer fweimer@redhat.com wrote:
> >>> 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.
> >
> > i think the man page should point this out.
>
> Yes, I should add this to the proposed rseq(2) man page.
>
> >
> > the memory of a registered rseq object must not be freed
> > before thread exit. (either unregister it or free later)
> >
> > and ideally also point out that c language thread storage
> > duration does not provide this guarantee: it may be freed
> > by the implementation before thread exit (which is currently
> > not observable, but with the rseq syscall it is).
>
> How about the following wording ?
>
> Memory of a registered rseq object must not be freed before the
> thread exits. Reclaim of rseq object's memory must only be
> done after either an explicit rseq unregistration is performed
> or after the thread exit. Keep in mind that the implementation
> of the Thread-Local Storage (C language __thread) lifetime does
> not guarantee existence of the TLS area up until the thread exits.
This is all really ugly for application/library code to have to deal
with. Maybe if the man page is considered as documenting the syscall
only, and not something you can use, it's okay, but "until the thread
exits" is not well-defined in the sense you want it here. It's more
like "until the kernel task for the thread exits", and the whole point
is that there is some interval in time between the abstract thread
exit and the kernel task exit that is not observable without rseq but
is observable if the rseq is wrongly left installed.
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 18:35 UTC (permalink / raw)
To: Szabolcs Nagy
Cc: Florian Weimer, nd, Rich Felker, carlos, Joseph Myers, 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: <7032ab45-8314-b85d-5b4d-f83b41dab5b6@arm.com>
----- On Nov 22, 2018, at 11:24 AM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
> On 22/11/18 15:33, Mathieu Desnoyers wrote:
>> ----- On Nov 22, 2018, at 10:21 AM, Florian Weimer fweimer@redhat.com wrote:
>>> 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.
>
> i think the man page should point this out.
Yes, I should add this to the proposed rseq(2) man page.
>
> the memory of a registered rseq object must not be freed
> before thread exit. (either unregister it or free later)
>
> and ideally also point out that c language thread storage
> duration does not provide this guarantee: it may be freed
> by the implementation before thread exit (which is currently
> not observable, but with the rseq syscall it is).
How about the following wording ?
Memory of a registered rseq object must not be freed before the
thread exits. Reclaim of rseq object's memory must only be
done after either an explicit rseq unregistration is performed
or after the thread exit. Keep in mind that the implementation
of the Thread-Local Storage (C language __thread) lifetime does
not guarantee existence of the TLS area up until the thread exits.
Thanks,
Mathieu
--
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: Mathieu Desnoyers @ 2018-11-22 17:29 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: <87k1l5xd33.fsf@oldenburg.str.redhat.com>
----- On Nov 22, 2018, at 11:59 AM, Florian Weimer fweimer@redhat.com wrote:
> * Mathieu Desnoyers:
>
>> ----- On Nov 22, 2018, at 11:28 AM, Florian Weimer fweimer@redhat.com wrote:
>>
>>> * Mathieu Desnoyers:
>>>
>>>> Here is one scenario: we have 2 early adopter libraries using rseq which
>>>> are deployed in an environment with an older glibc (which does not
>>>> support rseq).
>>>>
>>>> Of course, none of those libraries can be dlclose'd unless they somehow
>>>> track all registered threads.
>>>
>>> Well, you can always make them NODELETE so that dlclose is not an issue.
>>> If the library is small enough, that shouldn't be a problem.
>>
>> That's indeed what I do with lttng-ust, mainly due to use of pthread_key.
>>
>>>
>>>> But let's focus on how exactly those libraries can handle lazily
>>>> registering rseq. They can use pthread_key, and pthread_setspecific on
>>>> first use by the thread to setup a destructor function to be invoked
>>>> at thread exit. But each early adopter library is unaware of the
>>>> other, so if we just use a "is_initialized" flag, the first destructor
>>>> to run will unregister rseq while the second library may still be
>>>> using it.
>>>
>>> I don't think you need unregistering if the memory is initial-exec TLS
>>> memory. Initial-exec TLS memory is tied directly to the TCB and cannot
>>> be freed while the thread is running, so it should be safe to put the
>>> rseq area there even if glibc knows nothing about it.
>>
>> Is it true for user-supplied stacks as well ?
>
> I'm not entirely sure because the glibc terminology is confusing, but I
> think it places intial-exec TLS into the static TLS area (so that it has
> a fixed offset from the TCB). The static TLS area is placed on the
> user-supplied stack.
You said earlier in the email thread that user-supplied stack can be
reclaimed by __free_tcb () while the thread still runs, am I correct ?
If so, then we really want to unregister the rseq TLS before that.
I notice that __free_tcb () calls __deallocate_stack (), which invokes
_dl_deallocate_tls (). Accessing the TLS from the kernel upon preemption
would appear fragile after this call.
[...]
>> One issue here is that early adopter libraries cannot always use
>> the IE model. I tried using it for other TLS variables in lttng-ust, and
>> it ended up hanging our CI tests when tracing a sample application with
>> lttng-ust under a Java virtual machine: being dlopen'd in a process that
>> possibly already exhausts the number of available backup TLS IE entries
>> seems to have odd effects. This is why I'm worried about using the IE model
>> within lttng-ust.
>
> You can work around this by preloading the library. I'm not sure if
> this is a compelling reason not to use initial-exec TLS memory.
LTTng-UST is meant to be used as a dependency for e.g. a java logger,
or a python logger. Those rely on dlopen, and it would be very painful
to ask all users to preload lttng-ust within their environment which is
sometimes already complex. It works today through dlopen, and I consider
this a user-facing behavior which I am very reluctant to break.
>
>>>> The same problem arises if we have an application early adopter which
>>>> explicitly deal with rseq, with a library early adopter. The issue is
>>>> similar, except that the application will explicitly want to unregister
>>>> rseq before exiting the thread, which leaves a race window where rseq
>>>> is unregistered, but the library may still need to use it.
>>>>
>>>> The reference counter solves this: only the last rseq user for a thread
>>>> performs unregistration.
>>>
>>> If you do explicit unregistration, you will run into issues related to
>>> destructor ordering. You should really find a way to avoid that.
>>
>> The per-thread reference counter is a way to avoid issues that arise from
>> lack of destructor ordering. Is it an acceptable approach for you, or
>> you have something else in mind ?
>
> Only for the involved libraries. It will not help if other TLS
> destructors run and use these libraries.
You bring an interesting point. The reference counter suffice to ensure
that the kernel will not try to reference the TLS area beyond its registration
scope, but it does not guarantee that another destructor (or a signal
handler) won't try to use the rseq TLS area after it has been unregistered.
Unregistration of the TLS before freeing its memory is required for correctness.
However, a use-after-unregistration can be dealt with by other means. This
is one of the reasons why I want to upstream the "cpu_opv" system call into
Linux: this is a fallback mechanism to use when rseq cannot do forward
progress (e.g. debugger single-stepping), or to use in those scenarios
where rseq is not registered (early at thread creation, or late at thread
exit). Moreover, it allows handling use-cases of migration of data between
per-cpu data structures, which is pretty much impossible to do right if we
only have rseq available.
Thanks,
Mathieu
--
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: Rich Felker @ 2018-11-22 17:10 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: <87k1l5xd33.fsf@oldenburg.str.redhat.com>
On Thu, Nov 22, 2018 at 05:59:44PM +0100, Florian Weimer wrote:
> * Mathieu Desnoyers:
>
> > ----- On Nov 22, 2018, at 11:28 AM, Florian Weimer fweimer@redhat.com wrote:
> >
> >> * Mathieu Desnoyers:
> >>
> >>> Here is one scenario: we have 2 early adopter libraries using rseq which
> >>> are deployed in an environment with an older glibc (which does not
> >>> support rseq).
> >>>
> >>> Of course, none of those libraries can be dlclose'd unless they somehow
> >>> track all registered threads.
> >>
> >> Well, you can always make them NODELETE so that dlclose is not an issue.
> >> If the library is small enough, that shouldn't be a problem.
> >
> > That's indeed what I do with lttng-ust, mainly due to use of pthread_key.
> >
> >>
> >>> But let's focus on how exactly those libraries can handle lazily
> >>> registering rseq. They can use pthread_key, and pthread_setspecific on
> >>> first use by the thread to setup a destructor function to be invoked
> >>> at thread exit. But each early adopter library is unaware of the
> >>> other, so if we just use a "is_initialized" flag, the first destructor
> >>> to run will unregister rseq while the second library may still be
> >>> using it.
> >>
> >> I don't think you need unregistering if the memory is initial-exec TLS
> >> memory. Initial-exec TLS memory is tied directly to the TCB and cannot
> >> be freed while the thread is running, so it should be safe to put the
> >> rseq area there even if glibc knows nothing about it.
> >
> > Is it true for user-supplied stacks as well ?
>
> I'm not entirely sure because the glibc terminology is confusing, but I
> think it places intial-exec TLS into the static TLS area (so that it has
> a fixed offset from the TCB). The static TLS area is placed on the
> user-supplied stack.
This is an implementation detail that should not leak to applications,
and I believe it's still considered a bug, in that, with large static
TLS, it could overflow or leave unusably little space left on an
otherwise-plenty-large application-provided stack.
> > One issue here is that early adopter libraries cannot always use
> > the IE model. I tried using it for other TLS variables in lttng-ust, and
> > it ended up hanging our CI tests when tracing a sample application with
> > lttng-ust under a Java virtual machine: being dlopen'd in a process that
> > possibly already exhausts the number of available backup TLS IE entries
> > seems to have odd effects. This is why I'm worried about using the IE model
> > within lttng-ust.
>
> You can work around this by preloading the library. I'm not sure if
> this is a compelling reason not to use initial-exec TLS memory.
Use of IE model from a .so file (except possibly libc.so or something
else that inherently needs to be present at program startup for other
reasons) should be a considered a bug and unsupported usage.
Encouraging libraries to perpetuate this behavior is going backwards
on progress that's being made to end it.
> >>> The same problem arises if we have an application early adopter which
> >>> explicitly deal with rseq, with a library early adopter. The issue is
> >>> similar, except that the application will explicitly want to unregister
> >>> rseq before exiting the thread, which leaves a race window where rseq
> >>> is unregistered, but the library may still need to use it.
> >>>
> >>> The reference counter solves this: only the last rseq user for a thread
> >>> performs unregistration.
> >>
> >> If you do explicit unregistration, you will run into issues related to
> >> destructor ordering. You should really find a way to avoid that.
> >
> > The per-thread reference counter is a way to avoid issues that arise from
> > lack of destructor ordering. Is it an acceptable approach for you, or
> > you have something else in mind ?
>
> Only for the involved libraries. It will not help if other TLS
> destructors run and use these libraries.
Presumably they should have registered their need for rseq too,
thereby incrementing the reference count. I'm not sure this is a good
idea, but I think I understand it now.
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 16:59 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: <1045257294.10291.1542905262086.JavaMail.zimbra@efficios.com>
* Mathieu Desnoyers:
> ----- On Nov 22, 2018, at 11:28 AM, Florian Weimer fweimer@redhat.com wrote:
>
>> * Mathieu Desnoyers:
>>
>>> Here is one scenario: we have 2 early adopter libraries using rseq which
>>> are deployed in an environment with an older glibc (which does not
>>> support rseq).
>>>
>>> Of course, none of those libraries can be dlclose'd unless they somehow
>>> track all registered threads.
>>
>> Well, you can always make them NODELETE so that dlclose is not an issue.
>> If the library is small enough, that shouldn't be a problem.
>
> That's indeed what I do with lttng-ust, mainly due to use of pthread_key.
>
>>
>>> But let's focus on how exactly those libraries can handle lazily
>>> registering rseq. They can use pthread_key, and pthread_setspecific on
>>> first use by the thread to setup a destructor function to be invoked
>>> at thread exit. But each early adopter library is unaware of the
>>> other, so if we just use a "is_initialized" flag, the first destructor
>>> to run will unregister rseq while the second library may still be
>>> using it.
>>
>> I don't think you need unregistering if the memory is initial-exec TLS
>> memory. Initial-exec TLS memory is tied directly to the TCB and cannot
>> be freed while the thread is running, so it should be safe to put the
>> rseq area there even if glibc knows nothing about it.
>
> Is it true for user-supplied stacks as well ?
I'm not entirely sure because the glibc terminology is confusing, but I
think it places intial-exec TLS into the static TLS area (so that it has
a fixed offset from the TCB). The static TLS area is placed on the
user-supplied stack.
>> Then you'll only need a mechanism to find the address of the actually
>> active rseq area (which you probably have to store in a TLS variable
>> for performance reasons). And that part you need whether you have
>> reference counter or not.
>
> I'm not sure I follow your thoughts here. Currently, the __rseq_abi
> TLS symbol identifies a structure registered to the kernel. The
> "currently active" rseq critical section is identified by the field
> "rseq_cs" within the __rseq_abi structure.
>
> So here when you say "actually active rseq area", do you mean the
> currently registered struct rseq (__rseq_abi) or the currently running
> rseq critical section ? (pointed to by __rseq_abi.rseq_cs)
__rseq_abi.
> One issue here is that early adopter libraries cannot always use
> the IE model. I tried using it for other TLS variables in lttng-ust, and
> it ended up hanging our CI tests when tracing a sample application with
> lttng-ust under a Java virtual machine: being dlopen'd in a process that
> possibly already exhausts the number of available backup TLS IE entries
> seems to have odd effects. This is why I'm worried about using the IE model
> within lttng-ust.
You can work around this by preloading the library. I'm not sure if
this is a compelling reason not to use initial-exec TLS memory.
>>> The same problem arises if we have an application early adopter which
>>> explicitly deal with rseq, with a library early adopter. The issue is
>>> similar, except that the application will explicitly want to unregister
>>> rseq before exiting the thread, which leaves a race window where rseq
>>> is unregistered, but the library may still need to use it.
>>>
>>> The reference counter solves this: only the last rseq user for a thread
>>> performs unregistration.
>>
>> If you do explicit unregistration, you will run into issues related to
>> destructor ordering. You should really find a way to avoid that.
>
> The per-thread reference counter is a way to avoid issues that arise from
> lack of destructor ordering. Is it an acceptable approach for you, or
> you have something else in mind ?
Only for the involved libraries. It will not help if other TLS
destructors run and use these libraries.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v6 00/26] Control-flow Enforcement: Shadow Stack
From: Andy Lutomirski @ 2018-11-22 16:53 UTC (permalink / raw)
To: Yu-cheng Yu, Florian Weimer, Carlos O'Donell, Rich Felker
Cc: X86 ML, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, LKML,
open list:DOCUMENTATION, Linux-MM, linux-arch, Linux API,
Arnd Bergmann, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Eugene Syromiatnikov, H. J. Lu, Jann Horn, Jonathan Corbet,
Kees Cook, Mike Kravetz, Nadav Amit, Oleg Nesterov
In-Reply-To: <20181119214809.6086-1-yu-cheng.yu@intel.com>
[cc some more libc folks]
I have a general question about this patch set:
If I'm writing a user program, and I write a signal handler, there are
two things I want to make sure I can still do:
1. I want to be able to unwind directly from the signal handler
without involving sigreturn() -- that is, I want to make sure that
siglongjmp() works. How does this work? Is INCSSP involved? How
exactly does the user program know how much to increment SSP by? (And
why on Earth does INCSSP only consider the low 8 bits of its argument?
That sounds like a mistake. Can Intel still fix that? On the other
hand, what happens if you INCSSP off the end of the shadow stack
entirely? I assume the next access will fault as long as there's an
appropriate guard page.)
2. I want to be able to modify the signal context from a signal
handler such that, when the signal handler returns, it will return to
a frame higher up on the call stack than where the signal started and
to a different RIP value. How can I do this? I guess I can modify
the shadow stack with WRSS if WR_SHSTK_EN=1, but how do I tell the
kernel to kindly skip the frames I want to skip when I do sigreturn()?
The reason I'm asking #2 is that I think it's time to resurrect my old
vDSO syscall cancellation helper series here:
https://lwn.net/Articles/679434/
and it's not at all clear to me that __vdso_abort_pending_syscall()
can work without kernel assistance when CET is enabled. I want to
make sure that it can be done, or I want to come up with some other
way to allow a signal handler to abort a syscall while CET is on. I
could probably change __vdso_abort_pending_syscall() to instead point
RIP to __kernel_vsyscall's epilogue so that we con't change the depth
of the call stack. But I could imagine that other user programs might
engage in similar shenanigans and want to have some way to unwind a
signal's return context without actually jumping there a la
siglongjmp().
Also, what is the intended setting of WR_SHSTK_EN with this patch set applied?
(I suppose we could just say that 32-bit processes should not use CET,
but that seems a bit sad.)
^ 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 16:47 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: <87wop5xeit.fsf@oldenburg.str.redhat.com>
----- On Nov 22, 2018, at 11:28 AM, Florian Weimer fweimer@redhat.com wrote:
> * Mathieu Desnoyers:
>
>> Here is one scenario: we have 2 early adopter libraries using rseq which
>> are deployed in an environment with an older glibc (which does not
>> support rseq).
>>
>> Of course, none of those libraries can be dlclose'd unless they somehow
>> track all registered threads.
>
> Well, you can always make them NODELETE so that dlclose is not an issue.
> If the library is small enough, that shouldn't be a problem.
That's indeed what I do with lttng-ust, mainly due to use of pthread_key.
>
>> But let's focus on how exactly those libraries can handle lazily
>> registering rseq. They can use pthread_key, and pthread_setspecific on
>> first use by the thread to setup a destructor function to be invoked
>> at thread exit. But each early adopter library is unaware of the
>> other, so if we just use a "is_initialized" flag, the first destructor
>> to run will unregister rseq while the second library may still be
>> using it.
>
> I don't think you need unregistering if the memory is initial-exec TLS
> memory. Initial-exec TLS memory is tied directly to the TCB and cannot
> be freed while the thread is running, so it should be safe to put the
> rseq area there even if glibc knows nothing about it.
Is it true for user-supplied stacks as well ?
> Then you'll only
> need a mechanism to find the address of the actually active rseq area
> (which you probably have to store in a TLS variable for performance
> reasons). And that part you need whether you have reference counter or
> not.
I'm not sure I follow your thoughts here. Currently, the __rseq_abi
TLS symbol identifies a structure registered to the kernel. The
"currently active" rseq critical section is identified by the field
"rseq_cs" within the __rseq_abi structure.
So here when you say "actually active rseq area", do you mean the
currently registered struct rseq (__rseq_abi) or the currently running
rseq critical section ? (pointed to by __rseq_abi.rseq_cs)
One issue here is that early adopter libraries cannot always use
the IE model. I tried using it for other TLS variables in lttng-ust, and
it ended up hanging our CI tests when tracing a sample application with
lttng-ust under a Java virtual machine: being dlopen'd in a process that
possibly already exhausts the number of available backup TLS IE entries
seems to have odd effects. This is why I'm worried about using the IE model
within lttng-ust.
So using the IE model for glibc makes sense, because nobody dlopen
glibc AFAIK. But it's not so simple for early adopter libraries which
can be dlopen'd.
>
>> The same problem arises if we have an application early adopter which
>> explicitly deal with rseq, with a library early adopter. The issue is
>> similar, except that the application will explicitly want to unregister
>> rseq before exiting the thread, which leaves a race window where rseq
>> is unregistered, but the library may still need to use it.
>>
>> The reference counter solves this: only the last rseq user for a thread
>> performs unregistration.
>
> If you do explicit unregistration, you will run into issues related to
> destructor ordering. You should really find a way to avoid that.
The per-thread reference counter is a way to avoid issues that arise from
lack of destructor ordering. Is it an acceptable approach for you, or
you have something else in mind ?
Thanks,
Mathieu
--
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 16:28 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: <686626451.10113.1542901620250.JavaMail.zimbra@efficios.com>
* Mathieu Desnoyers:
> Here is one scenario: we have 2 early adopter libraries using rseq which
> are deployed in an environment with an older glibc (which does not
> support rseq).
>
> Of course, none of those libraries can be dlclose'd unless they somehow
> track all registered threads.
Well, you can always make them NODELETE so that dlclose is not an issue.
If the library is small enough, that shouldn't be a problem.
> But let's focus on how exactly those libraries can handle lazily
> registering rseq. They can use pthread_key, and pthread_setspecific on
> first use by the thread to setup a destructor function to be invoked
> at thread exit. But each early adopter library is unaware of the
> other, so if we just use a "is_initialized" flag, the first destructor
> to run will unregister rseq while the second library may still be
> using it.
I don't think you need unregistering if the memory is initial-exec TLS
memory. Initial-exec TLS memory is tied directly to the TCB and cannot
be freed while the thread is running, so it should be safe to put the
rseq area there even if glibc knows nothing about it. Then you'll only
need a mechanism to find the address of the actually active rseq area
(which you probably have to store in a TLS variable for performance
reasons). And that part you need whether you have reference counter or
not.
> The same problem arises if we have an application early adopter which
> explicitly deal with rseq, with a library early adopter. The issue is
> similar, except that the application will explicitly want to unregister
> rseq before exiting the thread, which leaves a race window where rseq
> is unregistered, but the library may still need to use it.
>
> The reference counter solves this: only the last rseq user for a thread
> performs unregistration.
If you do explicit unregistration, you will run into issues related to
destructor ordering. You should really find a way to avoid that.
Thanks,
Florian
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Szabolcs Nagy @ 2018-11-22 16:24 UTC (permalink / raw)
To: Mathieu Desnoyers, Florian Weimer
Cc: nd, Rich Felker, carlos, Joseph Myers, 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 22/11/18 15:33, Mathieu Desnoyers wrote:
> ----- On Nov 22, 2018, at 10:21 AM, Florian Weimer fweimer@redhat.com wrote:
>> 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.
i think the man page should point this out.
the memory of a registered rseq object must not be freed
before thread exit. (either unregister it or free later)
and ideally also point out that c language thread storage
duration does not provide this guarantee: it may be freed
by the implementation before thread exit (which is currently
not observable, but with the rseq syscall it is).
^ 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:47 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: <20181122151444.GE23599@brightrain.aerifal.cx>
----- On Nov 22, 2018, at 10:14 AM, Rich Felker dalias@libc.org wrote:
> 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.
As discussed in the other leg of the email thread, disabling signals is
not enough to prevent the kernel to access the rseq TLS area on preemption.
> 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.
Here is one scenario: we have 2 early adopter libraries using rseq which
are deployed in an environment with an older glibc (which does not
support rseq).
Of course, none of those libraries can be dlclose'd unless they somehow
track all registered threads. But let's focus on how exactly those
libraries can handle lazily registering rseq. They can use pthread_key,
and pthread_setspecific on first use by the thread to setup a destructor
function to be invoked at thread exit. But each early adopter library
is unaware of the other, so if we just use a "is_initialized" flag, the
first destructor to run will unregister rseq while the second library
may still be using it.
The same problem arises if we have an application early adopter which
explicitly deal with rseq, with a library early adopter. The issue is
similar, except that the application will explicitly want to unregister
rseq before exiting the thread, which leaves a race window where rseq
is unregistered, but the library may still need to use it.
The reference counter solves this: only the last rseq user for a thread
performs unregistration.
Thanks,
Mathieu
--
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: 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
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