Code refactoring - Githubissues

sm1ling-knight commented 5 months ago

We can try to improve landlock code structure and extensibility, remove repetitive patterns.

As @gnoack suggested it'll be effective to gather refactoring ideas before implementing the patch itself. It'll probably be convenient to suggest any ideas in the comments on this issue so that they can be discussed and approved or rejected here. All approved ideas can be marked in the description of this issue.

After collecting ideas, someone can be assigned to implement the appropriate patch.

sm1ling-knight commented 5 months ago

Note that #1 is related to refactoring the internal structures of Landlock, but probably the implementation of more efficient data structure for domains should be a separate patch.

sm1ling-knight commented 5 months ago

Some of the common helpers should be inlined if it's not possible to generalize them [1].

[1] https://lore.kernel.org/all/ZmCf9JVIXmRZrCWk@google.com/.

sm1ling-knight commented 2 months ago

Hello @l0kod, @gnoack! Please take a look at the following issue. It should be discussed before I send fix for the https://github.com/landlock-lsm/linux/issues/40.

Objective

Refactor protocol.* net tests. Separate conditionals from the logic of the tests.

Motivation

As @gnoack noted putting conditionals in the test is often considered bad practice (e.g.). Tests are becoming more complicated, harder to read and extend.

For example, this part of test_bind_and_connect() helper simply performs connect(2) and checks corresponding return value.

static void test_bind_and_connect(struct __test_metadata *const _metadata,
                  const struct service_fixture *const srv,
                  const bool deny_bind, const bool deny_connect)
[...]
    ret = connect_variant_addrlen(inval_fd, srv, get_addrlen(srv, true));
    if (srv->protocol.domain == AF_UNIX) {
        EXPECT_EQ(-EINVAL, ret);
    } else if (deny_connect) {
        EXPECT_EQ(-EACCES, ret);
    } else if (srv->protocol.type == SOCK_STREAM) {
        /* No listening server, whatever the value of deny_bind. */
        EXPECT_EQ(-ECONNREFUSED, ret);
    } else {
        EXPECT_EQ(0, ret)
        {
            TH_LOG("Failed to connect to socket: %s",
                   strerror(errno));
        }
    }
[...]

This problem has already appeared in two patches that I'm working on:

When extending the protocol.* tests to add support for LANDLOCK_ACCESS_NET_LISTEN_TCP.
When extending the fixture protocol with protocols that have address family AF_INET and socket type SOCK_STREAM (specifically IPPROTO_SCTP, IPPROTO_SMC, IPPROTO_MPTCP). This is needed for patch-fix of mishandling non-TCP protocols.

Both cases lead to the addition of new conditionals to existing tests, which negatively affects complexity and readability.

Proposed solution

Refactor protocol.* net tests by making a separate test for each set of protocols that have the same behavior in the original test. Use conditionals in TEST_F() to choose appropriate test to run.
Split test_bind_and_connect() in a few independent helpers. I've already proposed one way to do this.

Outline of changes

For example, protocol.bind test

TEST_F(protocol, bind)
{
[...]
    test_bind_and_connect(_metadata, &self->srv0, false, false);

    test_bind_and_connect(_metadata, &self->srv1, 
            is_restricted(&variant->prot, variant->sandbox), false);

    test_bind_and_connect(_metadata, &self->srv2,
                  is_restricted(&variant->prot, variant->sandbox),
                  is_restricted(&variant->prot, variant->sandbox));
}

can be refactored like that:


static void test_tcp_stream_allowed_bind_allowed_connect(
    struct __test_metadata *const _metadata,
    const struct service_fixture *const srv)
{
    EXPECT_EQ(0, test_bind_variant(_metadata, srv));
    EXPECT_EQ(-ECONNREFUSED, test_connect_variant(_metadata, srv));

    /* Simple server-client connection trial. */
    test_allowed_connection(_metadata, srv);
}

