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

[Konstantin Meskhidze <konstantin.meskhidze@huawei.com>]

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.

> .