[PATCH v1 0/4] Use scoped guards on Landlock

[PATCH v1 0/4] Use scoped guards on Landlock

[Mickaël Salaün]

Using scoped guards helps simplifying error handling by removing goto
statements.

Only one macro was missing for mutex_lock() calls with
SINGLE_DEPTH_NESTING.

Regards,

Mickaël Salaün (4):
  landlock: Use scoped guards for ruleset
  landlock: Use scoped guards for ruleset in landlock_add_rule()
  locking/mutex: Add mutex_nest_1() scoped guard
  landlock: Use scoped guards for mutex

 include/linux/mutex.h        |  2 +
 security/landlock/ruleset.c  | 74 +++++++++++++++---------------------
 security/landlock/ruleset.h  |  5 +++
 security/landlock/syscalls.c | 39 ++++++-------------
 4 files changed, 50 insertions(+), 70 deletions(-)


base-commit: 9d89551994a430b50c4fffcb1e617a057fa76e20
-- 
2.47.1

[PATCH v1 1/4] landlock: Use scoped guards for ruleset

[Mickaël Salaün]

Simplify error handling by replacing goto statements with automatic
calls to landlock_put_ruleset() when going out of scope.

This change will be easy to backport to v6.6 if needed, only the
kernel.h include line conflicts.  As for any other similar changes, we
should be careful when backporting without goto statements.

Add missing include file.

Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250113161112.452505-2-mic@digikod.net
---
 security/landlock/ruleset.c  | 22 ++++++++++------------
 security/landlock/ruleset.h  |  5 +++++
 security/landlock/syscalls.c | 25 ++++++++-----------------
 3 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index a93bdbf52fff..f27b7bdb19b9 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -8,11 +8,13 @@
 
 #include <linux/bits.h>
 #include <linux/bug.h>
+#include <linux/cleanup.h>
 #include <linux/compiler_types.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/lockdep.h>
+#include <linux/mutex.h>
 #include <linux/overflow.h>
 #include <linux/rbtree.h>
 #include <linux/refcount.h>
@@ -537,7 +539,7 @@ struct landlock_ruleset *
 landlock_merge_ruleset(struct landlock_ruleset *const parent,
 		       struct landlock_ruleset *const ruleset)
 {
-	struct landlock_ruleset *new_dom;
+	struct landlock_ruleset *new_dom __free(landlock_put_ruleset) = NULL;
 	u32 num_layers;
 	int err;
 
@@ -557,29 +559,25 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
 	new_dom = create_ruleset(num_layers);
 	if (IS_ERR(new_dom))
 		return new_dom;
+
 	new_dom->hierarchy =
 		kzalloc(sizeof(*new_dom->hierarchy), GFP_KERNEL_ACCOUNT);
-	if (!new_dom->hierarchy) {
-		err = -ENOMEM;
-		goto out_put_dom;
-	}
+	if (!new_dom->hierarchy)
+		return ERR_PTR(-ENOMEM);
+
 	refcount_set(&new_dom->hierarchy->usage, 1);
 
 	/* ...as a child of @parent... */
 	err = inherit_ruleset(parent, new_dom);
 	if (err)
-		goto out_put_dom;
+		return ERR_PTR(err);
 
 	/* ...and including @ruleset. */
 	err = merge_ruleset(new_dom, ruleset);
 	if (err)
-		goto out_put_dom;
-
-	return new_dom;
+		return ERR_PTR(err);
 
-out_put_dom:
-	landlock_put_ruleset(new_dom);
-	return ERR_PTR(err);
+	return no_free_ptr(new_dom);
 }
 
 /*
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 631e24d4ffe9..70e5b53d1c71 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -11,6 +11,8 @@
 
 #include <linux/bitops.h>
 #include <linux/build_bug.h>
+#include <linux/cleanup.h>
+#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
@@ -252,6 +254,9 @@ landlock_create_ruleset(const access_mask_t access_mask_fs,
 void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
 void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
 
+DEFINE_FREE(landlock_put_ruleset, struct landlock_ruleset *,
+	    if (!IS_ERR_OR_NULL(_T)) landlock_put_ruleset(_T))
+
 int landlock_insert_rule(struct landlock_ruleset *const ruleset,
 			 const struct landlock_id id,
 			 const access_mask_t access);
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 4ed8e70c25ed..5a7f1f77292e 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -10,6 +10,7 @@
 #include <linux/anon_inodes.h>
 #include <linux/build_bug.h>
 #include <linux/capability.h>
+#include <linux/cleanup.h>
 #include <linux/compiler_types.h>
 #include <linux/dcache.h>
 #include <linux/err.h>
@@ -456,10 +457,10 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
 SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
 		flags)
 {
-	struct landlock_ruleset *new_dom, *ruleset;
+	struct landlock_ruleset *new_dom,
+		*ruleset __free(landlock_put_ruleset) = NULL;
 	struct cred *new_cred;
 	struct landlock_cred_security *new_llcred;
-	int err;
 
 	if (!is_initialized())
 		return -EOPNOTSUPP;
@@ -483,10 +484,9 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
 
 	/* Prepares new credentials. */
 	new_cred = prepare_creds();
-	if (!new_cred) {
-		err = -ENOMEM;
-		goto out_put_ruleset;
-	}
+	if (!new_cred)
+		return -ENOMEM;
+
 	new_llcred = landlock_cred(new_cred);
 
 	/*
@@ -495,21 +495,12 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
 	 */
 	new_dom = landlock_merge_ruleset(new_llcred->domain, ruleset);
 	if (IS_ERR(new_dom)) {
-		err = PTR_ERR(new_dom);
-		goto out_put_creds;
+		abort_creds(new_cred);
+		return PTR_ERR(new_dom);
 	}
 
 	/* Replaces the old (prepared) domain. */
 	landlock_put_ruleset(new_llcred->domain);
 	new_llcred->domain = new_dom;
-
-	landlock_put_ruleset(ruleset);
 	return commit_creds(new_cred);
-
-out_put_creds:
-	abort_creds(new_cred);
-
-out_put_ruleset:
-	landlock_put_ruleset(ruleset);
-	return err;
 }
-- 
2.47.1

Re: [PATCH v1 1/4] landlock: Use scoped guards for ruleset

[Günther Noack]

On Mon, Jan 13, 2025 at 05:11:09PM +0100, Mickaël Salaün wrote:
> Simplify error handling by replacing goto statements with automatic
> calls to landlock_put_ruleset() when going out of scope.
> 
> This change will be easy to backport to v6.6 if needed, only the
> kernel.h include line conflicts.  As for any other similar changes, we
> should be careful when backporting without goto statements.
> 
> Add missing include file.
> 
> Cc: Günther Noack <gnoack@google.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250113161112.452505-2-mic@digikod.net
> ---
>  security/landlock/ruleset.c  | 22 ++++++++++------------
>  security/landlock/ruleset.h  |  5 +++++
>  security/landlock/syscalls.c | 25 ++++++++-----------------
>  3 files changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index a93bdbf52fff..f27b7bdb19b9 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -8,11 +8,13 @@
>  
>  #include <linux/bits.h>
>  #include <linux/bug.h>
> +#include <linux/cleanup.h>
>  #include <linux/compiler_types.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/lockdep.h>
> +#include <linux/mutex.h>
>  #include <linux/overflow.h>
>  #include <linux/rbtree.h>
>  #include <linux/refcount.h>
> @@ -537,7 +539,7 @@ struct landlock_ruleset *
>  landlock_merge_ruleset(struct landlock_ruleset *const parent,
>  		       struct landlock_ruleset *const ruleset)
>  {
> -	struct landlock_ruleset *new_dom;
> +	struct landlock_ruleset *new_dom __free(landlock_put_ruleset) = NULL;
>  	u32 num_layers;
>  	int err;
>  
> @@ -557,29 +559,25 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
>  	new_dom = create_ruleset(num_layers);
>  	if (IS_ERR(new_dom))
>  		return new_dom;
> +
>  	new_dom->hierarchy =
>  		kzalloc(sizeof(*new_dom->hierarchy), GFP_KERNEL_ACCOUNT);
> -	if (!new_dom->hierarchy) {
> -		err = -ENOMEM;
> -		goto out_put_dom;
> -	}
> +	if (!new_dom->hierarchy)
> +		return ERR_PTR(-ENOMEM);
> +
>  	refcount_set(&new_dom->hierarchy->usage, 1);
>  
>  	/* ...as a child of @parent... */
>  	err = inherit_ruleset(parent, new_dom);
>  	if (err)
> -		goto out_put_dom;
> +		return ERR_PTR(err);
>  
>  	/* ...and including @ruleset. */
>  	err = merge_ruleset(new_dom, ruleset);
>  	if (err)
> -		goto out_put_dom;
> -
> -	return new_dom;
> +		return ERR_PTR(err);
>  
> -out_put_dom:
> -	landlock_put_ruleset(new_dom);
> -	return ERR_PTR(err);
> +	return no_free_ptr(new_dom);
>  }
>  
>  /*
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 631e24d4ffe9..70e5b53d1c71 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -11,6 +11,8 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/build_bug.h>
> +#include <linux/cleanup.h>
> +#include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
>  #include <linux/rbtree.h>
> @@ -252,6 +254,9 @@ landlock_create_ruleset(const access_mask_t access_mask_fs,
>  void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>  void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
>  
> +DEFINE_FREE(landlock_put_ruleset, struct landlock_ruleset *,
> +	    if (!IS_ERR_OR_NULL(_T)) landlock_put_ruleset(_T))
> +
>  int landlock_insert_rule(struct landlock_ruleset *const ruleset,
>  			 const struct landlock_id id,
>  			 const access_mask_t access);
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 4ed8e70c25ed..5a7f1f77292e 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -10,6 +10,7 @@
>  #include <linux/anon_inodes.h>
>  #include <linux/build_bug.h>
>  #include <linux/capability.h>
> +#include <linux/cleanup.h>
>  #include <linux/compiler_types.h>
>  #include <linux/dcache.h>
>  #include <linux/err.h>
> @@ -456,10 +457,10 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>  SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
>  		flags)
>  {
> -	struct landlock_ruleset *new_dom, *ruleset;
> +	struct landlock_ruleset *new_dom,
> +		*ruleset __free(landlock_put_ruleset) = NULL;
>  	struct cred *new_cred;
>  	struct landlock_cred_security *new_llcred;
> -	int err;
>  
>  	if (!is_initialized())
>  		return -EOPNOTSUPP;
> @@ -483,10 +484,9 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
>  
>  	/* Prepares new credentials. */
>  	new_cred = prepare_creds();
> -	if (!new_cred) {
> -		err = -ENOMEM;
> -		goto out_put_ruleset;
> -	}
> +	if (!new_cred)
> +		return -ENOMEM;
> +
>  	new_llcred = landlock_cred(new_cred);
>  
>  	/*
> @@ -495,21 +495,12 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,
>  	 */
>  	new_dom = landlock_merge_ruleset(new_llcred->domain, ruleset);
>  	if (IS_ERR(new_dom)) {
> -		err = PTR_ERR(new_dom);
> -		goto out_put_creds;
> +		abort_creds(new_cred);
> +		return PTR_ERR(new_dom);
>  	}
>  
>  	/* Replaces the old (prepared) domain. */
>  	landlock_put_ruleset(new_llcred->domain);
>  	new_llcred->domain = new_dom;
> -
> -	landlock_put_ruleset(ruleset);
>  	return commit_creds(new_cred);
> -
> -out_put_creds:
> -	abort_creds(new_cred);
> -
> -out_put_ruleset:
> -	landlock_put_ruleset(ruleset);
> -	return err;
>  }
> -- 
> 2.47.1
> 

Very nice :)

Reviewed-by: Günther Noack <gnoack@google.com>

[PATCH v1 2/4] landlock: Use scoped guards for ruleset in landlock_add_rule()

[Mickaël Salaün]

Simplify error handling by replacing goto statements with automatic
calls to landlock_put_ruleset() when going out of scope.

This change depends on the TCP support.

Cc: Günther Noack <gnoack@google.com>
Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250113161112.452505-3-mic@digikod.net
---
 security/landlock/syscalls.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 5a7f1f77292e..a9760d252fc2 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -399,8 +399,7 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
 		const enum landlock_rule_type, rule_type,
 		const void __user *const, rule_attr, const __u32, flags)
 {
-	struct landlock_ruleset *ruleset;
-	int err;
+	struct landlock_ruleset *ruleset __free(landlock_put_ruleset) = NULL;
 
 	if (!is_initialized())
 		return -EOPNOTSUPP;
@@ -416,17 +415,12 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
 
 	switch (rule_type) {
 	case LANDLOCK_RULE_PATH_BENEATH:
-		err = add_rule_path_beneath(ruleset, rule_attr);
-		break;
+		return add_rule_path_beneath(ruleset, rule_attr);
 	case LANDLOCK_RULE_NET_PORT:
-		err = add_rule_net_port(ruleset, rule_attr);
-		break;
+		return add_rule_net_port(ruleset, rule_attr);
 	default:
-		err = -EINVAL;
-		break;
+		return -EINVAL;
 	}
-	landlock_put_ruleset(ruleset);
-	return err;
 }
 
 /* Enforcement */
-- 
2.47.1

Re: [PATCH v1 2/4] landlock: Use scoped guards for ruleset in landlock_add_rule()

[Günther Noack]

On Mon, Jan 13, 2025 at 05:11:10PM +0100, Mickaël Salaün wrote:
> Simplify error handling by replacing goto statements with automatic
> calls to landlock_put_ruleset() when going out of scope.
> 
> This change depends on the TCP support.
> 
> Cc: Günther Noack <gnoack@google.com>
> Cc: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250113161112.452505-3-mic@digikod.net
> ---
>  security/landlock/syscalls.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 5a7f1f77292e..a9760d252fc2 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -399,8 +399,7 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>  		const enum landlock_rule_type, rule_type,
>  		const void __user *const, rule_attr, const __u32, flags)
>  {
> -	struct landlock_ruleset *ruleset;
> -	int err;
> +	struct landlock_ruleset *ruleset __free(landlock_put_ruleset) = NULL;
>  
>  	if (!is_initialized())
>  		return -EOPNOTSUPP;
> @@ -416,17 +415,12 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>  
>  	switch (rule_type) {
>  	case LANDLOCK_RULE_PATH_BENEATH:
> -		err = add_rule_path_beneath(ruleset, rule_attr);
> -		break;
> +		return add_rule_path_beneath(ruleset, rule_attr);
>  	case LANDLOCK_RULE_NET_PORT:
> -		err = add_rule_net_port(ruleset, rule_attr);
> -		break;
> +		return add_rule_net_port(ruleset, rule_attr);
>  	default:
> -		err = -EINVAL;
> -		break;
> +		return -EINVAL;
>  	}
> -	landlock_put_ruleset(ruleset);
> -	return err;
>  }
>  
>  /* Enforcement */
> -- 
> 2.47.1
> 

Reviewed-by: Günther Noack <gnoack@google.com>

[PATCH v1 3/4] locking/mutex: Add mutex_nest_1() scoped guard

[Mickaël Salaün]

Several places use a mutex with SINGLE_DEPTH_NESTING, but there is no
proper way to use a scoped guard with that.

Add a mutex_nest_1() scoped guard that call metex_lock() with
SINGLE_DEPTH_NESTING (defined in lockdep.h).

This will initially by used by Landlock in a following commit.

Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Günther Noack <gnoack@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250113161112.452505-4-mic@digikod.net
---
 include/linux/mutex.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2bf91b57591b..dfd128a3f365 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -199,6 +199,8 @@ extern void mutex_unlock(struct mutex *lock);
 extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);
 
 DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
+DEFINE_GUARD(mutex_nest_1, struct mutex *,
+	     mutex_lock_nested(_T, SINGLE_DEPTH_NESTING), mutex_unlock(_T))
 DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
 DEFINE_GUARD_COND(mutex, _intr, mutex_lock_interruptible(_T) == 0)
 
-- 
2.47.1

[PATCH v1 4/4] landlock: Use scoped guards for mutex

[Mickaël Salaün]

Simplify error handling by replacing goto statements with automatic
calls to mutex_unlock() when going out of scope.

Do not initialize the err variable for compiler/linter to warn us about
inconsistent use, if any.

Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Günther Noack <gnoack@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250113161112.452505-5-mic@digikod.net
---
 security/landlock/ruleset.c | 52 +++++++++++++++----------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index f27b7bdb19b9..f1c3104aea6c 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -367,7 +367,7 @@ static int merge_tree(struct landlock_ruleset *const dst,
 static int merge_ruleset(struct landlock_ruleset *const dst,
 			 struct landlock_ruleset *const src)
 {
-	int err = 0;
+	int err;
 
 	might_sleep();
 	/* Should already be checked by landlock_merge_ruleset() */
@@ -378,32 +378,28 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
 		return -EINVAL;
 
 	/* Locks @dst first because we are its only owner. */
-	mutex_lock(&dst->lock);
-	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
+	guard(mutex)(&dst->lock);
+	guard(mutex_nest_1)(&src->lock);
 
 	/* Stacks the new layer. */
-	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
+	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1))
+		return -EINVAL;
+
 	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
 
 	/* Merges the @src inode tree. */
 	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
 	if (err)
-		goto out_unlock;
+		return err;
 
 #if IS_ENABLED(CONFIG_INET)
 	/* Merges the @src network port tree. */
 	err = merge_tree(dst, src, LANDLOCK_KEY_NET_PORT);
 	if (err)
-		goto out_unlock;
+		return err;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
-out_unlock:
-	mutex_unlock(&src->lock);
-	mutex_unlock(&dst->lock);
-	return err;
+	return 0;
 }
 
 static int inherit_tree(struct landlock_ruleset *const parent,
@@ -441,47 +437,41 @@ static int inherit_tree(struct landlock_ruleset *const parent,
 static int inherit_ruleset(struct landlock_ruleset *const parent,
 			   struct landlock_ruleset *const child)
 {
-	int err = 0;
+	int err;
 
 	might_sleep();
 	if (!parent)
 		return 0;
 
 	/* Locks @child first because we are its only owner. */
-	mutex_lock(&child->lock);
-	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
+	guard(mutex)(&child->lock);
+	guard(mutex_nest_1)(&parent->lock);
 
 	/* Copies the @parent inode tree. */
 	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
 	if (err)
-		goto out_unlock;
+		return err;
 
 #if IS_ENABLED(CONFIG_INET)
 	/* Copies the @parent network port tree. */
 	err = inherit_tree(parent, child, LANDLOCK_KEY_NET_PORT);
 	if (err)
-		goto out_unlock;
+		return err;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
-	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
+	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers))
+		return -EINVAL;
+
 	/* Copies the parent layer stack and leaves a space for the new layer. */
 	memcpy(child->access_masks, parent->access_masks,
 	       flex_array_size(parent, access_masks, parent->num_layers));
 
-	if (WARN_ON_ONCE(!parent->hierarchy)) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
+	if (WARN_ON_ONCE(!parent->hierarchy))
+		return -EINVAL;
+
 	get_hierarchy(parent->hierarchy);
 	child->hierarchy->parent = parent->hierarchy;
-
-out_unlock:
-	mutex_unlock(&parent->lock);
-	mutex_unlock(&child->lock);
-	return err;
+	return 0;
 }
 
 static void free_ruleset(struct landlock_ruleset *const ruleset)
-- 
2.47.1

Re: [PATCH v1 4/4] landlock: Use scoped guards for mutex

[Günther Noack]

On Mon, Jan 13, 2025 at 05:11:12PM +0100, Mickaël Salaün wrote:
> Simplify error handling by replacing goto statements with automatic
> calls to mutex_unlock() when going out of scope.
> 
> Do not initialize the err variable for compiler/linter to warn us about
> inconsistent use, if any.
> 
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Günther Noack <gnoack@google.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250113161112.452505-5-mic@digikod.net
> ---
>  security/landlock/ruleset.c | 52 +++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 31 deletions(-)
> 
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index f27b7bdb19b9..f1c3104aea6c 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -367,7 +367,7 @@ static int merge_tree(struct landlock_ruleset *const dst,
>  static int merge_ruleset(struct landlock_ruleset *const dst,
>  			 struct landlock_ruleset *const src)
>  {
> -	int err = 0;
> +	int err;
>  
>  	might_sleep();
>  	/* Should already be checked by landlock_merge_ruleset() */
> @@ -378,32 +378,28 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>  		return -EINVAL;
>  
>  	/* Locks @dst first because we are its only owner. */
> -	mutex_lock(&dst->lock);
> -	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
> +	guard(mutex)(&dst->lock);
> +	guard(mutex_nest_1)(&src->lock);
>  
>  	/* Stacks the new layer. */
> -	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
> -		err = -EINVAL;
> -		goto out_unlock;
> -	}
> +	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1))
> +		return -EINVAL;
> +
>  	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
>  
>  	/* Merges the @src inode tree. */
>  	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
>  	if (err)
> -		goto out_unlock;
> +		return err;
>  
>  #if IS_ENABLED(CONFIG_INET)
>  	/* Merges the @src network port tree. */
>  	err = merge_tree(dst, src, LANDLOCK_KEY_NET_PORT);
>  	if (err)
> -		goto out_unlock;
> +		return err;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> -out_unlock:
> -	mutex_unlock(&src->lock);
> -	mutex_unlock(&dst->lock);
> -	return err;
> +	return 0;
>  }
>  
>  static int inherit_tree(struct landlock_ruleset *const parent,
> @@ -441,47 +437,41 @@ static int inherit_tree(struct landlock_ruleset *const parent,
>  static int inherit_ruleset(struct landlock_ruleset *const parent,
>  			   struct landlock_ruleset *const child)
>  {
> -	int err = 0;
> +	int err;
>  
>  	might_sleep();
>  	if (!parent)
>  		return 0;
>  
>  	/* Locks @child first because we are its only owner. */
> -	mutex_lock(&child->lock);
> -	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
> +	guard(mutex)(&child->lock);
> +	guard(mutex_nest_1)(&parent->lock);
>  
>  	/* Copies the @parent inode tree. */
>  	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
>  	if (err)
> -		goto out_unlock;
> +		return err;
>  
>  #if IS_ENABLED(CONFIG_INET)
>  	/* Copies the @parent network port tree. */
>  	err = inherit_tree(parent, child, LANDLOCK_KEY_NET_PORT);
>  	if (err)
> -		goto out_unlock;
> +		return err;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> -	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
> -		err = -EINVAL;
> -		goto out_unlock;
> -	}
> +	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers))
> +		return -EINVAL;
> +
>  	/* Copies the parent layer stack and leaves a space for the new layer. */
>  	memcpy(child->access_masks, parent->access_masks,
>  	       flex_array_size(parent, access_masks, parent->num_layers));
>  
> -	if (WARN_ON_ONCE(!parent->hierarchy)) {
> -		err = -EINVAL;
> -		goto out_unlock;
> -	}
> +	if (WARN_ON_ONCE(!parent->hierarchy))
> +		return -EINVAL;
> +
>  	get_hierarchy(parent->hierarchy);
>  	child->hierarchy->parent = parent->hierarchy;
> -
> -out_unlock:
> -	mutex_unlock(&parent->lock);
> -	mutex_unlock(&child->lock);
> -	return err;
> +	return 0;
>  }
>  
>  static void free_ruleset(struct landlock_ruleset *const ruleset)
> -- 
> 2.47.1
> 

Reviewed-by: Günther Noack <gnoack@google.com>

(Assuming that the mutex_nest_1 guard from the previous commit is OK as well)

—Günther