[PATCH bpf-next 0/1] Fix for bpf_sysctl_set_new_value

[PATCH bpf-next 0/1] Fix for bpf_sysctl_set_new_value

[Raman Shukhau]

Noticed that 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

On investigation I found 2 things that needs to be changed:
* return value check
* new_len provided by bpf back to sysctl. 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`. To make it consistent
  with bpf_sysctl_set_new_value, this change also 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. Note: if incorrect count is used, sysctl
  returns EINVAL to the user.

Raman Shukhau (1):
  Fix for bpf_sysctl_set_new_value

 kernel/bpf/cgroup.c                           |  7 ++-
 .../bpf/progs/test_sysctl_overwrite.c         | 47 +++++++++++++++++++
 tools/testing/selftests/bpf/test_sysctl.c     | 35 +++++++++++++-
 3 files changed, 85 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c

-- 
2.43.0

[PATCH bpf-next 1/1] Fix for bpf_sysctl_set_new_value

[Raman Shukhau]

Noticed that 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

On investigation I found 2 things that needs to be changed:
* return value check
* new_len provided by bpf back to sysctl. 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`. To make it consistent
  with bpf_sysctl_set_new_value, this change also 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. Note: if incorrect count is used, sysctl
  returns EINVAL to the user.

Signed-off-by: Raman Shukhau <ramasha@fb.com>
---
 kernel/bpf/cgroup.c                           |  7 ++-
 .../bpf/progs/test_sysctl_overwrite.c         | 47 +++++++++++++++++++
 tools/testing/selftests/bpf/test_sysctl.c     | 35 +++++++++++++-
 3 files changed, 85 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 8ba73042a239..23736aed1b53 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -1739,10 +1739,13 @@ 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;
+		if (!(*buf)[ctx.new_len])
+			*pcount = ctx.new_len - 1;
+		else
+			*pcount = ctx.new_len;
 	} else {
 		kfree(ctx.new_val);
 	}
diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
new file mode 100644
index 000000000000..e44b429fcfc1
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Facebook
+
+#include <string.h>
+
+#include <linux/bpf.h>
+
+#include <bpf/bpf_helpers.h>
+
+#include "bpf_compiler.h"
+
+static const char sysctl_value[] = "31337";
+static const char sysctl_name[] = "net/ipv4/ip_local_reserved_ports";
+static __always_inline int is_expected_name(struct bpf_sysctl *ctx)
+{
+	unsigned char i;
+	char name[sizeof(sysctl_name)];
+	int ret;
+
+	memset(name, 0, sizeof(name));
+	ret = bpf_sysctl_get_name(ctx, name, sizeof(name), 0);
+	if (ret < 0 || ret != sizeof(sysctl_name) - 1)
+		return 0;
+
+	__pragma_loop_unroll_full
+	for (i = 0; i < sizeof(sysctl_name); ++i)
+		if (name[i] != sysctl_name[i])
+			return 0;
+
+	return 1;
+}
+
+SEC("cgroup/sysctl")
+int test_value_overwrite(struct bpf_sysctl *ctx)
+{
+	if (!ctx->write)
+		return 1;
+
+	if (!is_expected_name(ctx))
+		return 0;
+
+	if (bpf_sysctl_set_new_value(ctx, sysctl_value, sizeof(sysctl_value)) == 0)
+		return 1;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
index bcdbd27f22f0..dfa479861d3a 100644
--- a/tools/testing/selftests/bpf/test_sysctl.c
+++ b/tools/testing/selftests/bpf/test_sysctl.c
@@ -35,6 +35,7 @@ struct sysctl_test {
 	int seek;
 	const char *newval;
 	const char *oldval;
+	const char *updval;
 	enum {
 		LOAD_REJECT,
 		ATTACH_REJECT,
@@ -1395,6 +1396,16 @@ static struct sysctl_test tests[] = {
 		.open_flags = O_RDONLY,
 		.result = SUCCESS,
 	},
+	{
+		"C prog: override write to ip_local_reserved_ports",
+		.prog_file = "./test_sysctl_overwrite.bpf.o",
+		.attach_type = BPF_CGROUP_SYSCTL,
+		.sysctl = "net/ipv4/ip_local_reserved_ports",
+		.open_flags = O_RDWR,
+		.newval = "11111",
+		.updval = "31337",
+		.result = SUCCESS,
+	},
 };
 
 static size_t probe_prog_length(const struct bpf_insn *fp)
@@ -1520,13 +1531,33 @@ static int access_sysctl(const char *sysctl_path,
 			log_err("Read value %s != %s", buf, test->oldval);
 			goto err;
 		}
-	} else if (test->open_flags == O_WRONLY) {
+	} else if (test->open_flags == O_WRONLY || test->open_flags == O_RDWR) {
 		if (!test->newval) {
 			log_err("New value for sysctl is not set");
 			goto err;
 		}
-		if (write(fd, test->newval, strlen(test->newval)) == -1)
+		if (write(fd, test->newval, strlen(test->newval)) == -1) {
+			log_err("Unable to write sysctl value");
 			goto err;
+		}
+		if (test->open_flags == O_RDWR) {
+			char buf[128];
+
+			if (!test->updval) {
+				log_err("Expected value for sysctl is not set");
+				goto err;
+			}
+
+			lseek(fd, 0, SEEK_SET);
+			if (read(fd, buf, sizeof(buf)) == -1) {
+				log_err("Unable to read updated value");
+				goto err;
+			}
+			if (strncmp(buf, test->updval, strlen(test->updval))) {
+				log_err("Overwritten value %s != %s", buf, test->updval);
+				goto err;
+			}
+		}
 	} else {
 		log_err("Unexpected sysctl access: neither read nor write");
 		goto err;
-- 
2.43.0

Re: [PATCH bpf-next 1/1] Fix for bpf_sysctl_set_new_value

[Martin KaFai Lau]

On 5/4/24 3:23 AM, Raman Shukhau wrote:
> Noticed that 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
> 
> On investigation I found 2 things that needs to be changed:
> * return value check
> * new_len provided by bpf back to sysctl. proc_sys_call_handler	expects
>    this value NOT to include \0 symbol, e.g. if user do

Thanks for the report and the patch.

This patch is changing a few things (1 fix, 1 improvement, 1 test).

Separate these individual changes into its own patch. Patch 1 fixes the return 
value. Patch 2 improves the '\0' and *pcount situation. Patch 3 adds the test.

btw, I am curious what is missed in the test_sysctl.c that didn't catch the 
return value case?

> 
> 	```
>    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`. To make it consistent
>    with bpf_sysctl_set_new_value, this change also 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. Note: if incorrect count is used, sysctl
>    returns EINVAL to the user.
> 
> Signed-off-by: Raman Shukhau <ramasha@fb.com>
> ---
>   kernel/bpf/cgroup.c                           |  7 ++-
>   .../bpf/progs/test_sysctl_overwrite.c         | 47 +++++++++++++++++++
>   tools/testing/selftests/bpf/test_sysctl.c     | 35 +++++++++++++-
>   3 files changed, 85 insertions(+), 4 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 8ba73042a239..23736aed1b53 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1739,10 +1739,13 @@ 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;
> +		if (!(*buf)[ctx.new_len])
> +			*pcount = ctx.new_len - 1;

 From looking at how new_updated is set, my understanding is new_len cannot be 0 
here. just want to double check.


> +		else
> +			*pcount = ctx.new_len;
>   	} else {
>   		kfree(ctx.new_val);
>   	}
> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
> new file mode 100644
> index 000000000000..e44b429fcfc1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Facebook
> +
> +#include <string.h>
> +
> +#include <linux/bpf.h>
> +
> +#include <bpf/bpf_helpers.h>
> +
> +#include "bpf_compiler.h"
> +
> +static const char sysctl_value[] = "31337";
> +static const char sysctl_name[] = "net/ipv4/ip_local_reserved_ports";
> +static __always_inline int is_expected_name(struct bpf_sysctl *ctx)
> +{
> +	unsigned char i;
> +	char name[sizeof(sysctl_name)];
> +	int ret;
> +
> +	memset(name, 0, sizeof(name));
> +	ret = bpf_sysctl_get_name(ctx, name, sizeof(name), 0);
> +	if (ret < 0 || ret != sizeof(sysctl_name) - 1)
> +		return 0;
> +
> +	__pragma_loop_unroll_full
> +	for (i = 0; i < sizeof(sysctl_name); ++i)
> +		if (name[i] != sysctl_name[i])

bpf_strncmp() should be useful here.

> +			return 0;
> +
> +	return 1;
> +}
> +
> +SEC("cgroup/sysctl")
> +int test_value_overwrite(struct bpf_sysctl *ctx)
> +{
> +	if (!ctx->write)
> +		return 1;
> +
> +	if (!is_expected_name(ctx))
> +		return 0;
> +
> +	if (bpf_sysctl_set_new_value(ctx, sysctl_value, sizeof(sysctl_value)) == 0)
> +		return 1;
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
> index bcdbd27f22f0..dfa479861d3a 100644
> --- a/tools/testing/selftests/bpf/test_sysctl.c
> +++ b/tools/testing/selftests/bpf/test_sysctl.c
> @@ -35,6 +35,7 @@ struct sysctl_test {
>   	int seek;
>   	const char *newval;
>   	const char *oldval;
> +	const char *updval;
>   	enum {
>   		LOAD_REJECT,
>   		ATTACH_REJECT,
> @@ -1395,6 +1396,16 @@ static struct sysctl_test tests[] = {
>   		.open_flags = O_RDONLY,
>   		.result = SUCCESS,
>   	},
> +	{
> +		"C prog: override write to ip_local_reserved_ports",
> +		.prog_file = "./test_sysctl_overwrite.bpf.o",

test_sysctl.c is not run in bpf CI. It is not very useful to extend this test 
further. Lets take this chance to create a new progs/cgrp_sysctl.c test that 
will be exercised by ./test_progs in bpf CI. Then it can use the newer skel 
open_and_load also.

Not asking to to migrate the existing tests in test_sysctl.c to the new 
progs/cgrp_sysctl.c in this patch set. The new cgrp_sysctl.c can only have the 
tests that exercise the changes in this patch set. However, it will be useful if 
progs/cgrp_sysctl.c can be bootstrapped in a way that the future test_sysctl.c 
migration will be easier. I also wouldn't worry too much on the existing raw 
insns tests in test_sysctl.c for now. They will need to be moved to either C or 
bpf asm in the future.

pw-bot: cr

> +		.attach_type = BPF_CGROUP_SYSCTL,
> +		.sysctl = "net/ipv4/ip_local_reserved_ports",
> +		.open_flags = O_RDWR,
> +		.newval = "11111",
> +		.updval = "31337",
> +		.result = SUCCESS,
> +	},
>   };
>   
>   static size_t probe_prog_length(const struct bpf_insn *fp)
> @@ -1520,13 +1531,33 @@ static int access_sysctl(const char *sysctl_path,
>   			log_err("Read value %s != %s", buf, test->oldval);
>   			goto err;
>   		}
> -	} else if (test->open_flags == O_WRONLY) {
> +	} else if (test->open_flags == O_WRONLY || test->open_flags == O_RDWR) {
>   		if (!test->newval) {
>   			log_err("New value for sysctl is not set");
>   			goto err;
>   		}
> -		if (write(fd, test->newval, strlen(test->newval)) == -1)
> +		if (write(fd, test->newval, strlen(test->newval)) == -1) {
> +			log_err("Unable to write sysctl value");
>   			goto err;
> +		}
> +		if (test->open_flags == O_RDWR) {
> +			char buf[128];
> +
> +			if (!test->updval) {
> +				log_err("Expected value for sysctl is not set");
> +				goto err;
> +			}
> +
> +			lseek(fd, 0, SEEK_SET);
> +			if (read(fd, buf, sizeof(buf)) == -1) {
> +				log_err("Unable to read updated value");
> +				goto err;
> +			}
> +			if (strncmp(buf, test->updval, strlen(test->updval))) {
> +				log_err("Overwritten value %s != %s", buf, test->updval);
> +				goto err;
> +			}
> +		}
>   	} else {
>   		log_err("Unexpected sysctl access: neither read nor write");
>   		goto err;

Re: [PATCH bpf-next 1/1] Fix for bpf_sysctl_set_new_value

[Raman Shukhau]

> btw, I am curious what is missed in the test_sysctl.c that didn't catch the return value case?

Test didn’t check new sysctl value, only if return code is successful. In this case it silently ignores new value.


> From looking at how new_updated is set, my understanding is new_len cannot be 0 here. just want to double check.

bpf_sysctl_set_new_value checks that new_len is not 0, otherwise returns EINVAL



> On May 7, 2024, at 4:20 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
> 
> 
> On 5/4/24 3:23 AM, Raman Shukhau wrote:
>> Noticed that 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
>> On investigation I found 2 things that needs to be changed:
>> * return value check
>> * new_len provided by bpf back to sysctl. proc_sys_call_handler	expects
>>   this value NOT to include \0 symbol, e.g. if user do
> 
> Thanks for the report and the patch.
> 
> This patch is changing a few things (1 fix, 1 improvement, 1 test).
> 
> Separate these individual changes into its own patch. Patch 1 fixes the return value. Patch 2 improves the '\0' and *pcount situation. Patch 3 adds the test.
> 
> btw, I am curious what is missed in the test_sysctl.c that didn't catch the return value case?
> 
>> 	```
>>   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`. To make it consistent
>>   with bpf_sysctl_set_new_value, this change also 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. Note: if incorrect count is used, sysctl
>>   returns EINVAL to the user.
>> Signed-off-by: Raman Shukhau <ramasha@fb.com>
>> ---
>>  kernel/bpf/cgroup.c                           |  7 ++-
>>  .../bpf/progs/test_sysctl_overwrite.c         | 47 +++++++++++++++++++
>>  tools/testing/selftests/bpf/test_sysctl.c     | 35 +++++++++++++-
>>  3 files changed, 85 insertions(+), 4 deletions(-)
>>  create mode 100644 tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 8ba73042a239..23736aed1b53 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -1739,10 +1739,13 @@ 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;
>> +		if (!(*buf)[ctx.new_len])
>> +			*pcount = ctx.new_len - 1;
> 
> From looking at how new_updated is set, my understanding is new_len cannot be 0 here. just want to double check.
> 
> 
>> +		else
>> +			*pcount = ctx.new_len;
>>  	} else {
>>  		kfree(ctx.new_val);
>>  	}
>> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
>> new file mode 100644
>> index 000000000000..e44b429fcfc1
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
>> @@ -0,0 +1,47 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2019 Facebook
>> +
>> +#include <string.h>
>> +
>> +#include <linux/bpf.h>
>> +
>> +#include <bpf/bpf_helpers.h>
>> +
>> +#include "bpf_compiler.h"
>> +
>> +static const char sysctl_value[] = "31337";
>> +static const char sysctl_name[] = "net/ipv4/ip_local_reserved_ports";
>> +static __always_inline int is_expected_name(struct bpf_sysctl *ctx)
>> +{
>> +	unsigned char i;
>> +	char name[sizeof(sysctl_name)];
>> +	int ret;
>> +
>> +	memset(name, 0, sizeof(name));
>> +	ret = bpf_sysctl_get_name(ctx, name, sizeof(name), 0);
>> +	if (ret < 0 || ret != sizeof(sysctl_name) - 1)
>> +		return 0;
>> +
>> +	__pragma_loop_unroll_full
>> +	for (i = 0; i < sizeof(sysctl_name); ++i)
>> +		if (name[i] != sysctl_name[i])
> 
> bpf_strncmp() should be useful here.
> 
>> +			return 0;
>> +
>> +	return 1;
>> +}
>> +
>> +SEC("cgroup/sysctl")
>> +int test_value_overwrite(struct bpf_sysctl *ctx)
>> +{
>> +	if (!ctx->write)
>> +		return 1;
>> +
>> +	if (!is_expected_name(ctx))
>> +		return 0;
>> +
>> +	if (bpf_sysctl_set_new_value(ctx, sysctl_value, sizeof(sysctl_value)) == 0)
>> +		return 1;
>> +	return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
>> index bcdbd27f22f0..dfa479861d3a 100644
>> --- a/tools/testing/selftests/bpf/test_sysctl.c
>> +++ b/tools/testing/selftests/bpf/test_sysctl.c
>> @@ -35,6 +35,7 @@ struct sysctl_test {
>>  	int seek;
>>  	const char *newval;
>>  	const char *oldval;
>> +	const char *updval;
>>  	enum {
>>  		LOAD_REJECT,
>>  		ATTACH_REJECT,
>> @@ -1395,6 +1396,16 @@ static struct sysctl_test tests[] = {
>>  		.open_flags = O_RDONLY,
>>  		.result = SUCCESS,
>>  	},
>> +	{
>> +		"C prog: override write to ip_local_reserved_ports",
>> +		.prog_file = "./test_sysctl_overwrite.bpf.o",
> 
> test_sysctl.c is not run in bpf CI. It is not very useful to extend this test further. Lets take this chance to create a new progs/cgrp_sysctl.c test that will be exercised by ./test_progs in bpf CI. Then it can use the newer skel open_and_load also.
> 
> Not asking to to migrate the existing tests in test_sysctl.c to the new progs/cgrp_sysctl.c in this patch set. The new cgrp_sysctl.c can only have the tests that exercise the changes in this patch set. However, it will be useful if progs/cgrp_sysctl.c can be bootstrapped in a way that the future test_sysctl.c migration will be easier. I also wouldn't worry too much on the existing raw insns tests in test_sysctl.c for now. They will need to be moved to either C or bpf asm in the future.
> 
> pw-bot: cr
> 
>> +		.attach_type = BPF_CGROUP_SYSCTL,
>> +		.sysctl = "net/ipv4/ip_local_reserved_ports",
>> +		.open_flags = O_RDWR,
>> +		.newval = "11111",
>> +		.updval = "31337",
>> +		.result = SUCCESS,
>> +	},
>>  };
>>    static size_t probe_prog_length(const struct bpf_insn *fp)
>> @@ -1520,13 +1531,33 @@ static int access_sysctl(const char *sysctl_path,
>>  			log_err("Read value %s != %s", buf, test->oldval);
>>  			goto err;
>>  		}
>> -	} else if (test->open_flags == O_WRONLY) {
>> +	} else if (test->open_flags == O_WRONLY || test->open_flags == O_RDWR) {
>>  		if (!test->newval) {
>>  			log_err("New value for sysctl is not set");
>>  			goto err;
>>  		}
>> -		if (write(fd, test->newval, strlen(test->newval)) == -1)
>> +		if (write(fd, test->newval, strlen(test->newval)) == -1) {
>> +			log_err("Unable to write sysctl value");
>>  			goto err;
>> +		}
>> +		if (test->open_flags == O_RDWR) {
>> +			char buf[128];
>> +
>> +			if (!test->updval) {
>> +				log_err("Expected value for sysctl is not set");
>> +				goto err;
>> +			}
>> +
>> +			lseek(fd, 0, SEEK_SET);
>> +			if (read(fd, buf, sizeof(buf)) == -1) {
>> +				log_err("Unable to read updated value");
>> +				goto err;
>> +			}
>> +			if (strncmp(buf, test->updval, strlen(test->updval))) {
>> +				log_err("Overwritten value %s != %s", buf, test->updval);
>> +				goto err;
>> +			}
>> +		}
>>  	} else {
>>  		log_err("Unexpected sysctl access: neither read nor write");
>>  		goto err;
>