* [PATCH v2 bpf-next 1/3] net: Fix for bpf_sysctl_set_new_value
2024-05-20 9:14 [PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value Raman Shukhau
@ 2024-05-20 9:14 ` Raman Shukhau
2024-05-20 9:14 ` [PATCH v2 bpf-next 2/3] net: Improvement " Raman Shukhau
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Raman Shukhau @ 2024-05-20 9:14 UTC (permalink / raw)
To: bpf, ast, andrii, daniel; +Cc: Raman Shukhau
Call to bpf_sysctl_set_new_value doesn't change final value
of the parameter, when called from cgroup/syscall bpf handler. No error
thrown in this case, new value is simply ignored and original value, sent
to sysctl, is set. Example (see test added to this change for BPF handler
logic):
sysctl -w net.ipv4.ip_local_reserved_ports = 11111
... cgroup/syscal handler call bpf_sysctl_set_new_value and set 22222
sysctl net.ipv4.ip_local_reserved_ports
... returns 11111
Return value check is incorrect in __cgroup_bpf_run_filter_sysctl
specifically for the case when new value is set, as bpf_prog_run_array_cg
return 0 on success.
Signed-off-by: Raman Shukhau <ramasha@meta.com>
---
kernel/bpf/cgroup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 8ba73042a239..bfc36e7ca6f6 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1739,7 +1739,7 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
kfree(ctx.cur_val);
- if (ret == 1 && ctx.new_updated) {
+ if (ret == 0 && ctx.new_updated) {
kfree(*buf);
*buf = ctx.new_val;
*pcount = ctx.new_len;
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2 bpf-next 2/3] net: Improvement for bpf_sysctl_set_new_value
2024-05-20 9:14 [PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value Raman Shukhau
2024-05-20 9:14 ` [PATCH v2 bpf-next 1/3] net: Fix " Raman Shukhau
@ 2024-05-20 9:14 ` Raman Shukhau
2024-05-20 9:14 ` [PATCH v2 bpf-next 3/3] net: new cgrp_sysctl test suite Raman Shukhau
2024-05-20 14:59 ` [PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value Alexei Starovoitov
3 siblings, 0 replies; 5+ messages in thread
From: Raman Shukhau @ 2024-05-20 9:14 UTC (permalink / raw)
To: bpf, ast, andrii, daniel; +Cc: Raman Shukhau
When bpf_sysctl_set_new_value is called in cgroup/sysctl handler, updated
"new_len" is provided back to proc_sys_call_handler. But
proc_sys_call_handler expects this value NOT to include \0 symbol, e.g.
if user do:
open("/proc/sys/net/ipv4/ip_local_reserved_ports", ...)
write(fd, "11111", sizeof("22222"))
or
echo -n "11111" > /proc/sys/net/ipv4/ip_local_reserved_ports
or
sysctl -w net.ipv4.ip_local_reserved_ports=11111
proc_sys_call_handler receives count equal to `5`. if BPF handler code
doesn't account for that, new value will be rejected by
proc_sys_call_handler with EINVAL error.
To make behavior consistent for bpf_sysctl_set_new_value, this change
adjust `new_len` with `-1`, if `\0` passed as last character.
Alternatively, using `sizeof("11111") - 1` in BPF handler should work,
but it might not be obvious and spark confusion.
Signed-off-by: Raman Shukhau <ramasha@meta.com>
---
kernel/bpf/cgroup.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index bfc36e7ca6f6..23736aed1b53 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1742,7 +1742,10 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
if (ret == 0 && ctx.new_updated) {
kfree(*buf);
*buf = ctx.new_val;
- *pcount = ctx.new_len;
+ if (!(*buf)[ctx.new_len])
+ *pcount = ctx.new_len - 1;
+ else
+ *pcount = ctx.new_len;
} else {
kfree(ctx.new_val);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2 bpf-next 3/3] net: new cgrp_sysctl test suite
2024-05-20 9:14 [PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value Raman Shukhau
2024-05-20 9:14 ` [PATCH v2 bpf-next 1/3] net: Fix " Raman Shukhau
2024-05-20 9:14 ` [PATCH v2 bpf-next 2/3] net: Improvement " Raman Shukhau
@ 2024-05-20 9:14 ` Raman Shukhau
2024-05-20 14:59 ` [PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value Alexei Starovoitov
3 siblings, 0 replies; 5+ messages in thread
From: Raman Shukhau @ 2024-05-20 9:14 UTC (permalink / raw)
To: bpf, ast, andrii, daniel; +Cc: Raman Shukhau
Adding new prog_tests for sysctl BPF handlers, first version with
a single test to validate bpf_sysctl_set_new_value call
Signed-off-by: Raman Shukhau <ramasha@meta.com>
---
.../selftests/bpf/prog_tests/cgrp_sysctl.c | 103 ++++++++++++++++++
.../testing/selftests/bpf/progs/cgrp_sysctl.c | 51 +++++++++
2 files changed, 154 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/cgrp_sysctl.c
create mode 100644 tools/testing/selftests/bpf/progs/cgrp_sysctl.c
diff --git a/tools/testing/selftests/bpf/prog_tests/cgrp_sysctl.c b/tools/testing/selftests/bpf/prog_tests/cgrp_sysctl.c
new file mode 100644
index 000000000000..12a160428a1d
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgrp_sysctl.c
@@ -0,0 +1,103 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#define SYSCTL_ROOT_PATH "/proc/sys/"
+#define SYSCTL_NAME_LEN 128
+#define RESERVED_PORTS_SYSCTL_NAME "net/ipv4/ip_local_reserved_ports"
+#define RESERVED_PORTS_OVERRIDE_VALUE "31337"
+
+#define _GNU_SOURCE
+#include <unistd.h>
+#include <string.h>
+#include <fcntl.h>
+
+#include <sys/mount.h>
+
+#include "test_progs.h"
+#include "cgrp_sysctl.skel.h"
+
+struct sysctl_test {
+ const char *sysctl;
+ int open_flags;
+ const char *newval;
+ const char *updval;
+};
+
+static void subtest(int cgroup_fd, struct cgrp_sysctl *skel, struct sysctl_test *test_data)
+{
+ int fd;
+
+ fd = open(SYSCTL_ROOT_PATH RESERVED_PORTS_SYSCTL_NAME, test_data->open_flags | O_CLOEXEC);
+ if (!ASSERT_GT(fd, 0, "sysctl-open"))
+ return;
+
+ if (test_data->open_flags == O_RDWR) {
+ int wr_ret;
+
+ wr_ret = write(fd, test_data->newval, strlen(test_data->newval));
+ if (!ASSERT_GT(wr_ret, 0, "sysctl-write"))
+ goto out;
+
+ char buf[SYSCTL_NAME_LEN];
+ char updval[SYSCTL_NAME_LEN];
+
+ sprintf(updval, "%s\n", test_data->updval);
+ if (!ASSERT_OK(lseek(fd, 0, SEEK_SET), "sysctl-seek"))
+ goto out;
+ if (!ASSERT_GT(read(fd, buf, sizeof(buf)), 0, "sysctl-read"))
+ goto out;
+ if (!ASSERT_OK(strncmp(buf, updval, strlen(updval)), "sysctl-updval"))
+ goto out;
+ }
+
+out:
+ close(fd);
+}
+
+void test_cgrp_sysctl(void)
+{
+ struct cgrp_sysctl *skel;
+ int cgroup_fd;
+
+ cgroup_fd = test__join_cgroup("/cgrp_sysctl");
+ if (!ASSERT_GE(cgroup_fd, 0, "cg-create"))
+ return;
+
+ skel = cgrp_sysctl__open();
+ if (!ASSERT_OK_PTR(skel, "skel-open"))
+ goto close_cgroup;
+
+ struct sysctl_test test_data;
+
+ if (test__start_subtest("overwrite_success")) {
+ test_data = (struct sysctl_test){
+ .sysctl = RESERVED_PORTS_SYSCTL_NAME,
+ .open_flags = O_RDWR,
+ .newval = "22222",
+ .updval = RESERVED_PORTS_OVERRIDE_VALUE,
+ };
+ memcpy(skel->rodata->sysctl_name, RESERVED_PORTS_SYSCTL_NAME,
+ sizeof(RESERVED_PORTS_SYSCTL_NAME));
+ skel->rodata->name_len = sizeof(RESERVED_PORTS_SYSCTL_NAME);
+ memcpy(skel->rodata->sysctl_updval, RESERVED_PORTS_OVERRIDE_VALUE,
+ sizeof(RESERVED_PORTS_OVERRIDE_VALUE));
+ skel->rodata->updval_len = sizeof(RESERVED_PORTS_OVERRIDE_VALUE);
+ }
+
+ if (!ASSERT_OK(cgrp_sysctl__load(skel), "skel-load"))
+ goto close_cgroup;
+
+ skel->links.cgrp_sysctl_overwrite =
+ bpf_program__attach_cgroup(skel->progs.cgrp_sysctl_overwrite, cgroup_fd);
+ if (!ASSERT_OK_PTR(skel->links.cgrp_sysctl_overwrite, "cg-attach-sysctl"))
+ goto skel_destroy;
+
+ subtest(cgroup_fd, skel, &test_data);
+ goto skel_destroy;
+
+skel_destroy:
+ cgrp_sysctl__destroy(skel);
+
+close_cgroup:
+ close(cgroup_fd);
+}
diff --git a/tools/testing/selftests/bpf/progs/cgrp_sysctl.c b/tools/testing/selftests/bpf/progs/cgrp_sysctl.c
new file mode 100644
index 000000000000..1038e917c760
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/cgrp_sysctl.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) Meta Platforms, Inc. and affiliates. */
+
+#include <string.h>
+#include <stdbool.h>
+
+#include <linux/bpf.h>
+
+#include <bpf/bpf_helpers.h>
+
+#include "bpf_compiler.h"
+
+#define SYSCTL_VALUE_LEN 16
+#define SYSCTL_NAME_LEN 128
+
+#define SUCCESS 1
+#define FAILURE 0
+
+const char sysctl_updval[SYSCTL_VALUE_LEN];
+volatile const unsigned int updval_len;
+const char sysctl_name[SYSCTL_NAME_LEN];
+volatile const unsigned int name_len;
+
+static __always_inline bool is_expected_name(struct bpf_sysctl *ctx)
+{
+ char name[SYSCTL_NAME_LEN];
+ unsigned int size;
+
+ memset(name, 0, sizeof(name));
+ size = bpf_sysctl_get_name(ctx, name, sizeof(name), 0);
+ if (size == 0 || size != name_len - 1)
+ return 1;
+
+ return bpf_strncmp(name, size, (const char *)sysctl_name) == 0;
+}
+
+SEC("cgroup/sysctl")
+int cgrp_sysctl_overwrite(struct bpf_sysctl *ctx)
+{
+ if (!ctx->write)
+ return SUCCESS;
+
+ if (!is_expected_name(ctx))
+ return SUCCESS;
+
+ if (bpf_sysctl_set_new_value(ctx, (char *)sysctl_updval, updval_len) == 0)
+ return SUCCESS;
+ return FAILURE;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread