[bpf-next,v2,1/3] selftests/bpf: do not disable /dev/null device access in cgroup dev test

Commit Message

Alexis Lothoré (eBPF Foundation) July 29, 2024, 8:20 a.m. UTC

test_dev_cgroup currently loads a small bpf program allowing any access on
urandom and zero devices, disabling access to any other device. It makes
migrating this test to test_progs impossible, since this one manipulates
extensively /dev/null.

Allow /dev/null manipulation in dev_cgroup program to make its usage in
test_progs framework possible. Update test_dev_cgroup.c as well to match
this change while it has not been removed.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <[e-mail]>
---
 tools/testing/selftests/bpf/progs/dev_cgroup.c |  4 ++--
 tools/testing/selftests/bpf/test_dev_cgroup.c  | 18 +++++++++---------
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Alan Maguire July 29, 2024, 4:59 p.m. UTC | #1

On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
> test_dev_cgroup currently loads a small bpf program allowing any access on
> urandom and zero devices, disabling access to any other device. It makes
> migrating this test to test_progs impossible, since this one manipulates
> extensively /dev/null.
> 
> Allow /dev/null manipulation in dev_cgroup program to make its usage in
> test_progs framework possible. Update test_dev_cgroup.c as well to match
> this change while it has not been removed.
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <[e-mail]>
> ---
>  tools/testing/selftests/bpf/progs/dev_cgroup.c |  4 ++--
>  tools/testing/selftests/bpf/test_dev_cgroup.c  | 18 +++++++++---------

Not a big deal, but I found it a bit confusing that this file was
modified then deleted in patch 2. Would it work having patch 1 stop
building the standalone test/remove it and .gitignore entry, patch 2
updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access,
patch 3 add cgroup_dev.c test support, and patch 4 add the device type
subtest? Or are there issues with doing things that way? Thanks!

Alan

Alexis Lothoré (eBPF Foundation) July 29, 2024, 5:30 p.m. UTC | #2

Hello Alan,

On 7/29/24 18:59, Alan Maguire wrote:
> On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
>> test_dev_cgroup currently loads a small bpf program allowing any access on
>> urandom and zero devices, disabling access to any other device. It makes
>> migrating this test to test_progs impossible, since this one manipulates
>> extensively /dev/null.
>>
>> Allow /dev/null manipulation in dev_cgroup program to make its usage in
>> test_progs framework possible. Update test_dev_cgroup.c as well to match
>> this change while it has not been removed.
>>
>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <[e-mail]>
>> ---
>>  tools/testing/selftests/bpf/progs/dev_cgroup.c |  4 ++--
>>  tools/testing/selftests/bpf/test_dev_cgroup.c  | 18 +++++++++---------
> 
> Not a big deal, but I found it a bit confusing that this file was
> modified then deleted in patch 2. Would it work having patch 1 stop
> building the standalone test/remove it and .gitignore entry, patch 2
> updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access,
> patch 3 add cgroup_dev.c test support, and patch 4 add the device type
> subtest? Or are there issues with doing things that way? Thanks!

I've done this to make sure that at any point in the git history, there is one
working test for the targeted feature, either the old or the new one. I've done
it this way because the old test also helped me validate the new one while
developing it, but also because if at some point there is a (major) issue with
the new test, reverting only the relevant commit brings back the old test while
disabling the new one.

But maybe this concern is not worth the trouble (especially since the old tests
are not run automatically) ? If that's indeed the case, I can do it the way you
are suggesting :)

Thanks,

Alexis

Alan Maguire July 30, 2024, 8:16 a.m. UTC | #3

On 29/07/2024 18:30, Alexis Lothoré wrote:
> Hello Alan,
> 
> On 7/29/24 18:59, Alan Maguire wrote:
>> On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
>>> test_dev_cgroup currently loads a small bpf program allowing any access on
>>> urandom and zero devices, disabling access to any other device. It makes
>>> migrating this test to test_progs impossible, since this one manipulates
>>> extensively /dev/null.
>>>
>>> Allow /dev/null manipulation in dev_cgroup program to make its usage in
>>> test_progs framework possible. Update test_dev_cgroup.c as well to match
>>> this change while it has not been removed.
>>>
>>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <[e-mail]>
>>> ---
>>>  tools/testing/selftests/bpf/progs/dev_cgroup.c |  4 ++--
>>>  tools/testing/selftests/bpf/test_dev_cgroup.c  | 18 +++++++++---------
>>
>> Not a big deal, but I found it a bit confusing that this file was
>> modified then deleted in patch 2. Would it work having patch 1 stop
>> building the standalone test/remove it and .gitignore entry, patch 2
>> updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access,
>> patch 3 add cgroup_dev.c test support, and patch 4 add the device type
>> subtest? Or are there issues with doing things that way? Thanks!
> 
> I've done this to make sure that at any point in the git history, there is one
> working test for the targeted feature, either the old or the new one. I've done
> it this way because the old test also helped me validate the new one while
> developing it, but also because if at some point there is a (major) issue with
> the new test, reverting only the relevant commit brings back the old test while
> disabling the new one.
> 
> But maybe this concern is not worth the trouble (especially since the old tests
> are not run automatically) ? If that's indeed the case, I can do it the way you
> are suggesting :)
>

If no-one complains, it seems fine to me to stick with the way you've
constructed the series the next respin. Thanks!

> Thanks,
> 
> Alexis
>

Alexis Lothoré (eBPF Foundation) July 30, 2024, 8:42 a.m. UTC | #4

On 7/30/24 10:16, Alan Maguire wrote:
> On 29/07/2024 18:30, Alexis Lothoré wrote:
>> Hello Alan,

[...]

>>> Not a big deal, but I found it a bit confusing that this file was
>>> modified then deleted in patch 2. Would it work having patch 1 stop
>>> building the standalone test/remove it and .gitignore entry, patch 2
>>> updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access,
>>> patch 3 add cgroup_dev.c test support, and patch 4 add the device type
>>> subtest? Or are there issues with doing things that way? Thanks!
>>
>> I've done this to make sure that at any point in the git history, there is one
>> working test for the targeted feature, either the old or the new one. I've done
>> it this way because the old test also helped me validate the new one while
>> developing it, but also because if at some point there is a (major) issue with
>> the new test, reverting only the relevant commit brings back the old test while
>> disabling the new one.
>>
>> But maybe this concern is not worth the trouble (especially since the old tests
>> are not run automatically) ? If that's indeed the case, I can do it the way you
>> are suggesting :)
>>
> 
> If no-one complains, it seems fine to me to stick with the way you've
> constructed the series the next respin. Thanks!

ACK, thanks, I'll keep it that way then.

For the record, I am accumulating a few other converted tests that I will send
soon, and those follow the same logic (keeping one working test at any point of
time, and pushing it to the point where I start by fixing broken tests before
converting those), so if anyone has an opinion in favor of this or rather in
favor of Alan's suggestion, do not hesitate to share it, so I can adjust before
sending.

Thanks,

> 
>> Thanks,
>>
>> Alexis
>>
>

Patch

diff --git a/tools/testing/selftests/bpf/progs/dev_cgroup.c b/tools/testing/selftests/bpf/progs/dev_cgroup.c
index 79b54a4fa244..c1dfbd2b56fc 100644
--- a/tools/testing/selftests/bpf/progs/dev_cgroup.c
+++ b/tools/testing/selftests/bpf/progs/dev_cgroup.c
@@ -41,14 +41,14 @@  int bpf_prog1(struct bpf_cgroup_dev_ctx *ctx)
 	bpf_trace_printk(fmt, sizeof(fmt), ctx->major, ctx->minor);
 #endif
 
-	/* Allow access to /dev/zero and /dev/random.
+	/* Allow access to /dev/null and /dev/urandom.
 	 * Forbid everything else.
 	 */
 	if (ctx->major != 1 || type != BPF_DEVCG_DEV_CHAR)
 		return 0;
 
 	switch (ctx->minor) {
-	case 5: /* 1:5 /dev/zero */
+	case 3: /* 1:3 /dev/null */
 	case 9: /* 1:9 /dev/urandom */
 		return 1;
 	}
diff --git a/tools/testing/selftests/bpf/test_dev_cgroup.c b/tools/testing/selftests/bpf/test_dev_cgroup.c
index adeaf63cb6fa..33f544f0005a 100644
--- a/tools/testing/selftests/bpf/test_dev_cgroup.c
+++ b/tools/testing/selftests/bpf/test_dev_cgroup.c
@@ -54,25 +54,25 @@  int main(int argc, char **argv)
 		goto err;
 	}
 
-	/* All operations with /dev/zero and and /dev/urandom are allowed,
+	/* All operations with /dev/null and /dev/urandom are allowed,
 	 * everything else is forbidden.
 	 */
-	assert(system("rm -f /tmp/test_dev_cgroup_null") == 0);
-	assert(system("mknod /tmp/test_dev_cgroup_null c 1 3"));
-	assert(system("rm -f /tmp/test_dev_cgroup_null") == 0);
-
-	/* /dev/zero is whitelisted */
 	assert(system("rm -f /tmp/test_dev_cgroup_zero") == 0);
-	assert(system("mknod /tmp/test_dev_cgroup_zero c 1 5") == 0);
+	assert(system("mknod /tmp/test_dev_cgroup_zero c 1 5"));
 	assert(system("rm -f /tmp/test_dev_cgroup_zero") == 0);
 
-	assert(system("dd if=/dev/urandom of=/dev/zero count=64") == 0);
+	/* /dev/null is whitelisted */
+	assert(system("rm -f /tmp/test_dev_cgroup_null") == 0);
+	assert(system("mknod /tmp/test_dev_cgroup_null c 1 3") == 0);
+	assert(system("rm -f /tmp/test_dev_cgroup_null") == 0);
+
+	assert(system("dd if=/dev/urandom of=/dev/null count=64") == 0);
 
 	/* src is allowed, target is forbidden */
 	assert(system("dd if=/dev/urandom of=/dev/full count=64"));
 
 	/* src is forbidden, target is allowed */
-	assert(system("dd if=/dev/random of=/dev/zero count=64"));
+	assert(system("dd if=/dev/random of=/dev/null count=64"));
 
 	error = 0;
 	printf("test_dev_cgroup:PASS\n");