diff mbox series

[v5,03/15] landlock: merge and inherit function refactoring

Message ID 20220516152038.39594-4-konstantin.meskhidze@huawei.com
State Handled Elsewhere
Delegated to: Pablo Neira
Headers show
Series Network support for Landlock | expand

Commit Message

Konstantin Meskhidze (A) May 16, 2022, 3:20 p.m. UTC 
Merge_ruleset() and inherit_ruleset() functions were
refactored to support new rule types. This patch adds
tree_merge() and tree_copy() helpers. Each has
rule_type argument to choose a particular rb_tree
structure in a ruleset.

Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
---

Changes since v3:
* Split commit.
* Refactoring functions:
	-insert_rule.
	-merge_ruleset.
	-tree_merge.
	-inherit_ruleset.
	-tree_copy.
	-free_rule.

Changes since v4:
* None

---
 security/landlock/ruleset.c | 144 ++++++++++++++++++++++++------------
 1 file changed, 98 insertions(+), 46 deletions(-)

--
2.25.1

Comments

Mickaël Salaün May 17, 2022, 8:14 a.m. UTC | #1 
On 16/05/2022 17:20, Konstantin Meskhidze wrote:
> Merge_ruleset() and inherit_ruleset() functions were
> refactored to support new rule types. This patch adds
> tree_merge() and tree_copy() helpers. Each has
> rule_type argument to choose a particular rb_tree
> structure in a ruleset.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v3:
> * Split commit.
> * Refactoring functions:
> 	-insert_rule.
> 	-merge_ruleset.
> 	-tree_merge.
> 	-inherit_ruleset.
> 	-tree_copy.
> 	-free_rule.
> 
> Changes since v4:
> * None
> 
> ---
>   security/landlock/ruleset.c | 144 ++++++++++++++++++++++++------------
>   1 file changed, 98 insertions(+), 46 deletions(-)
> 
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index f079a2a320f1..4b4c9953bb32 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -112,12 +112,16 @@ static struct landlock_rule *create_rule(
>   	return new_rule;
>   }
> 
> -static void free_rule(struct landlock_rule *const rule)
> +static void free_rule(struct landlock_rule *const rule, const u16 rule_type)
>   {
>   	might_sleep();
>   	if (!rule)
>   		return;
> -	landlock_put_object(rule->object.ptr);
> +	switch (rule_type) {
> +	case LANDLOCK_RULE_PATH_BENEATH:
> +		landlock_put_object(rule->object.ptr);
> +		break;
> +	}
>   	kfree(rule);
>   }
> 
> @@ -227,12 +231,12 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>   			new_rule = create_rule(object_ptr, 0, &this->layers,
>   					       this->num_layers,
>   					       &(*layers)[0]);
> +			if (IS_ERR(new_rule))
> +				return PTR_ERR(new_rule);
> +			rb_replace_node(&this->node, &new_rule->node, &ruleset->root_inode);
> +			free_rule(this, rule_type);
>   			break;
>   		}
> -		if (IS_ERR(new_rule))
> -			return PTR_ERR(new_rule);
> -		rb_replace_node(&this->node, &new_rule->node, &ruleset->root_inode);
> -		free_rule(this);
>   		return 0;
>   	}
> 
> @@ -243,13 +247,12 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
>   	switch (rule_type) {
>   	case LANDLOCK_RULE_PATH_BENEATH:
>   		new_rule = create_rule(object_ptr, 0, layers, num_layers, NULL);
> +		if (IS_ERR(new_rule))
> +			return PTR_ERR(new_rule);
> +		rb_link_node(&new_rule->node, parent_node, walker_node);
> +		rb_insert_color(&new_rule->node, &ruleset->root_inode);
>   		break;
>   	}
> -	if (IS_ERR(new_rule))
> -		return PTR_ERR(new_rule);
> -	rb_link_node(&new_rule->node, parent_node, walker_node);
> -	rb_insert_color(&new_rule->node, &ruleset->root_inode);
> -	ruleset->num_rules++;

Why removing this last line?
Konstantin Meskhidze (A) May 18, 2022, 9:18 a.m. UTC | #2 
5/17/2022 11:14 AM, Mickaël Salaün пишет:
> 
> 
> On 16/05/2022 17:20, Konstantin Meskhidze wrote:
>> Merge_ruleset() and inherit_ruleset() functions were
>> refactored to support new rule types. This patch adds
>> tree_merge() and tree_copy() helpers. Each has
>> rule_type argument to choose a particular rb_tree
>> structure in a ruleset.
>>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>>
>> Changes since v3:
>> * Split commit.
>> * Refactoring functions:
>>     -insert_rule.
>>     -merge_ruleset.
>>     -tree_merge.
>>     -inherit_ruleset.
>>     -tree_copy.
>>     -free_rule.
>>
>> Changes since v4:
>> * None
>>
>> ---
>>   security/landlock/ruleset.c | 144 ++++++++++++++++++++++++------------
>>   1 file changed, 98 insertions(+), 46 deletions(-)
>>
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index f079a2a320f1..4b4c9953bb32 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -112,12 +112,16 @@ static struct landlock_rule *create_rule(
>>       return new_rule;
>>   }
>>
>> -static void free_rule(struct landlock_rule *const rule)
>> +static void free_rule(struct landlock_rule *const rule, const u16 
>> rule_type)
>>   {
>>       might_sleep();
>>       if (!rule)
>>           return;
>> -    landlock_put_object(rule->object.ptr);
>> +    switch (rule_type) {
>> +    case LANDLOCK_RULE_PATH_BENEATH:
>> +        landlock_put_object(rule->object.ptr);
>> +        break;
>> +    }
>>       kfree(rule);
>>   }
>>
>> @@ -227,12 +231,12 @@ static int insert_rule(struct landlock_ruleset 
>> *const ruleset,
>>               new_rule = create_rule(object_ptr, 0, &this->layers,
>>                              this->num_layers,
>>                              &(*layers)[0]);
>> +            if (IS_ERR(new_rule))
>> +                return PTR_ERR(new_rule);
>> +            rb_replace_node(&this->node, &new_rule->node, 
>> &ruleset->root_inode);
>> +            free_rule(this, rule_type);
>>               break;
>>           }
>> -        if (IS_ERR(new_rule))
>> -            return PTR_ERR(new_rule);
>> -        rb_replace_node(&this->node, &new_rule->node, 
>> &ruleset->root_inode);
>> -        free_rule(this);
>>           return 0;
>>       }
>>
>> @@ -243,13 +247,12 @@ static int insert_rule(struct landlock_ruleset 
>> *const ruleset,
>>       switch (rule_type) {
>>       case LANDLOCK_RULE_PATH_BENEATH:
>>           new_rule = create_rule(object_ptr, 0, layers, num_layers, 
>> NULL);
>> +        if (IS_ERR(new_rule))
>> +            return PTR_ERR(new_rule);
>> +        rb_link_node(&new_rule->node, parent_node, walker_node);
>> +        rb_insert_color(&new_rule->node, &ruleset->root_inode);
>>           break;
>>       }
>> -    if (IS_ERR(new_rule))
>> -        return PTR_ERR(new_rule);
>> -    rb_link_node(&new_rule->node, parent_node, walker_node);
>> -    rb_insert_color(&new_rule->node, &ruleset->root_inode);
>> -    ruleset->num_rules++;
> 
> Why removing this last line?

  Thank you for noticing that. Its my mistake during refactoring the 
code. Selftests did not show it.

> .
diff mbox series

Patch

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index f079a2a320f1..4b4c9953bb32 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -112,12 +112,16 @@  static struct landlock_rule *create_rule(
 	return new_rule;
 }

-static void free_rule(struct landlock_rule *const rule)
+static void free_rule(struct landlock_rule *const rule, const u16 rule_type)
 {
 	might_sleep();
 	if (!rule)
 		return;
-	landlock_put_object(rule->object.ptr);
+	switch (rule_type) {
+	case LANDLOCK_RULE_PATH_BENEATH:
+		landlock_put_object(rule->object.ptr);
+		break;
+	}
 	kfree(rule);
 }

@@ -227,12 +231,12 @@  static int insert_rule(struct landlock_ruleset *const ruleset,
 			new_rule = create_rule(object_ptr, 0, &this->layers,
 					       this->num_layers,
 					       &(*layers)[0]);
+			if (IS_ERR(new_rule))
+				return PTR_ERR(new_rule);
+			rb_replace_node(&this->node, &new_rule->node, &ruleset->root_inode);
+			free_rule(this, rule_type);
 			break;
 		}
-		if (IS_ERR(new_rule))
-			return PTR_ERR(new_rule);
-		rb_replace_node(&this->node, &new_rule->node, &ruleset->root_inode);
-		free_rule(this);
 		return 0;
 	}

@@ -243,13 +247,12 @@  static int insert_rule(struct landlock_ruleset *const ruleset,
 	switch (rule_type) {
 	case LANDLOCK_RULE_PATH_BENEATH:
 		new_rule = create_rule(object_ptr, 0, layers, num_layers, NULL);
+		if (IS_ERR(new_rule))
+			return PTR_ERR(new_rule);
+		rb_link_node(&new_rule->node, parent_node, walker_node);
+		rb_insert_color(&new_rule->node, &ruleset->root_inode);
 		break;
 	}
-	if (IS_ERR(new_rule))
-		return PTR_ERR(new_rule);
-	rb_link_node(&new_rule->node, parent_node, walker_node);
-	rb_insert_color(&new_rule->node, &ruleset->root_inode);
-	ruleset->num_rules++;
 	return 0;
 }

@@ -298,10 +301,53 @@  static void put_hierarchy(struct landlock_hierarchy *hierarchy)
 	}
 }

