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

From: "Mickaël Salaün" <mic@digikod.net>
To: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: willemdebruijn.kernel@gmail.com,
	linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
	netfilter-devel@vger.kernel.org, yusongping@huawei.com,
	anton.sirazetdinov@huawei.com
Subject: Re: [PATCH v5 03/15] landlock: merge and inherit function refactoring
Date: Tue, 17 May 2022 10:14:59 +0200	[thread overview]
Message-ID: <72c37309-3535-8119-b701-193ccc75e74e@digikod.net> (raw)
In-Reply-To: <20220516152038.39594-4-konstantin.meskhidze@huawei.com>

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?