Re: [PATCH v2 03/17] landlock: Split struct landlock_domain from struct landlock_ruleset

[Tingmao Wang <m@maowtm.org>]

On 4/6/26 15:37, Mickaël Salaün wrote:
> [...]
> @@ -197,10 +179,10 @@ static void build_check_ruleset(void)
>   *
>   * Return: 0 on success, -errno on failure.
>   */
> -static int insert_rule(struct landlock_rules *const rules,
> -		       const struct landlock_id id,
> -		       const struct landlock_layer (*layers)[],
> -		       const size_t num_layers)
> +int landlock_rule_insert(struct landlock_rules *const rules,
> +			 const struct landlock_id id,
> +			 const struct landlock_layer (*layers)[],
> +			 const size_t num_layers)

Maybe this is slightly off topic, but previously I've found this function,
along with create_rule and merge_tree, to be quite confusing, because the
logic for three different use cases (creating a copy of an old domain,
merging a new layer into this domain, and inserting a new rule into an
unmerged ruleset) are mixed in the code, and personally now that I'm
reading it again I still find these functions to be hard to reason about.
Therefore, given that we're refactoring these areas, I think this might be
a good opportunity to rewrite them (while getting the necessary testing
for this rewrite "for free" as part of this whole domain refactor).

For example, for this snippet:

		/* Only a single-level layer should match an existing rule. */
		if (WARN_ON_ONCE(num_layers != 1))
			return -EINVAL;

Someone unfamiliar with how this function is being used by its caller may
not realize that the reason the comment is true is because the only use
case where we have multiple layers in @layers being passed into this
function is when we're copying an existing domain into a new domain, and
so we should never match something that already exists.

Also, further along in this function, there is this snippet:

		/*
		 * Intersects access rights when it is a merge between a
		 * ruleset and a domain.
		 */
		new_rule = create_rule(id, &this->layers, this->num_layers,
				       &(*layers)[0]);

I also found the comment to be confusing because it's not "intersect"ing
anything (it's adding a new layer to an existing rule in the domain when
the object pointed to by "id" exists already in the domain, and the
intersection is only a consequence of how Landlock works when there are
multiple layers).  This realization is made harder by the fact that a few
lines above we just OR'd the access rights (for modification of an
unmerged ruleset), and so it makes it sound like it's doing a similar
thing except with AND instead of OR.

I think it might make sense to have separate functions, even if they result in some slight code duplication, for use by:

1. Copying a domain: inherit_ruleset() -> inherit_tree() -> _____()
    RB tree search to find the insertion point + create_rule().  Maybe
    this logic could just be in inherit_tree() without creating a separate
    function?
2. Merging a rule into a domain: merge_ruleset() -> merge_tree() -> _____()
    insert_or_append_rule()?  RB tree search, call create_rule() to create
    a new rule with the new layer added, then either
    rb_link_node()+rb_insert_color() or rb_replace_node().

Neither functions above will contain any actual logical AND/OR.

3. Inserting a new rule into an unmerged ruleset: landlock_add_rule() -> ... -> _____()
    insert_or_update_rule()?  RB tree search, and either update the access
    rights of an existing rule in-place (as we currently do), or create a
    new rule if the search fails.

We could create a utility static function or macro for the shared RB tree
search code.

How does this sound?