static void test_tcp_stream_denied_bind_allowed_connect(
    struct __test_metadata *const _metadata,
    const struct service_fixture *const srv)
{
    EXPECT_EQ(-EACCES, test_bind_variant(_metadata, srv));
    EXPECT_EQ(-ECONNREFUSED, test_connect_variant(_metadata, srv));
}

static void test_tcp_stream_allowed_bind_denied_connect(
    struct __test_metadata *const _metadata,
    const struct service_fixture *const srv)
{
    EXPECT_EQ(0, test_bind_variant(_metadata, srv));
    EXPECT_EQ(-EACCES, test_connect_variant(_metadata, srv));
    test_denied_connection(_metadata, srv);
}

static void test_tcp_stream_denied_bind_denied_connect(
    struct __test_metadata *const _metadata,
    const struct service_fixture *const srv)
{
    EXPECT_EQ(-EACCES, test_bind_variant(_metadata, srv));
    EXPECT_EQ(-EACCES, test_connect_variant(_metadata, srv));
}

static void
test_not_restricted_stream_bind_connect(struct __test_metadata *const _metadata,
                 const struct service_fixture *const srv)
{
    EXPECT_EQ(0, test_bind_variant(_metadata, srv));
    EXPECT_EQ(-ECONNREFUSED, test_connect_variant(_metadata, srv));
    test_allowed_connection(_metadata, srv);
}

static void
test_non_stream_bind_connect(struct __test_metadata *const _metadata,
                 const struct service_fixture *const srv)
{
    EXPECT_EQ(0, test_bind_variant(_metadata, srv));
    EXPECT_EQ(0, test_connect_variant(_metadata, srv));
}

TEST_F(protocol, bind)
{
[...]
    if (is_tcp(variant) && is_sandboxed(variant)) {
        test_tcp_stream_allowed_bind_allowed_connect(_metadata,
                                 &self->srv0);
        test_tcp_stream_denied_bind_allowed_connect(_metadata,
                                &self->srv1);
        test_tcp_stream_denied_bind_denied_connect(_metadata,
                               &self->srv2);
    } else if (is_stream(variant)) {
        test_not_restricted_stream_bind_connect(_metadata, &self->srv0);
        test_not_restricted_stream_bind_connect(_metadata, &self->srv1);
        test_not_restricted_stream_bind_connect(_metadata, &self->srv2);
    } else {
        test_non_stream_bind_connect(_metadata, &self->srv0);
        test_non_stream_bind_connect(_metadata, &self->srv1);
        test_non_stream_bind_connect(_metadata, &self->srv2);
    }
}

sm1ling-knight commented 2 months ago

Hello @l0kod, @gnoack! Please inform me if you got the previous message... I guess this fix is a blocker for two patches (including TCP listen).

l0kod commented 1 month ago

Sorry for the delay, you message wasn't lost but we were at the OSS/LSS/LPC conferences last week and I guess we had some work to catch up. I'm thinking about that and I'll answer tomorrow.

sm1ling-knight commented 1 month ago

I get it, thank you!

gnoack commented 1 month ago

Hi! Apologies from my side as well, the last two weeks went by in a blur, and I also had some surprises waiting for me at work after the conference ;-)

If the different test_ helper functions are all doing EXPECT_EQ(X, test_bind_variant()) and EXPECT_EQ(Y, test_connect_variant()), do you think we could make X and Y part of the fixture? If feels that if we flattened this out into the test fixture structs, maybe we could avoid the remaining ifs as well? So that the scenario, the inputs and the outputs are just listed in a tabular fashion there? Is that feasible?

This is admittedly a shallow feedback without looking too deep in the code, I might be missing something. I'll have another deeper look at the code review on Friday.

sm1ling-knight commented 1 month ago

Hi! Apologies from my side as well, the last two weeks went by in a blur, and I also had some surprises waiting for me at work after the conference ;-)

No problem! Please, take your time. Presentation was really good btw 👍

If the different test_ helper functions are all doing EXPECT_EQ(X, test_bind_variant()) and EXPECT_EQ(Y, test_connect_variant()), do you think we could make X and Y part of the fixture? If feels that if we flattened this out into the test fixture structs, maybe we could avoid the remaining ifs as well? So that the scenario, the inputs and the outputs are just listed in a tabular fashion there? Is that feasible?

Yeah, I've considered such approach. It has following pros:

Improvement from current implementation: We can really avoid per-operation conditional with this.
Improvement from suggested implementation: All tested logic would be in a single entity. We won't need any extra helpers and conditionals for them.

And cons:

I'm not sure that adding error code variable for each tested action will make the code demonstrative enough.
There would be meaningless testing scenarios for a few tests and protocols. For example, testing server-client connection (test_allowed_connection()) for datagram sockets or testing ephemeral port binding for non-INET sockets (in protocol.bind_unspec test).

To be sure that we're on the same, consider following example:

TEST_F(protocol, connect_unspec)
{
    [...]
    EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
    EXPECT_EQ(srv0->err_listen, listen(bind_fd, backlog));

    child = fork();
    ASSERT_LE(0, child);
    if (child == 0) {
        int connect_fd, ret;

        [...]
        EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
        EXPECT_EQ(srv0->err_reconnect, connect_variant(connect_fd, &self->srv0));

        [...]
        EXPECT_EQ(srv0->err_disconnect, connect_variant(connect_fd, &self->unspec_any0));
        EXPECT_EQ(srv0->err_reconnect, connect_variant(connect_fd, &self->srv0));

        [...]
        EXPECT_EQ(srv0->err_disconnect, connect_variant(connect_fd, &self->unspec_any0));

        [...]
    }

    client_fd = bind_fd;
    ASSERT_LE(srv1->err_accept, accept(bind_fd, NULL, 0));

    [...]
}

This is admittedly a shallow feedback without looking too deep in the code, I might be missing something. I'll have another deeper look at the code review on Friday.

l0kod commented 1 month ago

Hello @l0kod, @gnoack! Please take a look at the following issue. It should be discussed before I send fix for the #40.

About the fix, it should change as less as possible the kernel and test code to ease backporting down to Linux 6.10 (which is the oldest supported branch just after Linux 6.7). So this should not be an issue but we need to check against the latest changes merged for Linux 6.12: https://github.com/landlock-lsm/linux/commit/e1b061b444fb01c237838f0d8238653afe6a8094

So, the following changes should come after the #40 fix.

Objective

Refactor protocol.* net tests. Separate conditionals from the logic of the tests.

Motivation

As @gnoack noted putting conditionals in the test is often considered bad practice (e.g.). Tests are becoming more complicated, harder to read and extend.

We should indeed try to write simpler tests and put the expected results directly in the tests.

[...]


[...]
TEST_F(protocol, bind)
{
[...]
  if (is_tcp(variant) && is_sandboxed(variant)) {
      test_tcp_stream_allowed_bind_allowed_connect(_metadata,
                               &self->srv0);
      test_tcp_stream_denied_bind_allowed_connect(_metadata,
                              &self->srv1);
      test_tcp_stream_denied_bind_denied_connect(_metadata,
                             &self->srv2);
[...]

What about something like this instead:

struct expect_srv {
    const struct service_fixture *const srv;
    int err_bind;
    int err_connect;
};

TEST_F(protocol, bind)
{
    struct expect_srv expect0 = {
        .srv = self->srv0,
    };
    struct expect_srv expect1 = {
        .srv = self->srv1,
    };
    struct expect_srv expect2 = {
        .srv = self->srv2,
    };

    if (is_tcp(variant) && is_sandboxed(variant)) {
        expect1->err_bind = -ECONNREFUSED;
        expect2->err_bind = -ECONNREFUSED;
        expect2->err_connect = -EACCES;
    }

    // [...]

    test_bind_connect_listen(_metadata, expect0));
    test_bind_connect_listen(_metadata, expect1));
    test_bind_connect_listen(_metadata, expect2));

