Message-ID: <1565638541.5051.1594229688015.JavaMail.zimbra@efficios.com>
Date:   Wed, 8 Jul 2020 13:34:48 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     Florian Weimer <fw@...eb.enyo.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        carlos <carlos@...hat.com>, Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        paulmck <paulmck@...ux.ibm.com>,
        Boqun Feng <boqun.feng@...il.com>,
        "H. Peter Anvin" <hpa@...or.com>, Paul Turner <pjt@...gle.com>,
        linux-api <linux-api@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Neel Natu <neelnatu@...gle.com>
Subject: Re: [RFC PATCH for 5.8 3/4] rseq: Introduce
 RSEQ_FLAG_RELIABLE_CPU_ID

----- On Jul 8, 2020, at 12:22 PM, Christian Brauner christian.brauner@...ntu.com wrote:
[...]
> I've been following this a little bit. The kernel version itself doesn't
> really mean anything and the kernel version is imho not at all
> interesting to userspace applications. Especially for cross-distro
> programs. We can't go around and ask Red Hat, SUSE, Ubuntu, Archlinux,
> openSUSE and god knows who what other distro what their fixed kernel
> version is. That's not feasible at all and not how must programs do it.
> Sure, a lot of programs name a minimal kernel version they require but
> realistically we can't keep bumping it all the time. So the best
> strategy for userspace imho has been to introduce a re-versioned flag or
> enum that indicates the fixed behavior.
> 
> So I would suggest to just introduce
> RSEQ_FLAG_REGISTER_2                      = (1 << 2),
> that's how these things are usually done (Netlink etc.). So not
> introducing a fix bit or whatever but simply reversion your flag/enum.
> We already deal with this today.

Because rseq is effectively a per-thread resource shared across application
and libraries, it is not practical to merge the notion of version with the
registration. Typically __rseq_abi is registered by libc, and can be used
by the application and by many libraries. Early adopter libraries and
applications (e.g. librseq, tcmalloc) can also choose to handle registration
if it's not already done by libc.

For instance, it is acceptable for glibc to register rseq for all threads,
even in the presence of the cpu_id field inaccuracy, for use by the
sched_getcpu(3) implementation. However, users of rseq which need to
implement critical sections performing per-cpu data updates may want
to know whether the cpu_id field is reliable to ensure they do not crash
the process due to per-cpu data corruption.

This led me to consider exposing a feature-specific flag which can be
queried by specific users to know whether rseq has specific set of correct
behaviors implemented.

> (Also, as a side-note. I see that you're passing struct rseq *rseq with
> a length argument but you are not versioning by size. Is that
> intentional? That basically somewhat locks you to the current struct
> rseq layout and means users might run into problems when you extend
> struct rseq in the future as they can't pass the new struct down to
> older kernels. The way we deal with this is now - rseq might preceed
> this - is copy_struct_from_user() (for example in sched_{get,set}attr(),
> openat2(), bpf(), clone3(), etc.). Maybe you want to switch to that to
> keep rseq extensible? Users can detect the new rseq version by just
> passing a larger struct down to the kernel with the extra bytes set to 0
> and if rseq doesn't complain they know they're dealing with an rseq that
> knows larger struct sizes. Might be worth it if you have any reason to
> belive that struct rseq might need to grow.)

In the initial iterations of the rseq patch set, we initially had the rseq_len
argument hoping we would eventually be able to extend struct rseq. However,
it was finally decided against making it extensible, so the rseq ABI merged
into the Linux kernel with a fixed-size.

One of the key reasons for this is explained in
commit 83b0b15bcb0f ("rseq: Remove superfluous rseq_len from task_struct")

    The rseq system call, when invoked with flags of "0" or
    "RSEQ_FLAG_UNREGISTER" values, expects the rseq_len parameter to
    be equal to sizeof(struct rseq), which is fixed-size and fixed-layout,
    specified in uapi linux/rseq.h.
    
    Expecting a fixed size for rseq_len is a design choice that ensures
    multiple libraries and application defining __rseq_abi in the same
    process agree on its exact size.

The issue here is caused by the fact that the __rseq_abi variable is
shared across application/libraries for a given thread. So it's not
enough to agree between kernel and user-space on the extensibility
scheme, but we'd also have to find a way for all users within a process
to agree on the layout.

The "rseq_len" parameter could eventually become useful when combined
with additional flags, but currently its size is fixed.

There are indeed clear use-cases for extending struct rseq (or add a new
similar TLS structure), for instance exposing the current numa node id,
or to implement a fast signal-disabling scheme. But the fact that struct rseq
is shared across application/libraries makes it tricky to "just extend" struct
rseq.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists