Thread (74 messages) 74 messages, 6 authors, 2023-07-13

Re: [PATCH v9 03/12] landlock: Refactor landlock_find_rule/insert_rule

From: Konstantin Meskhidze (A) <hidden>
Date: 2023-02-14 10:15:16
Also in: netdev, netfilter-devel


2/10/2023 8:36 PM, Mickaël Salaün пишет:
On 16/01/2023 09:58, Konstantin Meskhidze wrote:
quoted
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.
quoted
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Konstantin Meskhidze <redacted>
---

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
[...]
quoted
@@ -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?
quoted
  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))
[...]
quoted
@@ -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.
[...]
quoted
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
[...]
quoted
@@ -285,23 +333,23 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
quoted
-		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.
.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help