Message ID | 20220309134459.6448-11-konstantin.meskhidze@huawei.com (mailing list archive) |
---|---|
State | RFC |
Headers | show |
Series | Landlock LSM | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Not a local patch, async |
You need to update tools/testing/selftests/landlock/config to enable CONFIG_NET and CONFIG_INET. On 09/03/2022 14:44, Konstantin Meskhidze wrote: > Adds two selftests for bind socket action. > The one is with no landlock restrictions: > - bind_no_restrictions; > The second one is with mixed landlock rules: > - bind_with_restrictions; Some typos (that propagated to all selftest patches): selftest/landlock: Add tests for bind hook Add two tests for bind socket actions: - bind_no_restrictions - bind_with_restrictions > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > --- > > Changes since v3: > * Split commit. > * Add helper create_socket. > * Add FIXTURE_SETUP. > > --- > .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++ > 1 file changed, 153 insertions(+) > create mode 100644 tools/testing/selftests/landlock/network_test.c > > diff --git a/tools/testing/selftests/landlock/network_test.c b/tools/testing/selftests/landlock/network_test.c > new file mode 100644 > index 000000000000..4c60f6d973a8 > --- /dev/null > +++ b/tools/testing/selftests/landlock/network_test.c For consistency, please rename to net_test.c > @@ -0,0 +1,153 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Landlock tests - Network > + * > + * Copyright (C) 2022 Huawei Tech. Co., Ltd. > + * Author: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > + * > + */ > + > +#define _GNU_SOURCE > +#include <arpa/inet.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <linux/landlock.h> > +#include <netinet/in.h> > +#include <string.h> > +#include <sys/prctl.h> > +#include <sys/socket.h> > +#include <sys/types.h> > + > +#include "common.h" > + > +#define MAX_SOCKET_NUM 10 > + > +#define SOCK_PORT_START 3470 > +#define SOCK_PORT_ADD 10 > + > +#define IP_ADDRESS "127.0.0.1" > + > +uint port[MAX_SOCKET_NUM]; > +struct sockaddr_in addr[MAX_SOCKET_NUM]; > + > +const int one = 1; > + > +/* Number pending connections queue to be hold */ > +#define BACKLOG 10 > + > +static int create_socket(struct __test_metadata *const _metadata) > +{ > + > + int sockfd; > + > + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); > + ASSERT_LE(0, sockfd); > + /* Allows to reuse of local address */ > + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one))); Why is it required? > + > + return sockfd; > +} > + > +static void enforce_ruleset(struct __test_metadata *const _metadata, > + const int ruleset_fd) > +{ > + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); > + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { > + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); > + } > +} > + > +FIXTURE(socket) { }; You should pick another more meaningful name. > + > +FIXTURE_SETUP(socket) > +{ > + int i; > + /* Creates socket addresses */ > + for (i = 0; i < MAX_SOCKET_NUM; i++) { > + port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; > + addr[i].sin_family = AF_INET; > + addr[i].sin_port = htons(port[i]); > + addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS); > + memset(&(addr[i].sin_zero), '\0', 8); > + } This is the right place to set up network namespace. It will make tests non-flaky. > +} > + > +FIXTURE_TEARDOWN(socket) > +{ } > + > +TEST_F_FORK(socket, bind_no_restrictions) { > + > + int sockfd; > + > + sockfd = create_socket(_metadata); > + ASSERT_LE(0, sockfd); > + > + /* Binds a socket to port[0] */ > + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&addr[0], sizeof(addr[0]))); > + > + ASSERT_EQ(0, close(sockfd)); > +} > + > +TEST_F_FORK(socket, bind_with_restrictions) { > + > + int sockfd_1, sockfd_2, sockfd_3; Do you really need to have 3 opened socket at the same time? > + > + struct landlock_ruleset_attr ruleset_attr = { > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | > + LANDLOCK_ACCESS_NET_CONNECT_TCP, > + }; > + struct landlock_net_service_attr net_service_1 = { > + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | > + LANDLOCK_ACCESS_NET_CONNECT_TCP, > + .port = port[0], > + }; > + struct landlock_net_service_attr net_service_2 = { > + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, > + .port = port[1], > + }; > + struct landlock_net_service_attr net_service_3 = { > + .allowed_access = 0, > + .port = port[2], > + }; > + > + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, > + sizeof(ruleset_attr), 0); > + ASSERT_LE(0, ruleset_fd); > + > + /* Allows connect and bind operations to the port[0] socket. */ > + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_1, 0)); > + /* Allows connect and deny bind operations to the port[1] socket. */ > + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_2, 0)); > + /* Empty allowed_access (i.e. deny rules) are ignored in network actions > + * for port[2] socket. > + */ > + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_3, 0)); > + ASSERT_EQ(ENOMSG, errno); > + > + /* Enforces the ruleset. */ > + enforce_ruleset(_metadata, ruleset_fd); > + > + sockfd_1 = create_socket(_metadata); > + ASSERT_LE(0, sockfd_1); > + /* Binds a socket to port[0] */ > + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr[0], sizeof(addr[0]))); > + > + /* Close bounded socket*/ > + ASSERT_EQ(0, close(sockfd_1)); > + > + sockfd_2 = create_socket(_metadata); > + ASSERT_LE(0, sockfd_2); > + /* Binds a socket to port[1] */ > + ASSERT_EQ(-1, bind(sockfd_2, (struct sockaddr *)&addr[1], sizeof(addr[1]))); > + ASSERT_EQ(EACCES, errno); > + > + sockfd_3 = create_socket(_metadata); > + ASSERT_LE(0, sockfd_3); > + /* Binds a socket to port[2] */ > + ASSERT_EQ(-1, bind(sockfd_3, (struct sockaddr *)&addr[2], sizeof(addr[2]))); > + ASSERT_EQ(EACCES, errno); > +} > +TEST_HARNESS_MAIN > -- > 2.25.1 >
4/1/2022 7:52 PM, Mickaël Salaün пишет: > You need to update tools/testing/selftests/landlock/config to enable > CONFIG_NET and CONFIG_INET. > > > On 09/03/2022 14:44, Konstantin Meskhidze wrote: >> Adds two selftests for bind socket action. >> The one is with no landlock restrictions: >> - bind_no_restrictions; >> The second one is with mixed landlock rules: >> - bind_with_restrictions; > > Some typos (that propagated to all selftest patches): > > selftest/landlock: Add tests for bind hook > > Add two tests for bind socket actions: > - bind_no_restrictions > - bind_with_restrictions > Got it. I will fix it. Thanks. > > >> >> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >> --- >> >> Changes since v3: >> * Split commit. >> * Add helper create_socket. >> * Add FIXTURE_SETUP. >> >> --- >> .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++ >> 1 file changed, 153 insertions(+) >> create mode 100644 tools/testing/selftests/landlock/network_test.c >> >> diff --git a/tools/testing/selftests/landlock/network_test.c >> b/tools/testing/selftests/landlock/network_test.c >> new file mode 100644 >> index 000000000000..4c60f6d973a8 >> --- /dev/null >> +++ b/tools/testing/selftests/landlock/network_test.c > > For consistency, please rename to net_test.c > Ok. I will. >> @@ -0,0 +1,153 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Landlock tests - Network >> + * >> + * Copyright (C) 2022 Huawei Tech. Co., Ltd. >> + * Author: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >> + * >> + */ >> + >> +#define _GNU_SOURCE >> +#include <arpa/inet.h> >> +#include <errno.h> >> +#include <fcntl.h> >> +#include <linux/landlock.h> >> +#include <netinet/in.h> >> +#include <string.h> >> +#include <sys/prctl.h> >> +#include <sys/socket.h> >> +#include <sys/types.h> >> + >> +#include "common.h" >> + >> +#define MAX_SOCKET_NUM 10 >> + >> +#define SOCK_PORT_START 3470 >> +#define SOCK_PORT_ADD 10 >> + >> +#define IP_ADDRESS "127.0.0.1" >> + >> +uint port[MAX_SOCKET_NUM]; >> +struct sockaddr_in addr[MAX_SOCKET_NUM]; >> + >> +const int one = 1; >> + >> +/* Number pending connections queue to be hold */ >> +#define BACKLOG 10 >> + >> +static int create_socket(struct __test_metadata *const _metadata) >> +{ >> + >> + int sockfd; >> + >> + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >> + ASSERT_LE(0, sockfd); >> + /* Allows to reuse of local address */ >> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, >> &one, sizeof(one))); > > Why is it required? Without SO_REUSEADDR there is an error that a socket's port is in use. > >> + >> + return sockfd; >> +} >> + >> +static void enforce_ruleset(struct __test_metadata *const _metadata, >> + const int ruleset_fd) >> +{ >> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { >> + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); >> + } >> +} >> + >> +FIXTURE(socket) { }; > > You should pick another more meaningful name. Ok. I will change it. Thanks. > >> + >> +FIXTURE_SETUP(socket) >> +{ >> + int i; >> + /* Creates socket addresses */ >> + for (i = 0; i < MAX_SOCKET_NUM; i++) { >> + port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >> + addr[i].sin_family = AF_INET; >> + addr[i].sin_port = htons(port[i]); >> + addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr[i].sin_zero), '\0', 8); >> + } > > This is the right place to set up network namespace. It will make tests > non-flaky. > Thanks. >> +} >> + >> +FIXTURE_TEARDOWN(socket) >> +{ } >> + >> +TEST_F_FORK(socket, bind_no_restrictions) { >> + >> + int sockfd; >> + >> + sockfd = create_socket(_metadata); >> + ASSERT_LE(0, sockfd); >> + >> + /* Binds a socket to port[0] */ >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&addr[0], >> sizeof(addr[0]))); >> + >> + ASSERT_EQ(0, close(sockfd)); >> +} >> + >> +TEST_F_FORK(socket, bind_with_restrictions) { >> + >> + int sockfd_1, sockfd_2, sockfd_3; > > Do you really need to have 3 opened socket at the same time? I just wanted to "kill two birds with one stone" in this test. It possible to split it in 3 tests and open just one socket in each one. > >> + >> + struct landlock_ruleset_attr ruleset_attr = { >> + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + }; >> + struct landlock_net_service_attr net_service_1 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = port[0], >> + }; >> + struct landlock_net_service_attr net_service_2 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = port[1], >> + }; >> + struct landlock_net_service_attr net_service_3 = { >> + .allowed_access = 0, >> + .port = port[2], >> + }; >> + >> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> + sizeof(ruleset_attr), 0); >> + ASSERT_LE(0, ruleset_fd); >> + >> + /* Allows connect and bind operations to the port[0] socket. */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_1, 0)); >> + /* Allows connect and deny bind operations to the port[1] socket. */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_2, 0)); >> + /* Empty allowed_access (i.e. deny rules) are ignored in network >> actions >> + * for port[2] socket. >> + */ >> + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_3, 0)); >> + ASSERT_EQ(ENOMSG, errno); >> + >> + /* Enforces the ruleset. */ >> + enforce_ruleset(_metadata, ruleset_fd); >> + >> + sockfd_1 = create_socket(_metadata); >> + ASSERT_LE(0, sockfd_1); >> + /* Binds a socket to port[0] */ >> + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr[0], >> sizeof(addr[0]))); >> + >> + /* Close bounded socket*/ >> + ASSERT_EQ(0, close(sockfd_1)); >> + >> + sockfd_2 = create_socket(_metadata); >> + ASSERT_LE(0, sockfd_2); >> + /* Binds a socket to port[1] */ >> + ASSERT_EQ(-1, bind(sockfd_2, (struct sockaddr *)&addr[1], >> sizeof(addr[1]))); >> + ASSERT_EQ(EACCES, errno); >> + >> + sockfd_3 = create_socket(_metadata); >> + ASSERT_LE(0, sockfd_3); >> + /* Binds a socket to port[2] */ >> + ASSERT_EQ(-1, bind(sockfd_3, (struct sockaddr *)&addr[2], >> sizeof(addr[2]))); >> + ASSERT_EQ(EACCES, errno); >> +} >> +TEST_HARNESS_MAIN >> -- >> 2.25.1 >> > > .
On 04/04/2022 10:28, Konstantin Meskhidze wrote: > > > 4/1/2022 7:52 PM, Mickaël Salaün пишет: [...] >>> +static int create_socket(struct __test_metadata *const _metadata) >>> +{ >>> + >>> + int sockfd; >>> + >>> + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >>> + ASSERT_LE(0, sockfd); >>> + /* Allows to reuse of local address */ >>> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, >>> &one, sizeof(one))); >> >> Why is it required? > > Without SO_REUSEADDR there is an error that a socket's port is in use. I'm sure there is, but why is this port reused? I think this means that there is an issue in the tests and that could hide potential issue with the tests (and then with the kernel code). Could you investigate and find the problem? This would make these tests reliable. Without removing the need to find this issue, the next series should use a network namespace per test, which will confine such issue from other tests and the host. [...] >>> +TEST_F_FORK(socket, bind_with_restrictions) { >>> + >>> + int sockfd_1, sockfd_2, sockfd_3; >> >> Do you really need to have 3 opened socket at the same time? > > I just wanted to "kill two birds with one stone" in this test. > It possible to split it in 3 tests and open just one socket in each one. I wanted to point out that these three variables could be replaced with only one (taking into account that successful opened socket are closed before the variable is reused). It may not be obvious if we need to split a test into multiple. The rules I try to follow are: - use a consistent Landlock rule setup, with potentially nested rules, to test specific edge cases; - don't tamper the context of a test (e.g. with FS topology/layout modification or network used port) unless it is clearly documented and easy to spot, but be careful about the dependent tests; - don't make tests too long unless it makes sense for a specific scenario. >> >>> + >>> + struct landlock_ruleset_attr ruleset_attr = { >>> + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >>> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >>> + }; >>> + struct landlock_net_service_attr net_service_1 = { >>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | >>> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >>> + .port = port[0], >>> + }; >>> + struct landlock_net_service_attr net_service_2 = { >>> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >>> + .port = port[1], >>> + }; >>> + struct landlock_net_service_attr net_service_3 = { >>> + .allowed_access = 0, >>> + .port = port[2], >>> + }; >>> + >>> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >>> + sizeof(ruleset_attr), 0); >>> + ASSERT_LE(0, ruleset_fd); >>> + >>> + /* Allows connect and bind operations to the port[0] socket. */ >>> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >>> LANDLOCK_RULE_NET_SERVICE, >>> + &net_service_1, 0)); >>> + /* Allows connect and deny bind operations to the port[1] >>> socket. */ >>> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >>> LANDLOCK_RULE_NET_SERVICE, >>> + &net_service_2, 0)); >>> + /* Empty allowed_access (i.e. deny rules) are ignored in network >>> actions >>> + * for port[2] socket. >>> + */ >>> + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, >>> LANDLOCK_RULE_NET_SERVICE, >>> + &net_service_3, 0)); >>> + ASSERT_EQ(ENOMSG, errno); >>> + >>> + /* Enforces the ruleset. */ >>> + enforce_ruleset(_metadata, ruleset_fd); >>> + >>> + sockfd_1 = create_socket(_metadata); >>> + ASSERT_LE(0, sockfd_1); >>> + /* Binds a socket to port[0] */ >>> + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr[0], >>> sizeof(addr[0]))); >>> + >>> + /* Close bounded socket*/ >>> + ASSERT_EQ(0, close(sockfd_1)); >>> + >>> + sockfd_2 = create_socket(_metadata); >>> + ASSERT_LE(0, sockfd_2); >>> + /* Binds a socket to port[1] */ >>> + ASSERT_EQ(-1, bind(sockfd_2, (struct sockaddr *)&addr[1], >>> sizeof(addr[1]))); >>> + ASSERT_EQ(EACCES, errno); >>> + >>> + sockfd_3 = create_socket(_metadata); >>> + ASSERT_LE(0, sockfd_3); >>> + /* Binds a socket to port[2] */ >>> + ASSERT_EQ(-1, bind(sockfd_3, (struct sockaddr *)&addr[2], >>> sizeof(addr[2]))); >>> + ASSERT_EQ(EACCES, errno); >>> +} >>> +TEST_HARNESS_MAIN >>> -- >>> 2.25.1 >>> >> >> .
On 09/03/2022 14:44, Konstantin Meskhidze wrote: > Adds two selftests for bind socket action. > The one is with no landlock restrictions: > - bind_no_restrictions; > The second one is with mixed landlock rules: > - bind_with_restrictions; > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > --- > > Changes since v3: > * Split commit. > * Add helper create_socket. > * Add FIXTURE_SETUP. > > --- > .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++ > 1 file changed, 153 insertions(+) > create mode 100644 tools/testing/selftests/landlock/network_test.c > > diff --git a/tools/testing/selftests/landlock/network_test.c b/tools/testing/selftests/landlock/network_test.c > new file mode 100644 > index 000000000000..4c60f6d973a8 > --- /dev/null > +++ b/tools/testing/selftests/landlock/network_test.c [...] > + > +uint port[MAX_SOCKET_NUM]; > +struct sockaddr_in addr[MAX_SOCKET_NUM]; You should not change global variables, it is a source of issue. Instead use FIXTURE local variables accessible through self->X. > + > +const int one = 1; This doesn't need to be global. [...] > + > +static void enforce_ruleset(struct __test_metadata *const _metadata, > + const int ruleset_fd) > +{ > + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); > + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { > + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); > + } > +} You should move the same helper from fs_base.c to common.h (see caps helpers) and reuse it here. > + > +FIXTURE(socket) { }; > + > +FIXTURE_SETUP(socket) > +{ > + int i; Please add a new line between declaration and actual code (everywhere). > + /* Creates socket addresses */ > + for (i = 0; i < MAX_SOCKET_NUM; i++) { Use ARRAY_SIZE() instead of MAY_SOCKET_NUM. > + port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; Use self->port[i] and self->addr[i] instead. > + addr[i].sin_family = AF_INET; > + addr[i].sin_port = htons(port[i]); > + addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS); > + memset(&(addr[i].sin_zero), '\0', 8); > + } > +} [...] > + /* Allows connect and deny bind operations to the port[1] socket. */ > + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_2, 0)); > + /* Empty allowed_access (i.e. deny rules) are ignored in network actions The kernel coding style says to start a multi-line comments with a "/*" and a new line.
4/4/2022 12:44 PM, Mickaël Salaün пишет: > > On 04/04/2022 10:28, Konstantin Meskhidze wrote: >> >> >> 4/1/2022 7:52 PM, Mickaël Salaün пишет: > > [...] > >>>> +static int create_socket(struct __test_metadata *const _metadata) >>>> +{ >>>> + >>>> + int sockfd; >>>> + >>>> + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >>>> + ASSERT_LE(0, sockfd); >>>> + /* Allows to reuse of local address */ >>>> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, >>>> &one, sizeof(one))); >>> >>> Why is it required? >> >> Without SO_REUSEADDR there is an error that a socket's port is in use. > > I'm sure there is, but why is this port reused? I think this means that > there is an issue in the tests and that could hide potential issue with > the tests (and then with the kernel code). Could you investigate and > find the problem? This would make these tests reliable. The next scenario is possible here: "In order for a network connection to close, both ends have to send FIN (final) packets, which indicate they will not send any additional data, and both ends must ACK (acknowledge) each other's FIN packets. The FIN packets are initiated by the application performing a close(), a shutdown(), or an exit(). The ACKs are handled by the kernel after the close() has completed. Because of this, it is possible for the process to complete before the kernel has released the associated network resource, and this port cannot be bound to another process until the kernel has decided that it is done." https://hea-www.harvard.edu/~fine/Tech/addrinuse.html. So in this case we have busy port in network selfttest and one of the solution is to set SO_REUSEADDR socket option, "which explicitly allows a process to bind to a port which remains in TIME_WAIT (it still only allows a single process to be bound to that port). This is the both the simplest and the most effective option for reducing the "address already in use" error". > > Without removing the need to find this issue, the next series should use > a network namespace per test, which will confine such issue from other > tests and the host. So there are 2 options here: 1. Using SO_REUSEADDR option 2. Using network namespace. I prefer the first option - "the simplest and the most effective one" > > [...] > >>>> +TEST_F_FORK(socket, bind_with_restrictions) { >>>> + >>>> + int sockfd_1, sockfd_2, sockfd_3; >>> >>> Do you really need to have 3 opened socket at the same time? >> >> I just wanted to "kill two birds with one stone" in this test. >> It possible to split it in 3 tests and open just one socket in each >> one. > > I wanted to point out that these three variables could be replaced with > only one (taking into account that successful opened socket are closed > before the variable is reused). > > It may not be obvious if we need to split a test into multiple. The > rules I try to follow are: > - use a consistent Landlock rule setup, with potentially nested rules, > to test specific edge cases; > - don't tamper the context of a test (e.g. with FS topology/layout > modification or network used port) unless it is clearly documented and > easy to spot, but be careful about the dependent tests; > - don't make tests too long unless it makes sense for a specific scenario. > Ok. I got your point here. Thanks. > >>> >>>> + >>>> + struct landlock_ruleset_attr ruleset_attr = { >>>> + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >>>> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >>>> + }; >>>> + struct landlock_net_service_attr net_service_1 = { >>>> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | >>>> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >>>> + .port = port[0], >>>> + }; >>>> + struct landlock_net_service_attr net_service_2 = { >>>> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >>>> + .port = port[1], >>>> + }; >>>> + struct landlock_net_service_attr net_service_3 = { >>>> + .allowed_access = 0, >>>> + .port = port[2], >>>> + }; >>>> + >>>> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >>>> + sizeof(ruleset_attr), 0); >>>> + ASSERT_LE(0, ruleset_fd); >>>> + >>>> + /* Allows connect and bind operations to the port[0] socket. */ >>>> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >>>> LANDLOCK_RULE_NET_SERVICE, >>>> + &net_service_1, 0)); >>>> + /* Allows connect and deny bind operations to the port[1] >>>> socket. */ >>>> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >>>> LANDLOCK_RULE_NET_SERVICE, >>>> + &net_service_2, 0)); >>>> + /* Empty allowed_access (i.e. deny rules) are ignored in >>>> network actions >>>> + * for port[2] socket. >>>> + */ >>>> + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, >>>> LANDLOCK_RULE_NET_SERVICE, >>>> + &net_service_3, 0)); >>>> + ASSERT_EQ(ENOMSG, errno); >>>> + >>>> + /* Enforces the ruleset. */ >>>> + enforce_ruleset(_metadata, ruleset_fd); >>>> + >>>> + sockfd_1 = create_socket(_metadata); >>>> + ASSERT_LE(0, sockfd_1); >>>> + /* Binds a socket to port[0] */ >>>> + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr[0], >>>> sizeof(addr[0]))); >>>> + >>>> + /* Close bounded socket*/ >>>> + ASSERT_EQ(0, close(sockfd_1)); >>>> + >>>> + sockfd_2 = create_socket(_metadata); >>>> + ASSERT_LE(0, sockfd_2); >>>> + /* Binds a socket to port[1] */ >>>> + ASSERT_EQ(-1, bind(sockfd_2, (struct sockaddr *)&addr[1], >>>> sizeof(addr[1]))); >>>> + ASSERT_EQ(EACCES, errno); >>>> + >>>> + sockfd_3 = create_socket(_metadata); >>>> + ASSERT_LE(0, sockfd_3); >>>> + /* Binds a socket to port[2] */ >>>> + ASSERT_EQ(-1, bind(sockfd_3, (struct sockaddr *)&addr[2], >>>> sizeof(addr[2]))); >>>> + ASSERT_EQ(EACCES, errno); >>>> +} >>>> +TEST_HARNESS_MAIN >>>> -- >>>> 2.25.1 >>>> >>> >>> . > .
4/4/2022 9:32 PM, Mickaël Salaün пишет: > > On 09/03/2022 14:44, Konstantin Meskhidze wrote: >> Adds two selftests for bind socket action. >> The one is with no landlock restrictions: >> - bind_no_restrictions; >> The second one is with mixed landlock rules: >> - bind_with_restrictions; >> >> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >> --- >> >> Changes since v3: >> * Split commit. >> * Add helper create_socket. >> * Add FIXTURE_SETUP. >> >> --- >> .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++ >> 1 file changed, 153 insertions(+) >> create mode 100644 tools/testing/selftests/landlock/network_test.c >> >> diff --git a/tools/testing/selftests/landlock/network_test.c >> b/tools/testing/selftests/landlock/network_test.c >> new file mode 100644 >> index 000000000000..4c60f6d973a8 >> --- /dev/null >> +++ b/tools/testing/selftests/landlock/network_test.c > > [...] > >> + >> +uint port[MAX_SOCKET_NUM]; >> +struct sockaddr_in addr[MAX_SOCKET_NUM]; > > You should not change global variables, it is a source of issue. Instead > use FIXTURE local variables accessible through self->X. > Sorry. Did not get your point here. >> + >> +const int one = 1; > > This doesn't need to be global. Ok. Got it. > > [...] > >> + >> +static void enforce_ruleset(struct __test_metadata *const _metadata, >> + const int ruleset_fd) >> +{ >> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { >> + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); >> + } >> +} > > You should move the same helper from fs_base.c to common.h (see caps > helpers) and reuse it here. > Ok. Thanks. > >> + >> +FIXTURE(socket) { }; >> + >> +FIXTURE_SETUP(socket) >> +{ >> + int i; > > Please add a new line between declaration and actual code (everywhere). Ok. Got it. Will be refactored. > >> + /* Creates socket addresses */ >> + for (i = 0; i < MAX_SOCKET_NUM; i++) { > > Use ARRAY_SIZE() instead of MAY_SOCKET_NUM. > Ok. I got it. > >> + port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; > > Use self->port[i] and self->addr[i] instead. > Do you mean to add it in FIXTURE variables? >> + addr[i].sin_family = AF_INET; >> + addr[i].sin_port = htons(port[i]); >> + addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr[i].sin_zero), '\0', 8); >> + } >> +} > > [...] > >> + /* Allows connect and deny bind operations to the port[1] socket. */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_2, 0)); >> + /* Empty allowed_access (i.e. deny rules) are ignored in network >> actions > > The kernel coding style says to start a multi-line comments with a "/*" > and a new line. I missed it here. Thanks. > .
On 06/04/2022 16:12, Konstantin Meskhidze wrote: > > > 4/4/2022 12:44 PM, Mickaël Salaün пишет: >> >> On 04/04/2022 10:28, Konstantin Meskhidze wrote: >>> >>> >>> 4/1/2022 7:52 PM, Mickaël Salaün пишет: >> >> [...] >> >>>>> +static int create_socket(struct __test_metadata *const _metadata) >>>>> +{ >>>>> + >>>>> + int sockfd; >>>>> + >>>>> + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >>>>> + ASSERT_LE(0, sockfd); >>>>> + /* Allows to reuse of local address */ >>>>> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, >>>>> &one, sizeof(one))); >>>> >>>> Why is it required? >>> >>> Without SO_REUSEADDR there is an error that a socket's port is in >>> use. >> >> I'm sure there is, but why is this port reused? I think this means >> that there is an issue in the tests and that could hide potential >> issue with the tests (and then with the kernel code). Could you >> investigate and find the problem? This would make these tests reliable. > The next scenario is possible here: > "In order for a network connection to close, both ends have to send > FIN (final) packets, which indicate they will not send any additional > data, and both ends must ACK (acknowledge) each other's FIN packets. The > FIN packets are initiated by the application performing a close(), a > shutdown(), or an exit(). The ACKs are handled by the kernel after the > close() has completed. Because of this, it is possible for the process > to complete before the kernel has released the associated network > resource, and this port cannot be bound to another process until the > kernel has decided that it is done." > https://hea-www.harvard.edu/~fine/Tech/addrinuse.html. > > So in this case we have busy port in network selfttest and one of the > solution is to set SO_REUSEADDR socket option, "which explicitly allows > a process to bind to a port which remains in TIME_WAIT (it still only > allows a single process to be bound to that port). This is the both the > simplest and the most effective option for reducing the "address already > in use" error". In know what this option does, but I'm wondering what do you need it for these tests: which specific line requires it and why? Isn't it a side effect of running partial tests? I'm worried that this hides some issues in the tests that may make them flaky. >> >> Without removing the need to find this issue, the next series should >> use a network namespace per test, which will confine such issue from >> other tests and the host. > > So there are 2 options here: > 1. Using SO_REUSEADDR option > 2. Using network namespace. > > I prefer the first option - "the simplest and the most effective one" If SO_REUSEADDR is really required (and justified), then it should be used. Either it is required or not, we should use a dedicated network namespace for each test anyway. This enables to not mess with the host and not be impacted by it neither (e.g. if some process already use such ports). > >> >> [...]
4/8/2022 7:41 PM, Mickaël Salaün пишет: > > On 06/04/2022 16:12, Konstantin Meskhidze wrote: >> >> >> 4/4/2022 12:44 PM, Mickaël Salaün пишет: >>> >>> On 04/04/2022 10:28, Konstantin Meskhidze wrote: >>>> >>>> >>>> 4/1/2022 7:52 PM, Mickaël Salaün пишет: >>> >>> [...] >>> >>>>>> +static int create_socket(struct __test_metadata *const _metadata) >>>>>> +{ >>>>>> + >>>>>> + int sockfd; >>>>>> + >>>>>> + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >>>>>> + ASSERT_LE(0, sockfd); >>>>>> + /* Allows to reuse of local address */ >>>>>> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, >>>>>> &one, sizeof(one))); >>>>> >>>>> Why is it required? >>>> >>>> Without SO_REUSEADDR there is an error that a socket's port is in >>>> use. >>> >>> I'm sure there is, but why is this port reused? I think this means >>> that there is an issue in the tests and that could hide potential >>> issue with the tests (and then with the kernel code). Could you >>> investigate and find the problem? This would make these tests reliable. >> The next scenario is possible here: >> "In order for a network connection to close, both ends have to send >> FIN (final) packets, which indicate they will not send any additional >> data, and both ends must ACK (acknowledge) each other's FIN packets. >> The FIN packets are initiated by the application performing a close(), >> a shutdown(), or an exit(). The ACKs are handled by the kernel after >> the close() has completed. Because of this, it is possible for the >> process to complete before the kernel has released the associated >> network resource, and this port cannot be bound to another process >> until the kernel has decided that it is done." >> https://hea-www.harvard.edu/~fine/Tech/addrinuse.html. >> >> So in this case we have busy port in network selfttest and one of the >> solution is to set SO_REUSEADDR socket option, "which explicitly >> allows a process to bind to a port which remains in TIME_WAIT (it >> still only allows a single process to be bound to that port). This is >> the both the simplest and the most effective option for reducing the >> "address already in use" error". > > In know what this option does, but I'm wondering what do you need it for > these tests: which specific line requires it and why? Isn't it a side > effect of running partial tests? I'm worried that this hides some issues > in the tests that may make them flaky. > I need it cause we have a possibility here that process (launching tests) has to wait the kernel's releasing the associated network socket after closing it. > >>> >>> Without removing the need to find this issue, the next series should >>> use a network namespace per test, which will confine such issue from >>> other tests and the host. >> >> So there are 2 options here: >> 1. Using SO_REUSEADDR option >> 2. Using network namespace. >> >> I prefer the first option - "the simplest and the most effective one" > > If SO_REUSEADDR is really required (and justified), then it should be > used. Either it is required or not, we should use a dedicated network > namespace for each test anyway. This enables to not mess with the host > and not be impacted by it neither (e.g. if some process already use such > ports). > Ok. I update the code. > >> >>> >>> [...] > .
On 01/04/2022 18:52, Mickaël Salaün wrote: > You need to update tools/testing/selftests/landlock/config to enable > CONFIG_NET and CONFIG_INET. > > > On 09/03/2022 14:44, Konstantin Meskhidze wrote: >> Adds two selftests for bind socket action. >> The one is with no landlock restrictions: >> - bind_no_restrictions; >> The second one is with mixed landlock rules: >> - bind_with_restrictions; > > Some typos (that propagated to all selftest patches): > > selftest/landlock: Add tests for bind hook I did some typo myself, it should be "selftests/landlock:"
5/16/2022 1:10 PM, Mickaël Salaün пишет: > > On 01/04/2022 18:52, Mickaël Salaün wrote: >> You need to update tools/testing/selftests/landlock/config to enable >> CONFIG_NET and CONFIG_INET. >> >> >> On 09/03/2022 14:44, Konstantin Meskhidze wrote: >>> Adds two selftests for bind socket action. >>> The one is with no landlock restrictions: >>> - bind_no_restrictions; >>> The second one is with mixed landlock rules: >>> - bind_with_restrictions; >> >> Some typos (that propagated to all selftest patches): >> >> selftest/landlock: Add tests for bind hook > > I did some typo myself, it should be "selftests/landlock:" > . Thanks, cause I was ready to send a patch V5. I will fix it.
diff --git a/tools/testing/selftests/landlock/network_test.c b/tools/testing/selftests/landlock/network_test.c new file mode 100644 index 000000000000..4c60f6d973a8 --- /dev/null +++ b/tools/testing/selftests/landlock/network_test.c @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Landlock tests - Network + * + * Copyright (C) 2022 Huawei Tech. Co., Ltd. + * Author: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> + * + */ + +#define _GNU_SOURCE +#include <arpa/inet.h> +#include <errno.h> +#include <fcntl.h> +#include <linux/landlock.h> +#include <netinet/in.h> +#include <string.h> +#include <sys/prctl.h> +#include <sys/socket.h> +#include <sys/types.h> + +#include "common.h" + +#define MAX_SOCKET_NUM 10 + +#define SOCK_PORT_START 3470 +#define SOCK_PORT_ADD 10 + +#define IP_ADDRESS "127.0.0.1" + +uint port[MAX_SOCKET_NUM]; +struct sockaddr_in addr[MAX_SOCKET_NUM]; + +const int one = 1; + +/* Number pending connections queue to be hold */ +#define BACKLOG 10 + +static int create_socket(struct __test_metadata *const _metadata) +{ + + int sockfd; + + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); + ASSERT_LE(0, sockfd); + /* Allows to reuse of local address */ + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one))); + + return sockfd; +} + +static void enforce_ruleset(struct __test_metadata *const _metadata, + const int ruleset_fd) +{ + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); + } +} + +FIXTURE(socket) { }; + +FIXTURE_SETUP(socket) +{ + int i; + /* Creates socket addresses */ + for (i = 0; i < MAX_SOCKET_NUM; i++) { + port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; + addr[i].sin_family = AF_INET; + addr[i].sin_port = htons(port[i]); + addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS); + memset(&(addr[i].sin_zero), '\0', 8); + } +} + +FIXTURE_TEARDOWN(socket) +{ } + +TEST_F_FORK(socket, bind_no_restrictions) { + + int sockfd; + + sockfd = create_socket(_metadata); + ASSERT_LE(0, sockfd); + + /* Binds a socket to port[0] */ + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&addr[0], sizeof(addr[0]))); + + ASSERT_EQ(0, close(sockfd)); +} + +TEST_F_FORK(socket, bind_with_restrictions) { + + int sockfd_1, sockfd_2, sockfd_3; + + struct landlock_ruleset_attr ruleset_attr = { + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | + LANDLOCK_ACCESS_NET_CONNECT_TCP, + }; + struct landlock_net_service_attr net_service_1 = { + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | + LANDLOCK_ACCESS_NET_CONNECT_TCP, + .port = port[0], + }; + struct landlock_net_service_attr net_service_2 = { + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, + .port = port[1], + }; + struct landlock_net_service_attr net_service_3 = { + .allowed_access = 0, + .port = port[2], + }; + + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, + sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + /* Allows connect and bind operations to the port[0] socket. */ + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_1, 0)); + /* Allows connect and deny bind operations to the port[1] socket. */ + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_2, 0)); + /* Empty allowed_access (i.e. deny rules) are ignored in network actions + * for port[2] socket. + */ + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_3, 0)); + ASSERT_EQ(ENOMSG, errno); + + /* Enforces the ruleset. */ + enforce_ruleset(_metadata, ruleset_fd); + + sockfd_1 = create_socket(_metadata); + ASSERT_LE(0, sockfd_1); + /* Binds a socket to port[0] */ + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr[0], sizeof(addr[0]))); + + /* Close bounded socket*/ + ASSERT_EQ(0, close(sockfd_1)); + + sockfd_2 = create_socket(_metadata); + ASSERT_LE(0, sockfd_2); + /* Binds a socket to port[1] */ + ASSERT_EQ(-1, bind(sockfd_2, (struct sockaddr *)&addr[1], sizeof(addr[1]))); + ASSERT_EQ(EACCES, errno); + + sockfd_3 = create_socket(_metadata); + ASSERT_LE(0, sockfd_3); + /* Binds a socket to port[2] */ + ASSERT_EQ(-1, bind(sockfd_3, (struct sockaddr *)&addr[2], sizeof(addr[2]))); + ASSERT_EQ(EACCES, errno); +} +TEST_HARNESS_MAIN
Adds two selftests for bind socket action. The one is with no landlock restrictions: - bind_no_restrictions; The second one is with mixed landlock rules: - bind_with_restrictions; Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> --- Changes since v3: * Split commit. * Add helper create_socket. * Add FIXTURE_SETUP. --- .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 tools/testing/selftests/landlock/network_test.c -- 2.25.1