And then add the logic of what can be tested (e.g. listen depends on err_bind being 0) in the test_bind_connect_listen() helper.

sm1ling-knight commented 1 month ago

Hello @l0kod, @gnoack! Please take a look at the following issue. It should be discussed before I send fix for the #40.

About the fix, it should change as less as possible the kernel and test code to ease backporting down to Linux 6.10 (which is the oldest supported branch just after Linux 6.7). So this should not be an issue but we need to check against the latest changes merged for Linux 6.12: e1b061b

So, the following changes should come after the #40 fix.

Ok, I'll send the fix first.

Objective

Refactor protocol.* net tests. Separate conditionals from the logic of the tests.

Motivation

As @gnoack noted putting conditionals in the test is often considered bad practice (e.g.). Tests are becoming more complicated, harder to read and extend.

We should indeed try to write simpler tests and put the expected results directly in the tests.

[...]
[...]
TEST_F(protocol, bind)
{
[...]
    if (is_tcp(variant) && is_sandboxed(variant)) {
        test_tcp_stream_allowed_bind_allowed_connect(_metadata,
                                 &self->srv0);
        test_tcp_stream_denied_bind_allowed_connect(_metadata,
                                &self->srv1);
        test_tcp_stream_denied_bind_denied_connect(_metadata,
                               &self->srv2);
[...]

What about something like this instead:
struct expect_srv {
  const struct service_fixture *const srv;
  int err_bind;
  int err_connect;
};

TEST_F(protocol, bind)
{
  struct expect_srv expect0 = {
      .srv = self->srv0,
  };
  struct expect_srv expect1 = {
      .srv = self->srv1,
  };
  struct expect_srv expect2 = {
      .srv = self->srv2,
  };

  if (is_tcp(variant) && is_sandboxed(variant)) {
      expect1->err_bind = -ECONNREFUSED;
      expect2->err_bind = -ECONNREFUSED;
      expect2->err_connect = -EACCES;
  }

  // [...]

  test_bind_connect_listen(_metadata, expect0));
  test_bind_connect_listen(_metadata, expect1));
  test_bind_connect_listen(_metadata, expect2));
And then add the logic of what can be tested (e.g. listen depends on err_bind being 0) in the test_bind_connect_listen() helper.

This is close to the approach proposed by @gnoack above. Placing error code in the fixture may be better in terms of the number of conditions.

I'm not sure about adding variables for error codes (as I commented above), but if everyone agrees with this approach, let's use it.

l0kod commented 1 month ago

Placing error code in the fixture may be better in terms of the number of conditions.

Fixtures are used to initialize common things, but these error codes may be different according to the sandbox, which is per-test (and then also per-variant).

I'm not sure about adding variables for error codes (as I commented above), but if everyone agrees with this approach, let's use it.

This makes the code less verbose because we don't need to specify when an access request should be allowed. This will also be useful if we want to add more tests to some helpers (e.g. test_bind_connect_listen() here) without changing the existing TEST() (except maybe renaming helpers) and this should then work without changing the handled accesses for these tests. For instance, the patch series for the new listen access rights should not change the existing TEST(), only some helpers and add new TEST().

l0kod commented 1 month ago

Some of the common helpers should be inlined if it's not possible to generalize them [1].

[1] https://lore.kernel.org/all/ZmCf9JVIXmRZrCWk@google.com/.

I sent a patch series to improve the current status: https://lore.kernel.org/all/20241001141234.397649-1-mic@digikod.net/

landlock-lsm / linux

Code refactoring #34

Objective

Motivation

Proposed solution

Outline of changes

Objective

Motivation

Objective

Motivation