syscalls: use gperf for lookups

[giuseppe]

use gperf to generate a perfect hash to lookup syscall names. It
improves significantly the complexity for seccomp_syscall_resolve_name.*
since it replaces the expensive strcmp for each syscall in the
database, with a lookup table.

The complexity for x86_64_syscall_resolve_num is not changed and it
uses the linear search, that is anyway less expensive than
seccomp_syscall_resolve_name.* as it uses an index for comparison
instead of doing a string comparison.

On my machine, calling 1000 seccomp_syscall_resolve_name_arch and
seccomp_syscall_resolve_num_arch over the entire syscalls DB passed
from ~0.45 sec to ~0.06s.

I've implemented it only for x86_64 as I've no access to other archs
for benchmarking the results.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

[coveralls]

Coverage increased (+2.1%) to 92.392% when pulling 9e7214a on giuseppe:gperf into dc2831e on seccomp:master.

[giuseppe]

@pcmoore could you please take a look?

[drakenclimber]

Seems like this could be a good improvement for applications that are quickly creating/deleting containers with seccomp filters on them. Nice work.

[pcmoore]

@pcmoore could you please take a look?

Hi @giuseppe, thanks for you work on this. I think this goes well with some other work I wanted to get done with libseccomp, but as I'm looking for the associated issue I realize that I never created one; the only reference I had outside my head was a discussion with @drakenclimber one day at lunch :/

Let me create that issue today and then we can talk further.

[pcmoore]

Hi @giuseppe, thanks for you work on this. I think this goes well with some other work I wanted to get done with libseccomp, but as I'm looking for the associated issue I realize that I never created one; the only reference I had outside my head was a discussion with @drakenclimber one day at lunch :/

Let me create that issue today and then we can talk further.

Okay, take a look at #207 and let me know what you think.

[pcmoore]

Also, if someone wants to work on this and #207, I think we could consider this for the v2.5 milestone/release.

(This would make @drakenclimber and my lives much easier from a maintenance perspective, so anyone willing to work on this get's a large number of gold stars!)

[giuseppe]

Okay, take a look at #207 and let me know what you think.

thanks for writing this up. I think it makes a lot of sense to not spread the same information among different files.

Would you be ok with having the .perf file as the single source instead of a .csv file?

It will be really easy to generalize the x86_64 code to handle multiple archs. We will need to add a column for each arch, in the same way as you've proposed for the .csv file, so it will look more like:

+struct arch_syscall_def_internal { int name; int x86_64; int x86; ...; int index; };

We still need the index column as gperf doesn't keep the same ordering we provide.

If you prefer the csv file then we would need to generate the .perf file based on that.

The main disadvantage with using directly gperf is that the lookup by syscall number is still O(n) as we need to scan the entire table, although we are comparing a single int so it is significantly faster than strcmp.
Instead, if we'll use a .csv file we could easily build an additional lookup array for each arch so that the syscall number -> name can also be O(1)

[pcmoore]

Would you be ok with having the .perf file as the single source instead of a .csv file?

Well, a CSV file easy to manipulate with any number of tools, the *.perf file format looks a bit more convoluted. Besides ...

The main disadvantage with using directly gperf is that the lookup by syscall number is still O(n) as we need to scan the entire table, although we are comparing a single int so it is significantly faster than strcmp.
Instead, if we'll use a .csv file we could easily build an additional lookup array for each arch so that the syscall number -> name can also be O(1).

Is using the *.perf file format as the source even desirable given what you've written above?

The CSV file format is tool agnostic and gives us plenty of options for other "stuff", including this gperf optimization. As of right now I think the CSV source format is our best option of the two.

[giuseppe]

Is using the *.perf file format as the source even desirable given what you've written above?

The CSV file format is tool agnostic and gives us plenty of options for other "stuff", including this gperf optimization. As of right now I think the CSV source format is our best option of the two.

I was not sure as I didn't want to add yet another level of source generation.

I went forward and added a .csv file. The perf file is generated from there. I've also adapted the code for arm, as an example of how it is possible to move existing archs to gperf.

[pcmoore]

I'm going to preface this by saying I know almost nothing about gperf at the moment, but it would be really nice if we could limit the amount of C code in the "syscalls.perf.template" file to close-to-nothing. It looks like we would need XXX_syscall_resolve_name, XXX_syscall_resolve_num, and XXX_syscall_iterate and that is it, right?

I would also like to see the "syscalls.csv to syscalls.perf" generation commands encapsulated into a separate script instead of open-coded into the makefile.

It would also be nice to drop the quotes around the syscall names in the CSV file. Let's also convert the "__PNR_xxx" references into something generic like just "PNR"; the conversion script can change that into the appropriate "__PNR_xxx" value. Further, can we get rid of the explicit offsets in the CSV, e.g. "(__SCMP_NR_BASE + 140)"? It seems like the conversion script could always assume an offset and generate the "(__SCMP_NR_BASE + xxx)" C code and it would be up to the ABI definition to define the offset; for ABIs like x86 that offset could be zero.

[pcmoore]

Side note: you also need to find a way to get your code coverage numbers back up. It might just be a symptom of the PR being a work-in-progress, but we've worked way too hard to have it the coverage numbers drop down into the 60% range.

[giuseppe]

I'm going to preface this by saying I know almost nothing about gperf at the moment, but it would be really nice if we could limit the amount of C code in the "syscalls.perf.template" file to close-to-nothing. It looks like we would need XXX_syscall_resolve_name, XXX_syscall_resolve_num, and XXX_syscall_iterate and that is it, right?

I would also like to see the "syscalls.csv to syscalls.perf" generation commands encapsulated into a separate script instead of open-coded into the makefile.

It would also be nice to drop the quotes around the syscall names in the CSV file. Let's also convert the "__PNR_xxx" references into something generic like just "PNR"; the conversion script can change that into the appropriate "__PNR_xxx" value. Further, can we get rid of the explicit offsets in the CSV, e.g. "(__SCMP_NR_BASE + 140)"? It seems like the conversion script could always assume an offset and generate the "(__SCMP_NR_BASE + xxx)" C code and it would be up to the ABI definition to define the offset; for ABIs like x86 that offset could be zero.

can we do these cleanups for the .csv file incrementally? I've taken care of the other comments, and also the coverage seems to be again ok now.

[pcmoore]

can we do these cleanups for the .csv file incrementally?

I would like to see them all in the same PR. The same goes for supporting all the arch/ABIs.

I'm not a big fan of incomplete work where there is just a promise to do it later ;)

[giuseppe]

I'm not a big fan of incomplete work where there is just a promise to do it later ;)

I wasn't even promising that :-) I thought it was fine to just add it for x86_64 and other arches could be adapted later by whoever cares about them.

Can we just use the existing values to populate the csv for now? I am not exactly sure how you'd like to rewrite them an it seems like an extra step that could be done separately.

Some arches, like s390, have some extra logic in $ARCH_syscall_resolve_num, and there are a series of if (num == VALUE) before iterating the table. Is it just an optimization for the most used syscalls or is it something else?

[giuseppe]

I've pushed a new version where I've adapted all the architectures

[pcmoore]

Thanks for renaming test 56 :)

