On Sun, Apr 12, 2026 at 05:27:51PM +0100, Tingmao Wang wrote: > 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?
This sounds good, but not in this series. The first part of this series is mainly about moving code, and I'd like to avoid unnecessary churn because the domain split is already a heavy prerequisite. Once this series land, a patch to improve the existing code would be welcome.
