-
Notifications
You must be signed in to change notification settings - Fork 165
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
new(driver,userspace): automatically generate syscall_info_table entries at startup time #649
Conversation
@@ -1197,344 +1197,375 @@ enum extra_event_prog_code | |||
/* | |||
* System-independent syscall codes | |||
*/ | |||
|
|||
#define PPM_SC_FIELDS \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use an X_MACRO to be able to automatically generate syscall names from PPM_SC code name, while filling the syscall_info_table.
extern const struct ppm_event_info g_event_info[PPM_EVENT_MAX]; | ||
extern const struct syscall_evt_pair g_syscall_table[SYSCALL_TABLE_SIZE]; | ||
|
||
static const struct ppm_syscall_desc *g_syscall_info_table; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleanup: use static
where it makes sense.
@@ -986,6 +987,8 @@ int main(int argc, char** argv) | |||
return EXIT_FAILURE; | |||
} | |||
|
|||
g_syscall_info_table = scap_get_syscall_info_table(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fill our syscall_info_table from data gathered by libscap, triggering syscall_info_table filling.
@@ -522,8 +522,7 @@ typedef struct scap_dumper scap_dumper_t; | |||
*/ | |||
struct ppm_syscall_desc { | |||
enum ppm_event_category category; /**< System call category. */ | |||
enum ppm_event_flags flags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flags where unused.
@@ -522,8 +522,7 @@ typedef struct scap_dumper scap_dumper_t; | |||
*/ | |||
struct ppm_syscall_desc { | |||
enum ppm_event_category category; /**< System call category. */ | |||
enum ppm_event_flags flags; | |||
char *name; /**< System call name, e.g. 'open'. */ | |||
char name[PPM_MAX_NAME_LEN]; /**< System call name, e.g. 'open'. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to do some small trickery to obtain a lowercase string, thus the need to make this a char array.
TODO:
|
94678ba
to
8732451
Compare
Rebased on top of master. |
@@ -683,13 +683,13 @@ const struct syscall_evt_pair g_syscall_table[SYSCALL_TABLE_SIZE] = { | |||
[__NR_shmat - SYSCALL_TABLE_ID0] = {.ppm_sc = PPM_SC_SHMAT}, | |||
#endif | |||
#ifdef __NR_rt_sigreturn | |||
[__NR_rt_sigreturn - SYSCALL_TABLE_ID0] = {.ppm_sc = PPM_SC_SIGRETURN}, | |||
[__NR_rt_sigreturn - SYSCALL_TABLE_ID0] = {.ppm_sc = PPM_SC_RT_SIGRETURN}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to match real syscall name.
#endif | ||
#ifdef __NR_fallocate | ||
[__NR_fallocate - SYSCALL_TABLE_ID0] = {.ppm_sc = PPM_SC_FALLOCATE}, | ||
#endif | ||
#ifdef __NR_newfstatat | ||
[__NR_newfstatat - SYSCALL_TABLE_ID0] = {.ppm_sc = PPM_SC_NEWFSSTAT}, | ||
[__NR_newfstatat - SYSCALL_TABLE_ID0] = {.ppm_sc = PPM_SC_NEWFSTATAT}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to match real syscall name.
@@ -159,7 +159,7 @@ void sinsp_parser::process_event(sinsp_evt *evt) | |||
{ | |||
sinsp_evt_param *parinfo = evt->get_param(0); | |||
uint16_t evid = *(uint16_t *)parinfo->m_val; | |||
flags = g_infotables.m_syscall_info_table[evid].flags; | |||
flags = EF_NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All flags were already EF_NONE anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the kind of change that is small enough to be ignored, yet can bring headaches to other people in the future. Could you elaborate on why do we need this change? If the only reason is the one you already provided, I would roll this back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we do not need flags in syscall info table; when they were present (and they are not present since some months now) they were just using the EF_DROP_SIMPLE_CONS
one, that is now useless.
I think it creates more tech debt than its provided value (that is near 0 btw :D )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then IMO the flags should be removed entirely and not silently escaped in a single place, this way we still have the flags field floating around, but it is not doing anything and can lead people to believing it is still being used. I would roll this back and open a PR properly removing flags entirely, would that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh there is no flags
field anymore:
struct ppm_syscall_desc {
enum ppm_event_category category; /**< System call category. */
char name[PPM_MAX_NAME_LEN]; /**< System call name, e.g. 'open'. */
};
I think i am misunderstanding something here :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that, it's still early for me, guess I need more coffee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahahahah no problem my man! ☕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how about initializing the variable when declared in line 156 and removing this line entirely? That would make it really explicit that flags are no longer used in the case of generic events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's true, but at the same time it would be less explicit about handling the special generic
case. I mean, if 2 months from now one decides to remove the initialization, we would end up reading garbage in case of generic events.
I don't know, it's a tradeoff :)
@@ -50,11 +50,20 @@ std::string get_event_category_name(ppm_event_category category) | |||
// | |||
// Get the string representation of a ppm_event_type | |||
// | |||
std::string get_event_type_name(uint16_t type) | |||
std::string get_event_type_name(sinsp& inspector, sinsp_evt* ev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use same logic used in evt.type
filtercheck!
You're welcome Mauro 💯 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This is a huge step forward!
/hold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
…ies at startup time. We use a lazy generation, ie: first time `scap_get_syscall_info_table` is called, we fill the table. The table is filled with correct names; the category is either fetched from the event_table, or EC_UNKNOWN (for syscalls that have no event attached, and use the generic one). Moreover, added generic event support for falcosecurity/falco#1998 syscalls; they won't use any specific filler, just the automatic generic one, and there is no even mapping for them. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…ting evt.type in capture mode. If a ppm_sc is unknown, for generic enter events, try to use real syscall nr to fetch correct ppm_sc. This allows forward compatibility of scap files: even if scap file has some unknown events at creation time, if proper syscalls are added afterwards, their name will appear. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…to also use syscall_info_table. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
… name. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Co-authored-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com> Co-authored-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com> Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
6793c9c
to
a0efa5f
Compare
Rebased on top of master @Andreagit97 @Molter73 ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 7cdb2bed79147e4d95e89d099709fd5ac31d070e
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, leogr, Molter73 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
What type of PR is this?
/kind cleanup
/kind feature
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libscap
/area libsinsp
Does this PR require a change in the driver versions?
I don't think so since we are using the automatic generic filler / event.
What this PR does / why we need it:
We use a lazy generation, ie: first time
scap_get_syscall_info_table
is called, we fill the table.The table is filled with correct names; the category is either fetched from the event_table, or EC_UNKNOWN (for syscalls that have no event attached, and use the generic one).
Moreover, added generic event support for falcosecurity/falco#1998 syscalls; they won't use any specific filler, just the automatic generic one, and there is no even mapping for them.
Only downside: we lost the ability to mark "generic" syscalls with a proper category. I don't think it is a huge downside, yet i want to highlight it.
Which issue(s) this PR fixes:
We add string-support (through generic event) for all syscalls listed here: falcosecurity/falco#1998
A proper specific filler where needed, will be introduced in subsequent PRs.
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: