[PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value

[PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value

[Raman Shukhau]

Changes v1 => v2:
1. corrected copyright comments
2. unsigned int for sysctl name to prevent build test error:
   "R2 min value is negative, either use unsigned or 'var &= const'"

Fix and improvement for bpf_sysctl_set_new_value

This patch set is doing several changes around bpf_sysctl_set_new_value
(1 fix, 1 improvement, 1 test):
1. Fix is for return value check, when sysctl value is updated from BPF
  handler with call to bpf_sysctl_set_new_value.
2. Improvement for bpf_sysctl_set_new_value to match behavior with
  sysctl write call. Result value shouldn't include "\0", otherwise
  proc_sys_call_handler rejects value.
3. New cgrp_sysctl test suite is added. It has single test to check
  behavior of bpf_sysctl_set_new_value and is called from BPF
  test_progs test suite.

Raman Shukhau (3):
  net: Fix for bpf_sysctl_set_new_value
  net: Improvement for bpf_sysctl_set_new_value
  net: new cgrp_sysctl test suite

 kernel/bpf/cgroup.c                           |   7 +-
 .../selftests/bpf/prog_tests/cgrp_sysctl.c    | 103 ++++++++++++++++++
 .../testing/selftests/bpf/progs/cgrp_sysctl.c |  51 +++++++++
 3 files changed, 159 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/cgrp_sysctl.c
 create mode 100644 tools/testing/selftests/bpf/progs/cgrp_sysctl.c

-- 
2.43.0

[PATCH v2 bpf-next 1/3] net: Fix for bpf_sysctl_set_new_value

[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

[PATCH v2 bpf-next 2/3] net: Improvement for bpf_sysctl_set_new_value

[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

[PATCH v2 bpf-next 3/3] net: new cgrp_sysctl test suite

[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

Re: [PATCH v2 bpf-next 0/3] Fix and improvement for bpf_sysctl_set_new_value

[Alexei Starovoitov]

On Mon, May 20, 2024 at 2:14 AM Raman Shukhau <ramasha@meta.com> wrote:
>
> Changes v1 => v2:
> 1. corrected copyright comments
> 2. unsigned int for sysctl name to prevent build test error:
>    "R2 min value is negative, either use unsigned or 'var &= const'"

CI disagrees. Same failure in test_progs-no_alu32

42: (18) r3 = 0xffffa5bd00dd5014 ;
R3_w=map_value(map=cgrp_sys.rodata,ks=4,vs=152,off=20)
44: (85) call bpf_strncmp#182
R2 min value is negative, either use unsigned or 'var &= const'

pw-bot: cr