userspace: Support for split 64 bit arguments #18064

andyross · 2019-08-06T22:09:26Z

userspace: Support for split 64 bit arguments

System call arguments, at the arch layer, are single words. So
passing wider values requires splitting them into two registers at
call time. This gets even more complicated for values (e.g
k_timeout_t) that may have different sizes depending on configuration.
This patch adds a feature to gen_syscalls.py to detect functions with
wide arguments and automatically generates code to split/unsplit them.

Unfortunately the current scheme of Z_SYSCALL_DECLARE_* macros won't
work with functions like this, because for N arguments (our current
maximum N is 10) there are 2^N possible configurations of argument
widths. So this generates the complete functions for each handler and
wrapper, effectively doing in python what was originally done in the
preprocessor.

Another complexity is that traditional the z_hdlr_() function for a
system call has taken the raw list of word arguments, which does not
work when some of those arguments must be 64 bit types. So instead of
using a single Z_SYSCALL_HANDLER macro, this splits the job of
z_hdlr_() into two steps: An automatically-generated unmarshalling
function, z_mrsh_(), which then calls a user-supplied verification
function z_vrfy_(). The verification function is typesafe, and is a
simple C function with exactly the same argument and return signature
as the syscall impl function. It is also not responsible for
validating the pointers to the extra parameter array or a wide return
value, that code gets automatically generated.

One downside is that the generated dispatch "mrsh" handler currently
needs to live in the syscall_dispatch.c file. So it's only able to
inline the vrfy/impl functions if those appear in the syscall header.
Implementations defined in C source files need to be called as real
functions. Long term, we could fix this by emitting a new header that
can be included at the bottom of C files containing syscalls
(analogous to the way the syscalls/*.h headers work). But this seems
OK for now.

Note that in theory, this new code is capable of generating wrappers
and handlers for any system call, there's nothing specific to 64 bit
arguments per se. In theory any system call could use that, and the
z_vrfy() scheme, with an appropriate way to tag it. Alternatively we
could consider migrating all the existing syscalls to this mechanism,
which would allow us to drop almost all the existing macro code and
generation.

andyross · 2019-08-06T22:10:59Z

Whoops, clicked on "open" before I'd pasted the description. Hopefully got it editted before the email goes out.

This is working now. It was done in the context of the timer API work to support a variable-sized k_timeout_t, but is submitted here for easier review as it has nothing in particular to do with timing.

zephyrbot · 2019-08-06T22:15:30Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

andrewboie · 2019-08-07T18:37:46Z

Note that in theory, this new code is capable of generating wrappers
and handlers for any system call, there's nothing specific to 64 bit
arguments per se. In theory any system call could use that, and the
z_vrfy() scheme, with an appropriate way to tag it. Alternatively we
could consider migrating all the existing syscalls to this mechanism,
which would allow us to drop almost all the existing macro code and
generation.

I would like to see this happen.
Having two different implementations and two different methods of defining syscalls is not good for Zephyr. If we need a new scheme so be it, but we need to commit to it and not have two.

This is working now. It was done in the context of the timer API work to support a variable-sized k_timeout_t, but is submitted here for easier review as it has nothing in particular to do with timing.

I'd like to see a PR with existing system calls converted to this mechanism.
Right now, the z_vrfy_* API doesn't take an ssf pointer, so Z_OOPS() won't work.
We want to get rid of Z_OOPS (see #17735) but it seems #17735 will need to be done first before we can convert existing syscalls.

One downside is that the generated dispatch "mrsh" handler currently
needs to live in the syscall_dispatch.c file. So it's only able to
inline the vrfy/impl functions if those appear in the syscall header.
Implementations defined in C source files need to be called as real
functions. Long term, we could fix this by emitting a new header that
can be included at the bottom of C files containing syscalls
(analogous to the way the syscalls/*.h headers work). But this seems
OK for now.

Is it really "OK for now?"
Seems like any syscall implementation function which is implemented as an inline function, will actually never be inlined if CONFIG_USERSPACE is turned on. Even if called from supervisor mode. I'd rather see this completely solved instead of punting.

andrewboie · 2019-08-07T20:15:01Z

Just to make my review clear: I don't want two different syscall generation mechanisms present in Zephyr simultaneously, and the new mechanism should be at functional parity with the old one.

If I understand this code correctly, it should be mechanical to replace instances of Z_SYSCALL_HANDLER() with the new scheme, as the body of the syscall handlers shouldn't have to change, just the function prototypes. Doing this will remove the need to maintain two mechanisms and more extensively show that this actually works and is at parity with the old mechanism.

I also don't want to see inline syscall implementations dragged down to non-inline status, I think the <syscalls/xxxxx.h> mechanism should be updated to handle this correctly.

Finally, the syscall documentation needs to be updated, it currently describes the old mechanism only.

andyross · 2019-08-08T15:49:11Z

OK, will work up a patch series to do it universally everywhere. And getting it inlining won't be difficult, just wordy (FWIW: the flaw is just the inlining of impl into the kernel-mode handler. Any other code that can inline it now will continue to work, it's just that during an actual syscall, we need to make an extra function call. This seemed tolerable enough given the overhead we're already taking, but it's not hard to fix).

As for ssf, yeah, that's a design flaw that it got forgotten. Have it working now by caching the value in the CPU struct during syscall execution so that you can Z_OOPS() from anywhere down the call stack.

andyross · 2019-08-13T16:33:06Z

As promised, here's one that reworks the whole system to use the new mrsh/vrfy scheme, with doc changes and removal of most of the legacy code (like all good features, this removes more code than it adds, heh).

Well... almost. It includes every syscall that gets hit during execution of a top-level sanitycheck. There are a few dozen that don't and are unmodified in this patch. I'll have those in within a few hours, in separate patches that carry warnings about blind changes (most of the changes are routine and scriptable). Currently trying to figure out a rig to at least get some build coverage in a pseudoautomatic way.

andrewboie · 2019-08-13T16:46:41Z

subsys/net/lib/sockets/sockets.c

@@ -353,6 +357,7 @@ Z_SYSCALL_HANDLER(zsock_connect, sock, addr, addrlen)
 	return z_impl_zsock_connect(sock, (struct sockaddr *)&dest_addr_copy,
 				   addrlen);
 }
+#include <syscalls/zsock_connect_mrsh.c>


Just a quick question, as I'm still reviewing this and haven't wrapped my head completely around how the build in this new scheme works. Instead of having to #include a separate C file for every syscall implemented, would it be possible to just put all the syscall marshalling functions in a single C file which gets built and linked in? or is there something specific which requires each marshalling function to be included in this way?

That's the way the original scheme worked, actually (they were in syscall_dispatch.c). But it has the side effect of being unable to inline the impl function unless it is in a header. This way they can live wherever the user wants them.

Personally I don't think that extra function call per actually-taken-syscall is a big deal, so I'd be happy with either scheme.

Personally I don't think that extra function call per actually-taken-syscall is a big deal, so I'd be happy with either scheme.

If I understand you correctly I am fine with that also and would rather just put them all in a single C dispatch file.

The inlining I was worried about was supervisor mode calls to syscall APIs, which are currently inlined if the z_impl_xxxx() function for it is declared inline.

+1 to putting the marshalling functions to separate C file.

Also, why not use the macro to create the function name so instead of this

static inline int z_vrfy_zsock_listen(int sock, int backlog)

we could have this

static int Z_SYSCALL_HANDLER(zsock_listen, int sock, int backlog)

so user does not need to know / remember how the function should be named.

andyross · 2019-08-13T17:20:59Z

Fix milestone. This is late, and scope grew at the last minute, but we need it in for the k_timeout_t stuff.

andyross · 2019-08-13T17:24:00Z

kernel/sem.c

+
+static inline unsigned int z_vrfy_k_sem_count_get(struct k_sem *sem)
+{
+	printk("%s @ %p\n", __func__, sem);


Whoops, debug code left in by mistake. Will fix in next version.

andyross · 2019-08-14T16:01:47Z

that'sFrustratingly, eliminating the "include mrsh file" scheme is not going to work everywhere. There are two problems: the minor one is a code size increase, because if we have a custom-generated handler for even disabled syscalls, that's a few hundred bytes extra vs. just putting handler_no_syscall in that slot.

The second one is harder. Syscalls have typed arguments, and to generate code for them in the general case means that we need to include the headers that define those types. I made this work. But then the problem is that these headers often don't compile in the wrong kconfig environment (i.e. if networking isn't defined, the network headers blow up). These are bugs, arguably, but they're looking to be difficult to fix in the immediate term.

I think we need to use the "include the mrsh file" scheme in the general case. There's really no other way to give the user control over the compilation environment of the handler.

How about this: we can commit this using the current scheme, and I'll submit an issue for 2.1 for a __syscall_simple() annotation that can generate the vrfy function also for common cases: we can detect the type of kobjects from the signature already, we can provide an annotation for struct device subtypes, we can even auto-generate copies of fixed-size pointer blocks. And using the two-stage link (which I haven't looked at since before cmake landed) we can even trim the list of generated syscalls to just those seen in linked code. I have lots of ideas. :)

Basically most stuff would port to that, except complicated handlers, which would still use hand-written vrfy functions and use the mrsh include. But those are the cases where the new code helps the most already, so it would still be a win.

Sound OK?

dbkinder · 2019-08-16T20:39:25Z

doc/reference/usermode/syscalls.rst

-    {
-        struct _syscall_9_args *margs = (struct _syscall_9_args *)more_args_ptr;
+All system calls are dispatched to a verifier function with a prefixed
+z_vrfy_ name based on the system call.  They have exactly the same


in reST, a trailing underscore indicates an outgoing link, hence the doc build error saying there's no link target for the name z_vrfy. The way to fix this is to change this reference to:

... with a prefixed ``z_vrfy_`` name

dbkinder · 2019-08-16T20:42:25Z

doc/reference/usermode/syscalls.rst

@@ -135,6 +135,8 @@ the project out directory under ``include/generated/``:
  API call, but the sensor subsystem is not enabled, the weak handler
  will be invoked instead.

+* An unmarshalling function will be defined in ``include/generated/<name>_mrsh.c``


Since the other bullets are expressed in the present tense (is created), this would read better if it were too:

* An unmarshalling function is defined ...

(unless this generated function really will be defined in the future :)

dbkinder · 2019-08-16T20:45:52Z

doc/reference/usermode/syscalls.rst

-untrusted memory in the handler function. This is done by the derived
-functions :c:func:`_syscall_invoke7` through :c:func:`_syscall_invoke10`.
+Some system calls may have more than six arguments. The number of
+arguments passed via registers is fixed at six for all


Is it fixed at six (meaning all system calls pass six arguments) or should it say limited to six?

dbkinder · 2019-08-16T20:49:38Z

doc/reference/usermode/syscalls.rst

-:c:func:`_syscall_ret64_invoke1`.
+Some system calls may return a value that will not fit in a 32-bit
+register, such as APIs that return a 64-bit value. In this scenario,
+the return value is populated in a (untrusted!) memory buffer that is


how about, populated in an **untrusted** memory buffer (use bold for emphasis instead of the parentheses and exclamation point.

andyross · 2019-08-20T17:23:51Z

@dbkinder docs updated, please recheck?

dbkinder

+1 for doc changes, thanks!

pizi-nordic · 2019-08-23T10:28:03Z

I noted that 64-bit return value always goes through memory.
Why we could not return it "normally" (in register pair)?

andyross · 2019-08-23T15:53:53Z

@pizi-nordic: That's just the way we've defined the ABI. The architecture syscall hooks are C functions that take 0-6 word arguments (actually it's hard-coded to u32_t right now, though that's easy enough to change once we start supporting syscalls on a 64 bit target) and return one output word.

Adding a new scheme with more than one output word would require every supporting architecture to write new hooks, so it's a heavy-weight change. But maybe worth it long term.

pizi-nordic · 2019-08-26T07:04:28Z

@andyross: Ok. IMHO returning data in registers will be faster (because most of ABIs the already use registers to pass the 64-bit return value) and safer (no write from kernel to userspace).

andyross · 2019-08-26T15:27:33Z

Agreed. But that's not what this PR is about, it's taking the existing syscall ABI (i.e. not changing any arch code) and changing the way we emit handlers for them so that more of the code can be automatically generated.

Long term I agree that we should look at improvements to the arch interface as well.

andyross · 2019-08-28T17:03:14Z

Routine rebase. Picked up support for the logging subsystem syscalls that were just added.

andyross · 2019-09-03T15:15:39Z

Rebase again. No changes, just keeping it fresh for an immediate post-release merge

System call arguments, at the arch layer, are single words. So passing wider values requires splitting them into two registers at call time. This gets even more complicated for values (e.g k_timeout_t) that may have different sizes depending on configuration. This patch adds a feature to gen_syscalls.py to detect functions with wide arguments and automatically generates code to split/unsplit them. Unfortunately the current scheme of Z_SYSCALL_DECLARE_* macros won't work with functions like this, because for N arguments (our current maximum N is 10) there are 2^N possible configurations of argument widths. So this generates the complete functions for each handler and wrapper, effectively doing in python what was originally done in the preprocessor. Another complexity is that traditional the z_hdlr_*() function for a system call has taken the raw list of word arguments, which does not work when some of those arguments must be 64 bit types. So instead of using a single Z_SYSCALL_HANDLER macro, this splits the job of z_hdlr_*() into two steps: An automatically-generated unmarshalling function, z_mrsh_*(), which then calls a user-supplied verification function z_vrfy_*(). The verification function is typesafe, and is a simple C function with exactly the same argument and return signature as the syscall impl function. It is also not responsible for validating the pointers to the extra parameter array or a wide return value, that code gets automatically generated. This commit includes new vrfy/msrh handling for all syscalls invoked during CI runs. Future commits will port the less testable code. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>

Add some simple tests of the new code generation for syscalls with 64 bit arguments. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>

These calls are buildable on common sanitycheck platforms, but are not invoked at runtime in any tests accessible to CI. The changes are mostly mechanical, so the risk is low, but this commit is separated from the main API change to allow for more careful review. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>

These calls are not accessible in CI test, nor do they get built on common platforms (in at least one case I found a typo which proved the code was truly unused). These changes are blind, so live in a separate commit. But the nature of the port is mechanical, all other syscalls in the system work fine, and any errors should be easily corrected. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>

The semi-automated API changes weren't checkpatch aware. Fix up whitespace warnings that snuck into the previous patches. Really this should be squashed, but that's somewhat difficult given the structure of the series. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>

andyross · 2019-09-04T22:47:48Z

Rebase again to fix the collision with #18744

andyross · 2019-09-09T15:48:23Z

Ping. Can we get this in now?

andyross requested review from andrewboie, carlescufi, galak, MaureenHelm and nashif as code owners August 6, 2019 22:09

andyross changed the title ~~Syscall split~~ userspace: Support for split 64 bit arguments Aug 6, 2019

zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Kernel labels Aug 6, 2019

aescolar added the area: Userspace Userspace label Aug 7, 2019

andyross force-pushed the syscall-split branch from 49a9fd5 to edd1e74 Compare August 7, 2019 16:04

ioannisg added this to the v2.1.0 milestone Aug 10, 2019

andyross force-pushed the syscall-split branch from edd1e74 to e894c3e Compare August 13, 2019 16:29

andyross requested review from dbkinder, jukkar, pfalcon and tbursztyka as code owners August 13, 2019 16:29

andrewboie reviewed Aug 13, 2019

View reviewed changes

andyross modified the milestones: v2.1.0, v2.0.0 Aug 13, 2019

andyross commented Aug 13, 2019

View reviewed changes

zephyrbot added area: C Library C Standard Library area: Networking labels Aug 13, 2019

andyross force-pushed the syscall-split branch from 757aac3 to d9e3ba9 Compare August 14, 2019 16:04

andyross mentioned this pull request Aug 15, 2019

New timeout API #17155

Closed

andrewboie approved these changes Aug 16, 2019

View reviewed changes

dbkinder suggested changes Aug 16, 2019

View reviewed changes

andyross force-pushed the syscall-split branch from d9e3ba9 to 6117c6b Compare August 20, 2019 17:22

dbkinder approved these changes Aug 20, 2019

View reviewed changes

pizi-nordic self-requested a review August 21, 2019 07:40

nashif modified the milestones: v2.0.0, v2.1.0 Aug 26, 2019

pfalcon mentioned this pull request Aug 28, 2019

kernel: deprecate and replace k_uptime_get_32() #18742

Closed

andyross force-pushed the syscall-split branch from 6117c6b to 9a3320f Compare August 28, 2019 17:02

andyross force-pushed the syscall-split branch from 9a3320f to 88e2e56 Compare September 3, 2019 15:15

Andy Ross added 5 commits September 4, 2019 15:39

tests/kernel/mem_protect/syscalls: Add cases to test 64 bit arguments

ff19b67

Add some simple tests of the new code generation for syscalls with 64 bit arguments. Signed-off-by: Andy Ross <andrew.j.ross@intel.com>

andyross force-pushed the syscall-split branch from 88e2e56 to 5b9d9bf Compare September 4, 2019 22:45

nashif approved these changes Sep 12, 2019

View reviewed changes

nashif merged commit 643701a into zephyrproject-rtos:master Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

userspace: Support for split 64 bit arguments #18064

userspace: Support for split 64 bit arguments #18064

andyross commented Aug 6, 2019 •

edited

Loading

andyross commented Aug 6, 2019

zephyrbot commented Aug 6, 2019 •

edited

Loading

andrewboie commented Aug 7, 2019

andrewboie commented Aug 7, 2019 •

edited

Loading

andyross commented Aug 8, 2019

andyross commented Aug 13, 2019

andrewboie Aug 13, 2019

andyross Aug 13, 2019

andrewboie Aug 13, 2019

jukkar Aug 14, 2019 •

edited

Loading

andyross commented Aug 13, 2019

andyross Aug 13, 2019

andyross commented Aug 14, 2019

dbkinder Aug 16, 2019

dbkinder Aug 16, 2019 •

edited

Loading

dbkinder Aug 16, 2019

dbkinder Aug 16, 2019

andyross commented Aug 20, 2019

dbkinder left a comment

pizi-nordic commented Aug 23, 2019

andyross commented Aug 23, 2019

pizi-nordic commented Aug 26, 2019 •

edited

Loading

andyross commented Aug 26, 2019

andyross commented Aug 28, 2019

andyross commented Sep 3, 2019

andyross commented Sep 4, 2019

andyross commented Sep 9, 2019

userspace: Support for split 64 bit arguments #18064

userspace: Support for split 64 bit arguments #18064

Conversation

andyross commented Aug 6, 2019 • edited Loading

andyross commented Aug 6, 2019

zephyrbot commented Aug 6, 2019 • edited Loading

andrewboie commented Aug 7, 2019

andrewboie commented Aug 7, 2019 • edited Loading

andyross commented Aug 8, 2019

andyross commented Aug 13, 2019

andrewboie Aug 13, 2019

Choose a reason for hiding this comment

andyross Aug 13, 2019

Choose a reason for hiding this comment

andrewboie Aug 13, 2019

Choose a reason for hiding this comment

jukkar Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

andyross commented Aug 13, 2019

andyross Aug 13, 2019

Choose a reason for hiding this comment

andyross commented Aug 14, 2019

dbkinder Aug 16, 2019

Choose a reason for hiding this comment

dbkinder Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

dbkinder Aug 16, 2019

Choose a reason for hiding this comment

dbkinder Aug 16, 2019

Choose a reason for hiding this comment

andyross commented Aug 20, 2019

dbkinder left a comment

Choose a reason for hiding this comment

pizi-nordic commented Aug 23, 2019

andyross commented Aug 23, 2019

pizi-nordic commented Aug 26, 2019 • edited Loading

andyross commented Aug 26, 2019

andyross commented Aug 28, 2019

andyross commented Sep 3, 2019

andyross commented Sep 4, 2019

andyross commented Sep 9, 2019

andyross commented Aug 6, 2019 •

edited

Loading

zephyrbot commented Aug 6, 2019 •

edited

Loading

andrewboie commented Aug 7, 2019 •

edited

Loading

jukkar Aug 14, 2019 •

edited

Loading

dbkinder Aug 16, 2019 •

edited

Loading

pizi-nordic commented Aug 26, 2019 •

edited

Loading