[drakenclimber]

I had a couple small comments and questions, but I think the code looks really good. I'm excited for the performance improvements.

With this large of a change, @pcmoore and I will have to be extra diligent when we validate the syscalls for the next kernel release, but that doesn't seem tremendously onerous.

[rhatdan]

@giuseppe @pcmoore @drakenclimber Is this ready to merge? Would love to get this merged and a new release out, so we could take advantage.

[drakenclimber]

Awesome! Thanks for all the hard work @giuseppe

Reviewed-by: Tom Hromatka <tom.hromatka@oracle.com>

[giuseppe]

@pcmoore does the last version look good to you?

[pcmoore]

Hi @giuseppe, can you explain the changes to src/arch-syscall-validate?

[pcmoore]

Hi @giuseppe, can you explain the changes to src/arch-syscall-validate?

Actually nevermind; I think I know what you were doing and that is just hiding some underlying problems with these changes and the syscalls table. Let me fixup arch-syscall-validate properly so we can use it to generate the CSV from the kernel sources. We are going to need to do that anyway, and it should avoid having to do what you did to hide problems in the CSV.

[pcmoore]

@giuseppe @drakenclimber I think this is what we need for arch-syscall-validate:

• https://gist.github.com/pcmoore/c33e64237798f05b3ecafca19c5221e5

It's really crude but it works (hint: run it with the '-c' option and point it at a recent kernel source tree):

PS1> ./arch-syscall-validate -c ~/sources/linux/linux-linus
# libseccomp syscall table
#
# kernel: 5.6.0-rc6 (Fri, 20 Mar 2020 03:49:05 +0000)
#
#syscall,x86,x86_64,x32,arm,aarch64,mips,mips64,mips64n32,ppc,ppc64,riscv64,s390,s390x
accept,PNR,43,43,285,202,168,42,42,330,330,202,PNR,PNR
accept4,364,288,288,366,242,334,293,297,344,344,242,364,364
access,33,21,21,33,PNR,33,20,20,33,33,PNR,33,33
acct,51,163,163,51,89,51,158,158,51,51,89,51,51
add_key,286,248,248,309,217,280,239,243,269,269,217,278,278
adjtimex,124,159,159,124,171,124,154,154,124,124,171,124,124
...

@giuseppe if that looks okay to you, you can include that in your patchset as a second patch or I can add it when I merge your patch; either approach is fine with me.

@drakenclimber as you mentioned previously, we are going to really need to verify that we haven't screwed up the syscall tables with this change. Thankfully this should be a one-time event, moving forward we can just generate a new CSV file for each release (and give the diff a quick sanity check of course).

[giuseppe]

Hi @giuseppe, can you explain the changes to src/arch-syscall-validate?

Actually nevermind; I think I know what you were doing and that is just hiding some underlying problems with these changes and the syscalls table.

I think the issue was in the previous implementation for some archs.

For example s390 had:

int s390_syscall_resolve_name(const char *name)
{
       unsigned int iter;
       const struct arch_syscall_def *table = s390_syscall_table;

       /* XXX - plenty of room for future improvement here */

       if (strcmp(name, "accept") == 0)
               return __PNR_accept;
       if (strcmp(name, "accept4") == 0)
               return __PNR_accept4;
       else if (strcmp(name, "bind") == 0)
               return __PNR_bind;
       else if (strcmp(name, "connect") == 0)
               return __PNR_connect;
       ....

so the value returned by *_resolve_name was different than the value stored into the syscalls table. With the new implementation there is not such per-arch customization and the value returned by *_resolve_name is the same as the value stored in the table.

src/arch-syscall-validate reads directly into the syscalls table and doesn't check for the real value returned by _resolve_name.

[giuseppe]

@giuseppe if that looks okay to you, you can include that in your patchset as a second patch or I can add it when I merge your patch; either approach is fine with me.

@pcmoore I am fine with including your patch in the PR, but I think we still need the filters I've added initially to arch-syscall-validate for the comment I've left previously.

Are you fine with something like:

diff --git a/src/arch-syscall-validate b/src/arch-syscall-validate
index 1a508bf..4c21653 100755
--- a/src/arch-syscall-validate
+++ b/src/arch-syscall-validate
@@ -664,15 +664,21 @@ function gen_csv() {
        done
        printf "\n"
        # output the syscall csv details
+       for abi in $abi_list; do
+               dump_sys_$abi $1 > sys_$abi.list
+       done
        for sc in $sc_list; do
                printf "%s" $sc
                for abi in $abi_list; do
-                       num=$(dump_sys_$abi "$1" | grep "^$sc," | awk -F "," '{ print $2 }' )
+                       num=$(cat sys_$abi.list | grep "^$sc," | awk -F "," '{ print $2 }' )
                        [[ -z $num ]] && num="PNR"
                        printf ",%s" $num
                done
                printf "\n"
        done
+       for abi in $abi_list; do
+               rm sys_$abi.list
+       done
 }

on top of your patch?

With the patch above arch-syscall-validate -c takes ~14 seconds on my system, which I think is acceptable.

[pcmoore]

I think the issue was in the previous implementation for some archs.

Sorta, there is an issue in s390 and s390x where we aren't handling the syscall lookup correctly, see the discussion in #215 for more information.

This of course is a reminder that we really need to be able to specify ABI specific lookup functions, e.g. x86_syscall_resolve_name() and x86_syscall_resolve_num(). The default raw hash table lookup function should work for most of the ABIs, but we will need special handling for some like x86, s390, and s390x.

@giuseppe do you mind if I spend some time hacking on your patch/PR? I'm not sure what my weekend is looking like (COVID-19 has introduced a lot of uncertainty in the world), but I may have some time to work on this if that is okay with you. Of course if you would prefer to fix it yourself, please do!

[pcmoore]

@pcmoore I am fine with including your patch in the PR, but I think we still need the filters I've added ...

See my previous comment, those filters are hiding a problem; we do not want them.

As I was reworking that patch (you had the right idea with only dumping the ABI lists once, but we really should use mktemp files like we do in other places) I realized that there is no reason we couldn't merge this now, independent of this PR. I'm going to do a test build and if all looks good I'll merge it "soon".

@drakenclimber I'm going to be forgiveness on this one and merge the arch-syscall-validate without the proper two ACKs. My reasoning is that we are the only ones that use this tool (and it is potentially blocking this PR) so if I mess it up too badly the pain will be limited :)

[giuseppe]

@giuseppe do you mind if I spend some time hacking on your patch/PR? I'm not sure what my weekend is looking like (COVID-19 has introduced a lot of uncertainty in the world), but I may have some time to work on this if that is okay with you. Of course if you would prefer to fix it yourself, please do!

of course I won't mind that :)

[pcmoore]

Okay, please check out PR #223, that has my changes incorporated with @giuseppe's changes.

[giuseppe]

closing in favor of #223