diff mbox series

[RFC,v2,04/12] selftests/landlock: Add protocol.socket_access_rights to socket tests

Message ID 20240524093015.2402952-5-ivanov.mikhail1@huawei-partners.com (mailing list archive)
State RFC
Headers show
Series Socket type control for Landlock | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async

Commit Message

Mikhail Ivanov May 24, 2024, 9:30 a.m. UTC
Add test that checks possibility of adding rule with every possible
access right.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Formats code with clang-format.
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Günther Noack May 27, 2024, 8:52 p.m. UTC | #1
Hello!

I see that this test is adapted from the network_access_rights test in
net_test.c, and some of the subsequent are similarly copied from there.  It
makes it hard to criticize the code, because being a little bit consistent is
probably a good thing.  Have you found any opportunities to extract
commonalities into common.h?

On Fri, May 24, 2024 at 05:30:07PM +0800, Mikhail Ivanov wrote:
> Add test that checks possibility of adding rule with every possible
> access right.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Formats code with clang-format.
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 4c51f89ed578..eb5d62263460 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -178,4 +178,32 @@ TEST_F(protocol, create)
>  	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>  }
>  
> +TEST_F(protocol, socket_access_rights)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = ACCESS_ALL,
> +	};
> +	struct landlock_socket_attr protocol = {
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +	int ruleset_fd;
> +	__u64 access;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
> +		protocol.allowed_access = access;
> +		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					       &protocol, 0))
> +		{
> +			TH_LOG("Failed to add rule with access 0x%llx: %s",
> +			       access, strerror(errno));
> +		}
> +	}
> +	EXPECT_EQ(0, close(ruleset_fd));

Reviewed-by: Günther Noack <gnoack@google.com>

P.S. We are inconsistent with our use of EXPECT/ASSERT for test teardown.  The
fs_test.c uses ASSERT_EQ in these places whereas net_test.c and your new tests
use EXPECT_EQ.

It admittedly does not make much of a difference for close(), so should be OK.
Some other selftests are even ignoring the result for close().  If we want to
make it consistent in the Landlock tests again, we can also do it in an
independent sweep.

I filed a small cleanup task as a reminder:
https://github.com/landlock-lsm/linux/issues/31

—Günther
Mikhail Ivanov May 30, 2024, 2:35 p.m. UTC | #2
5/27/2024 11:52 PM, Günther Noack wrote:
> Hello!
> 
> I see that this test is adapted from the network_access_rights test in
> net_test.c, and some of the subsequent are similarly copied from there.  It
> makes it hard to criticize the code, because being a little bit consistent is
> probably a good thing.  Have you found any opportunities to extract
> commonalities into common.h?

I think that all common tests should be extracted to common.h or maybe
some new header. *_test.c could maintain a fixture for these tests for
some rule-specific logic. Such refactoring should be in separate patch
though.

> 	
> On Fri, May 24, 2024 at 05:30:07PM +0800, Mikhail Ivanov wrote:
>> Add test that checks possibility of adding rule with every possible
>> access right.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Formats code with clang-format.
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 4c51f89ed578..eb5d62263460 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -178,4 +178,32 @@ TEST_F(protocol, create)
>>   	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>>   }
>>   
>> +TEST_F(protocol, socket_access_rights)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = ACCESS_ALL,
>> +	};
>> +	struct landlock_socket_attr protocol = {
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +	int ruleset_fd;
>> +	__u64 access;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
>> +		protocol.allowed_access = access;
>> +		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					       &protocol, 0))
>> +		{
>> +			TH_LOG("Failed to add rule with access 0x%llx: %s",
>> +			       access, strerror(errno));
>> +		}
>> +	}
>> +	EXPECT_EQ(0, close(ruleset_fd));
> 
> Reviewed-by: Günther Noack <gnoack@google.com>
> 
> P.S. We are inconsistent with our use of EXPECT/ASSERT for test teardown.  The
> fs_test.c uses ASSERT_EQ in these places whereas net_test.c and your new tests
> use EXPECT_EQ.
> 
> It admittedly does not make much of a difference for close(), so should be OK.
> Some other selftests are even ignoring the result for close().  If we want to
> make it consistent in the Landlock tests again, we can also do it in an
> independent sweep.
> 
> I filed a small cleanup task as a reminder:
> https://github.com/landlock-lsm/linux/issues/31
> 
> —Günther
diff mbox series

Patch

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 4c51f89ed578..eb5d62263460 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -178,4 +178,32 @@  TEST_F(protocol, create)
 	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
 }
 
+TEST_F(protocol, socket_access_rights)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = ACCESS_ALL,
+	};
+	struct landlock_socket_attr protocol = {
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+	int ruleset_fd;
+	__u64 access;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
+		protocol.allowed_access = access;
+		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					       &protocol, 0))
+		{
+			TH_LOG("Failed to add rule with access 0x%llx: %s",
+			       access, strerror(errno));
+		}
+	}
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
 TEST_HARNESS_MAIN