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.

Sign up for GitHub

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

Jump to bottom

Update(driver): Introduce the BPF commands name #1545

Merged
merged 5 commits into from
Feb 7, 2024
Merged

Update(driver): Introduce the BPF commands name #1545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da49823
update(bpf): introduce the BPF commands name
Rohith-Raju Dec 5, 2023
c16ba20
update: address review requests
Rohith-Raju Dec 14, 2023
38b40a4
update: use unsigned long
Rohith-Raju Dec 14, 2023
d98fb8a
update: made changes as per review request
Rohith-Raju Jan 10, 2024
aa9abc5
"Revert: made changes as per review request"
Rohith-Raju Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion driver/SCHEMA_VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2.16.0
2.17.0
6 changes: 3 additions & 3 deletions driver/bpf/fillers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5801,9 +5801,9 @@ FILLER(sys_bpf_x, true)
long fd = bpf_syscall_get_retval(data->ctx);
bpf_push_s64_to_ring(data, fd);

/* Parameter 2: cmd (type: PT_INT32) */
int32_t cmd = (int32_t)bpf_syscall_get_argument(data, 0);
return bpf_push_s32_to_ring(data, cmd);
/* Parameter 2: cmd (type: PT_ENUMFLAGS32) */
unsigned long cmd = bpf_syscall_get_argument(data, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

cmd is int why are we changing this? https://man7.org/linux/man-pages/man2/bpf.2.html

return bpf_push_s32_to_ring(data, (int32_t)bpf_cmd_to_scap(cmd));
}

FILLER(sys_unlinkat_x, true)
Expand Down
2 changes: 1 addition & 1 deletion driver/event_table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ const struct ppm_event_info g_event_info[] = {
[PPME_SYSCALL_DUP_1_E] = {"dup", EC_IO_OTHER | EC_SYSCALL, EF_CREATES_FD | EF_USES_FD | EF_MODIFIES_STATE, 1, {{"fd", PT_FD, PF_DEC} } },
[PPME_SYSCALL_DUP_1_X] = {"dup", EC_IO_OTHER | EC_SYSCALL, EF_CREATES_FD | EF_USES_FD | EF_MODIFIES_STATE, 2, {{"res", PT_FD, PF_DEC}, {"oldfd", PT_FD, PF_DEC} } },
[PPME_SYSCALL_BPF_2_E] = {"bpf", EC_OTHER | EC_SYSCALL, EF_CREATES_FD, 1, {{"cmd", PT_INT64, PF_DEC} } },
[PPME_SYSCALL_BPF_2_X] = {"bpf", EC_OTHER | EC_SYSCALL, EF_CREATES_FD, 2, { {"fd", PT_FD, PF_DEC}, {"cmd",PT_INT32, PF_DEC} } },
[PPME_SYSCALL_BPF_2_X] = {"bpf", EC_OTHER | EC_SYSCALL, EF_CREATES_FD, 2, { {"fd", PT_FD, PF_DEC}, {"cmd", PT_ENUMFLAGS32, PF_DEC, bpf_commands} } },
[PPME_SYSCALL_MLOCK2_E] = {"mlock2", EC_MEMORY | EC_SYSCALL, EF_NONE, 0},
[PPME_SYSCALL_MLOCK2_X] = {"mlock2", EC_MEMORY | EC_SYSCALL, EF_NONE, 4, {{"res", PT_ERRNO, PF_DEC}, {"addr", PT_UINT64, PF_HEX}, {"len", PT_UINT64, PF_DEC}, {"flags", PT_UINT32, PF_HEX, mlock2_flags}}},
[PPME_SYSCALL_FSCONFIG_E] = {"fsconfig", EC_SYSTEM | EC_SYSCALL, EF_NONE, 0},
Expand Down
40 changes: 40 additions & 0 deletions driver/flags_table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,3 +716,43 @@ const struct ppm_name_value mknod_mode[] = {
{0, 0},
};

const struct ppm_name_value bpf_commands[] = {
{"BPF_MAP_CREATE", PPM_BPF_MAP_CREATE},
{"BPF_MAP_LOOKUP_ELEM", PPM_BPF_MAP_LOOKUP_ELEM},
{"BPF_MAP_UPDATE_ELEM", PPM_BPF_MAP_UPDATE_ELEM},
{"BPF_MAP_DELETE_ELEM", PPM_BPF_MAP_DELETE_ELEM},
{"BPF_MAP_GET_NEXT_KEY", PPM_BPF_MAP_GET_NEXT_KEY},
{"BPF_PROG_LOAD", PPM_BPF_PROG_LOAD},
{"BPF_OBJ,PIN", PPM_BPF_OBJ_PIN},
{"BPF_OBJ_GET", PPM_BPF_OBJ_GET},
{"BPF_PROG_ATTACH", PPM_BPF_PROG_ATTACH},
{"BPF_PROG_DETACH", PPM_BPF_PROG_DETACH},
{"BPF_PROG_TEST_RUN", PPM_BPF_PROG_TEST_RUN},
{"BPF_PROG_RUN", PPM_BPF_PROG_RUN},
{"BPF_PROG_GET_NEXT_ID", PPM_BPF_PROG_GET_NEXT_ID},
{"BPF_MAP_GET_NEXT_ID", PPM_BPF_MAP_GET_NEXT_ID},
{"BPF_PROG_GET_FD_BY_ID", PPM_BPF_PROG_GET_FD_BY_ID},
{"BPF_MAP_GET_FD_BY_ID", PPM_BPF_MAP_GET_FD_BY_ID},
{"BPF_OBJ_GET_INFO_BY_FD", PPM_BPF_OBJ_GET_INFO_BY_FD},
{"BPF_PROG_QUERY", PPM_BPF_PROG_QUERY},
{"BPF_RAW_TRACEPOINT_OPEN", PPM_BPF_RAW_TRACEPOINT_OPEN},
{"BPF_BTF_LOAD", PPM_BPF_BTF_LOAD},
{"BPF_BTF_GET_FD_BY_ID", PPM_BPF_BTF_GET_FD_BY_ID},
{"BPF_TASK_FD_QUERY", PPM_BPF_TASK_FD_QUERY},
{"BPF_MAP_LOOKUP_AND_DELETE_ELEM", PPM_BPF_MAP_LOOKUP_AND_DELETE_ELEM},
{"BPF_MAP_FREEZE", PPM_BPF_MAP_FREEZE},
{"BPF_BTF_GET_NEXT_ID", PPM_BPF_BTF_GET_NEXT_ID},
{"BPF_MAP_LOOKUP_BATCH", PPM_BPF_MAP_LOOKUP_BATCH},
{"BPF_MAP_LOOKUP_AND_DELETE_BATCH", PPM_BPF_MAP_LOOKUP_AND_DELETE_BATCH},
{"BPF_MAP_UPDATE_BATCH", PPM_BPF_MAP_UPDATE_BATCH},
{"BPF_MAP_DELETE_BATCH", PPM_BPF_MAP_DELETE_BATCH},
{"BPF_LINK_CREATE", PPM_BPF_LINK_CREATE},
{"BPF_LINK_UPDATE", PPM_BPF_LINK_UPDATE},
{"BPF_LINK_GET_FD_BY_ID", PPM_BPF_LINK_GET_FD_BY_ID},
{"BPF_LINK_GET_NEXT_ID", PPM_BPF_LINK_GET_NEXT_ID},
{"BPF_ENABLE_STATS", PPM_BPF_ENABLE_STATS},
{"BPF_ITER_CREATE", PPM_BPF_ITER_CREATE},
{"BPF_LINK_DETACH", PPM_BPF_LINK_DETACH},
{"BPF_PROG_BIND_MAP", PPM_BPF_PROG_BIND_MAP},
{0,0},
};
4 changes: 2 additions & 2 deletions driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/bpf.bpf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ int BPF_PROG(bpf_x,
ringbuf__store_s64(&ringbuf, ret);

/* Parameter 2: cmd (type: PT_INT32) */
int32_t cmd = (int32_t)extract__syscall_argument(regs, 0);
ringbuf__store_s32(&ringbuf, cmd);
unsigned long cmd = extract__syscall_argument(regs, 0);
ringbuf__store_s32(&ringbuf,(int32_t)bpf_cmd_to_scap(cmd));


/*=============================== COLLECT PARAMETERS ===========================*/
Expand Down
43 changes: 42 additions & 1 deletion driver/ppm_events_public.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,47 @@ or GPL2.txt for full copies of the license.
#define PPM_MODULE_INIT_IGNORE_VERMAGIC 2
#define PPM_MODULE_INIT_COMPRESSED_FILE 4

/*
* bpf_commands
*/
#define PPM_BPF_MAP_CREATE 0
#define PPM_BPF_MAP_LOOKUP_ELEM 1
#define PPM_BPF_MAP_UPDATE_ELEM 2
#define PPM_BPF_MAP_DELETE_ELEM 3
#define PPM_BPF_MAP_GET_NEXT_KEY 4
#define PPM_BPF_PROG_LOAD 5
#define PPM_BPF_OBJ_PIN 6
#define PPM_BPF_OBJ_GET 7
#define PPM_BPF_PROG_ATTACH 8
#define PPM_BPF_PROG_DETACH 9
#define PPM_BPF_PROG_TEST_RUN 10
#define PPM_BPF_PROG_RUN PPM_BPF_PROG_TEST_RUN
#define PPM_BPF_PROG_GET_NEXT_ID 11
#define PPM_BPF_MAP_GET_NEXT_ID 12
#define PPM_BPF_PROG_GET_FD_BY_ID 13
#define PPM_BPF_MAP_GET_FD_BY_ID 14
#define PPM_BPF_OBJ_GET_INFO_BY_FD 15
#define PPM_BPF_PROG_QUERY 16
#define PPM_BPF_RAW_TRACEPOINT_OPEN 17
#define PPM_BPF_BTF_LOAD 18
#define PPM_BPF_BTF_GET_FD_BY_ID 19
#define PPM_BPF_TASK_FD_QUERY 20
#define PPM_BPF_MAP_LOOKUP_AND_DELETE_ELEM 21
#define PPM_BPF_MAP_FREEZE 22
#define PPM_BPF_BTF_GET_NEXT_ID 23
#define PPM_BPF_MAP_LOOKUP_BATCH 24
#define PPM_BPF_MAP_LOOKUP_AND_DELETE_BATCH 25
#define PPM_BPF_MAP_UPDATE_BATCH 26
#define PPM_BPF_MAP_DELETE_BATCH 27
#define PPM_BPF_LINK_CREATE 28
#define PPM_BPF_LINK_UPDATE 29
#define PPM_BPF_LINK_GET_FD_BY_ID 30
#define PPM_BPF_LINK_GET_NEXT_ID 31
#define PPM_BPF_ENABLE_STATS 32
#define PPM_BPF_ITER_CREATE 33
#define PPM_BPF_LINK_DETACH 34
#define PPM_BPF_PROG_BIND_MAP 35

/*
* Get/set the timerslack as used by poll/select/nanosleep
* A value of 0 means "use default"
Expand Down Expand Up @@ -2171,10 +2212,10 @@ extern const struct ppm_name_value fchownat_flags[];
extern const struct ppm_name_value prctl_options[];
extern const struct ppm_name_value memfd_create_flags[];
extern const struct ppm_name_value pidfd_open_flags[];
extern const struct ppm_name_value bpf_commands[];
extern const struct ppm_param_info sockopt_dynamic_param[];
extern const struct ppm_param_info ptrace_dynamic_param[];
extern const struct ppm_param_info bpf_dynamic_param[];

/*!
\brief Process information as returned by the PPM_IOCTL_GET_PROCLIST IOCTL.
*/
Expand Down
4 changes: 2 additions & 2 deletions driver/ppm_fillers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6733,7 +6733,7 @@ int f_sys_bpf_e(struct event_filler_arguments *args)
unsigned long val = 0;
syscall_get_arguments_deprecated(args, 0, 1, &val);

/* Parameter 1: cmd (type: PT_INT64) */
/* Parameter 1: cmd (type: PT_ENUMFLAGS32) */
cmd = (int32_t)val;
res = val_to_ring(args, (int64_t)cmd, 0, false, 0);
CHECK_RES(res);
Expand All @@ -6754,7 +6754,7 @@ int f_sys_bpf_x(struct event_filler_arguments *args)

/* Parameter 2: cmd (type: PT_INT64) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could update the comments throughout.

syscall_get_arguments_deprecated(args, 0, 1, &val);
cmd = (int32_t)val;
cmd = (int32_t)bpf_cmd_to_scap(val);
res = val_to_ring(args, cmd, 0, false, 0);
CHECK_RES(res);
return add_sentinel(args);
Expand Down
13 changes: 12 additions & 1 deletion driver/ppm_flag_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2222,4 +2222,15 @@ static __always_inline uint32_t mknod_mode_to_scap(uint32_t modes)
return res;
}

#endif /* PPM_FLAG_HELPERS_H_ */
static __always_inline uint32_t bpf_cmd_to_scap (unsigned long cmd){
Copy link
Contributor

Choose a reason for hiding this comment

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

This helper was already written as cmd was already an enum value; i'd just add the new switch cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can also do what eg: we did for fsconfig_cmds_to_scap, since bpf cmd are defined as uapi, they won't ever change (NEVER BREAK USER API rule by Torvalds itself :D), therefore a return cmd is enough here (if and only if our PPM_BPF_ values do perfectly match kernel BPF_ values!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this makes sense!!

Copy link
Contributor

Choose a reason for hiding this comment

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

I liked @FedeDP suggestion, but now I wonder why we even touch the driver code at all, and why we don't just do userspace conversions? Adding this statement to the userspace parsing would be nice!

/*
* bpf opcodes are defined via enum in uapi/linux/bpf.h.
* It is userspace API (thus stable) and arch-independent.
* Therefore we map them 1:1; if any unmapped flag arrives,
* we will just print its value to userspace without mapping it to a string flag.
* We then need to append new flags to both flags_table and ppm_events_public PPM_ flags.
*/

return cmd;
}
#endif /* PPM_FLAG_HELPERS_H_ */
10 changes: 5 additions & 5 deletions test/drivers/test_suites/syscall_exit_suite/bpf_x.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ TEST(SyscallExit, bpfX_MAP_CREATE)

/*=============================== TRIGGER SYSCALL ===========================*/

int32_t cmd = 1;
union bpf_attr *attr = NULL;

int32_t cmd = BPF_MAP_CREATE;
union bpf_attr *attr = NULL;

/* Here we need to call the `bpf` from a child because the main process throws lots of
* `bpf` syscalls to manage the bpf drivers.
Expand Down Expand Up @@ -147,8 +147,8 @@ TEST(SyscallExit, bpfX_MAP_CREATE)

/* Parameter 1: fd (type: PT_FD) */
evt_test->assert_numeric_param(1, errno_value);
/* Parameter 2: cmd (type: PT_INT32)*/
evt_test->assert_numeric_param(2, cmd);
/* Parameter 2: cmd (type: PT_ENUMFLAGS32)*/
evt_test->assert_numeric_param(2, PPM_BPF_MAP_CREATE);

/*=============================== ASSERT PARAMETERS ===========================*/

Expand Down
13 changes: 13 additions & 0 deletions userspace/libsinsp/test/filterchecks/evt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,17 @@ TEST_F(sinsp_with_test_input, EVT_FILTER_rawarg_str)
sinsp_evt* evt = add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_OPEN_E, 3, path.c_str(),
(uint32_t)0, (uint32_t)0);
ASSERT_EQ(get_field_as_string(evt, "evt.rawarg.name"), path);
}

TEST_F(sinsp_with_test_input, EVT_FILTER_cmd_str)
{
add_default_init_thread();

open_inspector();

int fd = 1;

sinsp_evt* evt = add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_BPF_2_X, 2, fd, (uint64_t)PPM_BPF_PROG_LOAD);
Copy link
Contributor Author

@Rohith-Raju Rohith-Raju Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Used uint64_t because emscripten build fails while using uint32_t

Copy link
Contributor

Choose a reason for hiding this comment

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

That is really weird :/ I don't like passing a wrong-sized parameter here since it is working fine until we add a parameter after this one, then everything breaks apart.

Can you share the build error?


ASSERT_EQ(get_field_as_string(evt, "evt.arg.cmd"), "BPF_PROG_LOAD");
}
Loading