+static int tree_merge(struct landlock_ruleset *const src,
+		struct landlock_ruleset *const dst, u16 rule_type)
+{
+	struct landlock_rule *walker_rule, *next_rule;
+	struct rb_root *src_root;
+	int err = 0;
+
+	/* Choose rb_tree structure depending on a rule type */
+	switch (rule_type) {
+	case LANDLOCK_RULE_PATH_BENEATH:
+		src_root = &src->root_inode;
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* Merges the @src tree. */
+	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
+					     src_root, node) {
+		struct landlock_layer layers[] = {{
+			.level = dst->num_layers,
+		}};
+
+		if (WARN_ON_ONCE(walker_rule->num_layers != 1)) {
+			err = -EINVAL;
+			return err;
+		}
+		if (WARN_ON_ONCE(walker_rule->layers[0].level != 0)) {
+			err = -EINVAL;
+			return err;
+		}
+		layers[0].access = walker_rule->layers[0].access;
+
+		switch (rule_type) {
+		case LANDLOCK_RULE_PATH_BENEATH:
+			err = insert_rule(dst, walker_rule->object.ptr, 0, rule_type,
+					  &layers, ARRAY_SIZE(layers));
+			break;
+		}
+		if (err)
+			return err;
+	}
+	return err;
+}
+
 static int merge_ruleset(struct landlock_ruleset *const dst,
 			 struct landlock_ruleset *const src)
 {
-	struct landlock_rule *walker_rule, *next_rule;
 	int err = 0;

 	might_sleep();
@@ -323,29 +369,10 @@  static int merge_ruleset(struct landlock_ruleset *const dst,
 	}
 	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];

-	/* Merges the @src tree. */
-	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
-			&src->root_inode, node) {
-		struct landlock_layer layers[] = {{
-			.level = dst->num_layers,
-		} };
-
-		if (WARN_ON_ONCE(walker_rule->num_layers != 1)) {
-			err = -EINVAL;
-			goto out_unlock;
-		}
-		if (WARN_ON_ONCE(walker_rule->layers[0].level != 0)) {
-			err = -EINVAL;
-			goto out_unlock;
-		}
-		layers[0].access = walker_rule->layers[0].access;
-
-		err = insert_rule(dst, walker_rule->object.ptr, 0,
-				  LANDLOCK_RULE_PATH_BENEATH, &layers,
-				  ARRAY_SIZE(layers));
-		if (err)
-			goto out_unlock;
-	}
+	/* Merges the @src inode tree. */
+	err = tree_merge(src, dst, LANDLOCK_RULE_PATH_BENEATH);
+	if (err)
+		goto out_unlock;

 out_unlock:
 	mutex_unlock(&src->lock);
@@ -353,10 +380,40 @@  static int merge_ruleset(struct landlock_ruleset *const dst,
 	return err;
 }

+static int tree_copy(struct landlock_ruleset *const parent,
+		struct landlock_ruleset *const child, u16 rule_type)
+{
+	struct landlock_rule *walker_rule, *next_rule;
+	struct rb_root *parent_root;
+	int err = 0;
+
+	/* Choose rb_tree structure depending on a rule type */
+	switch (rule_type) {
+	case LANDLOCK_RULE_PATH_BENEATH:
+		parent_root = &parent->root_inode;
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* Copies the @parent inode tree. */
+	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
+					     parent_root, node) {
+		switch (rule_type) {
+		case LANDLOCK_RULE_PATH_BENEATH:
+			err = insert_rule(child, walker_rule->object.ptr, 0,
+					  rule_type, &walker_rule->layers,
+					  walker_rule->num_layers);
+			break;
+		}
+		if (err)
+			return err;
+	}
+	return err;
+}
+
 static int inherit_ruleset(struct landlock_ruleset *const parent,
 			   struct landlock_ruleset *const child)
 {
-	struct landlock_rule *walker_rule, *next_rule;
 	int err = 0;

 	might_sleep();
@@ -367,15 +424,10 @@  static int inherit_ruleset(struct landlock_ruleset *const parent,
 	mutex_lock(&child->lock);
 	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);

-	/* Copies the @parent tree. */
-	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
-			&parent->root_inode, node) {
-		err = insert_rule(child, walker_rule->object.ptr, 0,
-				LANDLOCK_RULE_PATH_BENEATH, &walker_rule->layers,
-				walker_rule->num_layers);
-		if (err)
-			goto out_unlock;
-	}
+	/* Copies the @parent inode tree. */
+	err = tree_copy(parent, child, LANDLOCK_RULE_PATH_BENEATH);
+	if (err)
+		goto out_unlock;

 	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
 		err = -EINVAL;
@@ -405,7 +457,7 @@  static void free_ruleset(struct landlock_ruleset *const ruleset)
 	might_sleep();
 	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
 			node)
-		free_rule(freeme);
+		free_rule(freeme, LANDLOCK_RULE_PATH_BENEATH);
 	put_hierarchy(ruleset->hierarchy);
 	kfree(ruleset);
 }