[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <487bf00c-b5e1-c391-6ae1-7bd8127db739@huawei.com>
Date: Tue, 14 Feb 2023 13:15:07 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>,
<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
<artem.kuzin@...wei.com>
Subject: Re: [PATCH v9 03/12] landlock: Refactor
landlock_find_rule/insert_rule
2/10/2023 8:36 PM, Mickaël Salaün пишет:
>
> On 16/01/2023 09:58, Konstantin Meskhidze wrote:
>> Add a new landlock_key union and landlock_id structure to support
>> a socket port rule type. A struct landlock_id identifies a unique entry
>> in a ruleset: either a kernel object (e.g inode) or typed data (e.g TCP
>> port). There is one red-black tree per key type.
>>
>> This patch also adds is_object_pointer() and get_root() helpers.
>> is_object_pointer() returns true if key type is LANDLOCK_KEY_INODE.
>> get_root() helper returns a red_black tree root pointer according to
>> a key type.
>>
>> Refactor landlock_insert_rule() and landlock_find_rule() to support coming
>> network modifications. Adding or searching a rule in ruleset can now be
>> done thanks to a Landlock ID argument passed to these helpers.
>>
>> Remove unnecessary inlining.
>>
>
> You need to keep the Co-developed-by before the Signed-off-by for my entry.
Got it.
>
>> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> ---
>>
>> Changes since v8:
>> * Refactors commit message.
>> * Removes inlining.
>> * Minor fixes.
>>
>> Changes since v7:
>> * Completes all the new field descriptions landlock_key,
>> landlock_key_type, landlock_id.
>> * Refactors commit message, adds a co-developer.
>>
>> Changes since v6:
>> * Adds union landlock_key, enum landlock_key_type, and struct
>> landlock_id.
>> * Refactors ruleset functions and improves switch/cases: create_rule(),
>> insert_rule(), get_root(), is_object_pointer(), free_rule(),
>> landlock_find_rule().
>> * Refactors landlock_append_fs_rule() functions to support new
>> landlock_id type.
>>
>> Changes since v5:
>> * Formats code with clang-format-14.
>>
>> Changes since v4:
>> * Refactors insert_rule() and create_rule() functions by deleting
>> rule_type from their arguments list, it helps to reduce useless code.
>>
>> Changes since v3:
>> * Splits commit.
>> * Refactors landlock_insert_rule and landlock_find_rule functions.
>> * Rename new_ruleset->root_inode.
>>
>> ---
>> security/landlock/fs.c | 49 ++++++------
>> security/landlock/ruleset.c | 148 +++++++++++++++++++++++++-----------
>> security/landlock/ruleset.h | 65 +++++++++++++---
>> 3 files changed, 185 insertions(+), 77 deletions(-)
>>
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index 0ae54a639e16..273ed8549da1 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>
> [...]
>
>> @@ -191,12 +193,15 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>> *
>> * Returns NULL if no rule is found or if @dentry is negative.
>> */
>> -static inline const struct landlock_rule *
>> +static const struct landlock_rule *
>
> Can you please create a (previous) dedicated patch for all the inlining
> changes?
>
a patch with just inlining changes?
>
>> find_rule(const struct landlock_ruleset *const domain,
>> const struct dentry *const dentry)
>> {
>> const struct landlock_rule *rule;
>> const struct inode *inode;
>> + struct landlock_id id = {
>> + .type = LANDLOCK_KEY_INODE,
>> + };
>>
>> /* Ignores nonexistent leafs. */
>> if (d_is_negative(dentry))
>
> [...]
>
>> @@ -652,7 +657,7 @@ static inline int check_access_path(const struct landlock_ruleset *const domain,
>> }
>>
>> static int current_check_access_path(const struct path *const path,
>> - const access_mask_t access_request)
>> + const access_mask_t access_request)
>
> This syntax fix should be moved to patch 2/12.
>
Ok. Got it.
>
> [...]
>
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index 1f3188b4e313..c5c88a100f74 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>
> [...]
>
>> @@ -285,23 +333,23 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>
>
>> - 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;
>> - }
>> + if (WARN_ON_ONCE(walker_rule->num_layers != 1))
>> + return -EINVAL;
>> + if (WARN_ON_ONCE(walker_rule->layers[0].level != 0))
>> + return -EINVAL;
>
> This introduces two potential bugs. Why change this code?
My bad. These changes will appear in 4/12. Will be fixed.
Thanks.
> .
Powered by blists - more mailing lists