From abed271ceeba6fed452c4637db2729d4080356d0 Mon Sep 17 00:00:00 2001 From: Alan Maguire Date: Mon, 10 Jul 2023 17:40:03 +0100 Subject: [PATCH 1/2] libbpftune, tuners: propagate errors back for bpftune_bpf_[open,load,attach,init] we need to know if a phase fails as driving on in the face of failure can lead to segmentation faults. Signed-off-by: Alan Maguire --- include/bpftune/libbpftune.h | 61 ++++++++++++++++++++++-------------- src/neigh_table_tuner.c | 5 ++- src/net_buffer_tuner.c | 13 ++++++-- src/netns_tuner.c | 13 ++++++-- src/route_table_tuner.c | 7 +++-- src/sysctl_tuner.c | 4 ++- src/tcp_buffer_tuner.c | 13 ++++++-- src/tcp_cong_tuner.c | 4 ++- 8 files changed, 81 insertions(+), 39 deletions(-) diff --git a/include/bpftune/libbpftune.h b/include/bpftune/libbpftune.h index 1507164..7aeb05e 100644 --- a/include/bpftune/libbpftune.h +++ b/include/bpftune/libbpftune.h @@ -135,13 +135,14 @@ void bpftuner_tunables_fini(struct bpftuner *tuner); /* need a macro in order to generate code for skeleton-specific struct */ -#define bpftuner_bpf_open(tuner_name, tuner) \ +#define bpftuner_bpf_open(tuner_name, tuner) ({ \ + int __err = 0; \ do { \ struct tuner_name##_tuner_bpf *__skel; \ struct tuner_name##_tuner_bpf_legacy *__lskel; \ - int __err = bpftune_cap_add(); \ - \ - if (__err) return __err; \ + \ + __err = bpftune_cap_add(); \ + if (__err) break; \ tuner->name = #tuner_name; \ tuner->bpf_legacy = bpftuner_bpf_legacy(); \ if (!tuner->bpf_legacy) { \ @@ -168,9 +169,11 @@ void bpftuner_tunables_fini(struct bpftuner *tuner); if (__err) { \ bpftune_log_bpf_err(__err, \ #tuner_name " open bpf: %s\n"); \ - return __err; \ + break; \ } \ - } while (0) + } while (0); \ + __err; \ + }) #define bpftuner_bpf_destroy(tuner_name, tuner) \ do { \ @@ -178,51 +181,61 @@ void bpftuner_tunables_fini(struct bpftuner *tuner); tuner_name##_tuner_bpf__destroy(tuner->skel); \ else \ tuner_name##_tuner_bpf_legacy__destroy(tuner->skel); \ + tuner->skel = NULL; \ } while (0) -#define _bpftuner_bpf_load(tuner_name, tuner, optionals) \ +#define _bpftuner_bpf_load(tuner_name, tuner, optionals) ({ \ + int __err = 0; \ do { \ struct tuner_name##_tuner_bpf *__skel = tuner->skel; \ struct tuner_name##_tuner_bpf_legacy *__lskel = tuner->skel; \ - int __err; \ \ __err = __bpftuner_bpf_load(tuner, optionals); \ if (__err) { \ bpftuner_bpf_destroy(tuner_name, tuner); \ - return __err; \ + break; \ } \ if (!tuner->bpf_legacy) \ __skel->bss->tuner_id = bpftune_tuner_num(); \ else \ __lskel->bss->tuner_id = bpftune_tuner_num(); \ - } while (0) + } while (0); \ + __err; \ + }) #define bpftuner_bpf_load(tuner_name, tuner) \ _bpftuner_bpf_load(tuner_name, tuner, NULL) -#define bpftuner_bpf_attach(tuner_name, tuner, optionals) \ +#define bpftuner_bpf_attach(tuner_name, tuner, optionals) ({ \ + int __err = 0; \ do { \ - int __err; \ - \ __err = __bpftuner_bpf_attach(tuner); \ if (__err && optionals != NULL) { \ bpftuner_bpf_fini(tuner); \ - bpftuner_bpf_open(tuner_name, tuner); \ - _bpftuner_bpf_load(tuner_name, tuner, optionals); \ + __err = bpftuner_bpf_open(tuner_name, tuner); \ + if (!__err) \ + __err = _bpftuner_bpf_load(tuner_name, tuner, \ + optionals); \ + if (!__err) \ __err = __bpftuner_bpf_attach(tuner); \ } \ - if (__err) { \ + if (__err) \ bpftuner_bpf_destroy(tuner_name, tuner); \ - return __err; \ - } \ - } while (0) + } while (0); \ + __err; \ + }) -#define bpftuner_bpf_init(tuner_name, tuner, optionals) \ +#define bpftuner_bpf_init(tuner_name, tuner, optionals) ({ \ + int __err = 0; \ do { \ - bpftuner_bpf_open(tuner_name, tuner); \ - bpftuner_bpf_load(tuner_name, tuner); \ - bpftuner_bpf_attach(tuner_name, tuner, optionals); \ - } while (0) + __err = bpftuner_bpf_open(tuner_name, tuner); \ + if (!__err) \ + __err = bpftuner_bpf_load(tuner_name, tuner); \ + if (!__err) \ + __err = bpftuner_bpf_attach(tuner_name, tuner, optionals); \ + } while (0); \ + __err; \ + }) #define bpftuner_bpf_var_set(tuner_name, tuner, var, val) \ diff --git a/src/neigh_table_tuner.c b/src/neigh_table_tuner.c index fd47520..c6424d7 100644 --- a/src/neigh_table_tuner.c +++ b/src/neigh_table_tuner.c @@ -57,7 +57,10 @@ static struct bpftunable_scenario scenarios[] = { int init(struct bpftuner *tuner) { - bpftuner_bpf_init(neigh_table, tuner, NULL); + int err = bpftuner_bpf_init(neigh_table, tuner, NULL); + + if (err) + return err; return bpftuner_tunables_init(tuner, ARRAY_SIZE(descs), descs, ARRAY_SIZE(scenarios), scenarios); } diff --git a/src/net_buffer_tuner.c b/src/net_buffer_tuner.c index 6b2052c..cbc1c94 100644 --- a/src/net_buffer_tuner.c +++ b/src/net_buffer_tuner.c @@ -28,14 +28,21 @@ static struct bpftunable_scenario scenarios[] = { int init(struct bpftuner *tuner) { long cpu_bitmap = 0; + int err; bpftune_sysctl_read(0, "net.core.flow_limit_cpu_bitmap", &cpu_bitmap); - bpftuner_bpf_open(net_buffer, tuner); - bpftuner_bpf_load(net_buffer, tuner); + err = bpftuner_bpf_open(net_buffer, tuner); + if (err) + return err; + err = bpftuner_bpf_load(net_buffer, tuner); + if (err) + return err; bpftuner_bpf_var_set(net_buffer, tuner, flow_limit_cpu_bitmap, cpu_bitmap); - bpftuner_bpf_attach(net_buffer, tuner, NULL); + err = bpftuner_bpf_attach(net_buffer, tuner, NULL); + if (err) + return err; return bpftuner_tunables_init(tuner, NET_BUFFER_NUM_TUNABLES, descs, ARRAY_SIZE(scenarios), scenarios); diff --git a/src/netns_tuner.c b/src/netns_tuner.c index 4dc5b7a..235f1f1 100644 --- a/src/netns_tuner.c +++ b/src/netns_tuner.c @@ -39,13 +39,20 @@ static struct bpftunable_scenario scenarios[] = { int init(struct bpftuner *tuner) { const char *optionals[] = { "entry__net_free", NULL }; + int err; if (!bpftune_netns_cookie_supported()) return -ENOTSUP; - bpftuner_bpf_open(netns, tuner); - bpftuner_bpf_load(netns, tuner); - bpftuner_bpf_attach(netns, tuner, optionals); + err = bpftuner_bpf_open(netns, tuner); + if (err) + return err; + err = bpftuner_bpf_load(netns, tuner); + if (err) + return err; + err = bpftuner_bpf_attach(netns, tuner, optionals); + if (err) + return err; return bpftuner_tunables_init(tuner, ARRAY_SIZE(descs), descs, ARRAY_SIZE(scenarios), scenarios); diff --git a/src/route_table_tuner.c b/src/route_table_tuner.c index c8b8eb2..97ce798 100644 --- a/src/route_table_tuner.c +++ b/src/route_table_tuner.c @@ -37,9 +37,10 @@ static struct bpftunable_scenario scenarios[] = { int init(struct bpftuner *tuner) { - bpftuner_bpf_open(route_table, tuner); - bpftuner_bpf_load(route_table, tuner); - bpftuner_bpf_attach(route_table, tuner, NULL); + int err = bpftuner_bpf_init(route_table, tuner, NULL); + + if (err) + return err; return bpftuner_tunables_init(tuner, ARRAY_SIZE(descs), descs, ARRAY_SIZE(scenarios), scenarios); } diff --git a/src/sysctl_tuner.c b/src/sysctl_tuner.c index 59d7dbc..c2deb3c 100644 --- a/src/sysctl_tuner.c +++ b/src/sysctl_tuner.c @@ -33,8 +33,10 @@ extern unsigned short learning_rate; int init(struct bpftuner *tuner) { - bpftuner_bpf_init(sysctl, tuner, NULL); + int err = bpftuner_bpf_init(sysctl, tuner, NULL); + if (err) + return err; /* attach to root cgroup */ if (bpftuner_cgroup_attach(tuner, "sysctl_write", BPF_CGROUP_SYSCTL)) return 1; diff --git a/src/tcp_buffer_tuner.c b/src/tcp_buffer_tuner.c index 76fe980..1e071c5 100644 --- a/src/tcp_buffer_tuner.c +++ b/src/tcp_buffer_tuner.c @@ -139,9 +139,14 @@ long nr_free_buffer_pages(bool initial) int init(struct bpftuner *tuner) { int pagesize; + int err; - bpftuner_bpf_open(tcp_buffer, tuner); - bpftuner_bpf_load(tcp_buffer, tuner); + err = bpftuner_bpf_open(tcp_buffer, tuner); + if (err) + return err; + err = bpftuner_bpf_load(tcp_buffer, tuner); + if (err) + return err; pagesize = sysconf(_SC_PAGESIZE); if (pagesize < 0) @@ -154,7 +159,9 @@ int init(struct bpftuner *tuner) ilog2(SK_MEM_QUANTUM)); bpftuner_bpf_var_set(tcp_buffer, tuner, nr_free_buffer_pages, nr_free_buffer_pages(true)); - bpftuner_bpf_attach(tcp_buffer, tuner, NULL); + err = bpftuner_bpf_attach(tcp_buffer, tuner, NULL); + if (err) + return err; return bpftuner_tunables_init(tuner, TCP_BUFFER_NUM_TUNABLES, descs, ARRAY_SIZE(scenarios), scenarios); } diff --git a/src/tcp_cong_tuner.c b/src/tcp_cong_tuner.c index 9be3566..fe66efe 100644 --- a/src/tcp_cong_tuner.c +++ b/src/tcp_cong_tuner.c @@ -55,7 +55,9 @@ int init(struct bpftuner *tuner) bpftune_log(LOG_DEBUG, "could not load tcp_bbr module: %s\n", strerror(-err)); - bpftuner_bpf_init(tcp_cong, tuner, NULL); + err = bpftuner_bpf_init(tcp_cong, tuner, NULL); + if (err) + return err; err = bpftune_cap_add(); if (err) { From 6a73a359103c806396fb7e449971f29adf500f81 Mon Sep 17 00:00:00 2001 From: Alan Maguire Date: Mon, 10 Jul 2023 17:46:52 +0100 Subject: [PATCH 2/2] update sample tuner to return status --- sample_tuner/sample_tuner.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sample_tuner/sample_tuner.c b/sample_tuner/sample_tuner.c index 46de6c5..9db258c 100644 --- a/sample_tuner/sample_tuner.c +++ b/sample_tuner/sample_tuner.c @@ -24,8 +24,7 @@ int init(struct bpftuner *tuner) { - bpftuner_bpf_init(sample, tuner, NULL); - return 0; + return bpftuner_bpf_init(sample, tuner, NULL); } void fini(struct bpftuner *tuner)