Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFE: improve the internal syscall tables (gperf,CSV) #223

Closed
wants to merge 3 commits into from

Conversation

pcmoore
Copy link
Member

@pcmoore pcmoore commented Mar 22, 2020

This PR is based heavily on the work of @giuseppe and PR #204. I suggest reading through that PR first before looking at these patches.

In addition to splitting the initial monolithic patch into three (add the syscall CSV table, add gperf support, remove the old syscall tables), a number of other changes were made:

  • Various style tweaks
  • .gitignore fixes
  • Dropped the arch-syscall-validate changes as they were masking other problems
  • Fixed the x86, x32, ARM, all the MIPS ABIs, s390, and s390x ABIs as the syscall offsets were not properly incorporated into this change
  • Cleaned up the ABI specific headers
  • Cleaned up generate_syscalls_perf.sh and renamed to arch-gperf-generate
  • Fixed problems with automake's file packaging

pcmoore and others added 3 commits March 22, 2020 15:16
Later patches will make use of this new syscall table format instead
of the manually maintained tables.

The new CSV syscall table was generated with the following command:

  # ./arch-syscall-validate -c <kernel_source_dir> > syscalls.csv

Signed-off-by: Paul Moore <paul@paul-moore.com>
This patch significantly improves the performance of
seccomp_syscall_resolve_name since it replaces the expensive strcmp
for each syscall in the database, with a lookup table.

The complexity for 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.

PM: After talking with Giuseppe I made a number of additional
changes, some substantial, the highlights include:
* various style tweaks
* .gitignore fixes
* fixed subject line, tweaked the description
* dropped the arch-syscall-validate changes as they were masking
  other problems
* extracted the syscalls.csv and file deletions to other patches
  to keep this one more focused
* fixed the x86, x32, arm, all the MIPS ABIs, s390, and s390x ABIs as
  the syscall offsets were not properly incorporated into this change
* cleaned up the ABI specific headers
* cleaned up generate_syscalls_perf.sh and renamed to
  arch-gperf-generate
* fixed problems with automake's file packaging

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Reviewed-by: Tom Hromatka <tom.hromatka@oracle.com>
[PM: see notes in the "PM" section above]
Signed-off-by: Paul Moore <paul@paul-moore.com>
Since the move to gperf and the automatically generated syscall table
in CSV format, these manually maintained tables are no longer needed.

Signed-off-by: Paul Moore <paul@paul-moore.com>
@pcmoore
Copy link
Member Author

pcmoore commented Mar 22, 2020

@giuseppe I hope this looks okay to you. I left your name on the middle/gperf patch as I wanted you to get credit for all your work on this, but if you have a problem with that please let me know.

Copy link
Contributor

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcmoore thanks for the changes

LGTM

@drakenclimber
Copy link
Member

The cleanup looks really good, @pcmoore . I like the idea of using arch-syscall-validate to generate the csv file. Good call.

For all of the patches:
Reviewed-by: Tom Hromatka <tom.hromatka@gmail.com>

@pcmoore
Copy link
Member Author

pcmoore commented Mar 23, 2020

@pcmoore thanks for the changes

LGTM

Thanks for all of you work @giuseppe! All I did was clean it up a bit and smooth out some rough edges.

@pcmoore
Copy link
Member Author

pcmoore commented Mar 23, 2020

The cleanup looks really good, @pcmoore .

Thanks for the review.

I like the idea of using arch-syscall-validate to generate the csv file. Good call.

Yes, this should make updating the syscall tables a trivial task. We'll should be able to do much more frequent stable updates in the v2.5.x branch to catch new syscalls.

@pcmoore
Copy link
Member Author

pcmoore commented Mar 23, 2020

Merged at HEAD 4bd8de3.

@giuseppe
Copy link
Contributor

giuseppe commented Jul 3, 2020

@pcmoore there are a few changes (like the current PR), that would be nice to have in a release. Is there any plan for cutting a new one?

@pcmoore
Copy link
Member Author

pcmoore commented Jul 4, 2020

We track our release progress using milestones, you are most likely interested in v2.5.0:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: move arch/ABI syscall mapping tables into a common location
3 participants