diff mbox series

[v7,03/18] landlock: refactor merge/inherit_ruleset functions

Message ID 20220829170401.834298-4-konstantin.meskhidze@huawei.com (mailing list archive)
State Not Applicable
Headers show
Series Network support for Landlock | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch, async

Commit Message

Konstantin Meskhidze (A) Aug. 29, 2022, 5:03 p.m. UTC 
Refactors merge_ruleset() and inherit_ruleset() functions to support
new rule types. This patch adds merge_tree() and inherit_tree()
helpers. Each has key_type argument to choose a particular rb_tree
structure in a ruleset.

Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
---

Changes since v6:
* Refactors merge_ruleset() and inherit_ruleset() functions to support
  new rule types.
* Renames tree_merge() to merge_tree() (and reorder arguments), and
  tree_copy() to inherit_tree().

Changes since v5:
* Refactors some logic errors.
* Formats code with clang-format-14.

Changes since v4:
* None

---
 security/landlock/ruleset.c | 108 +++++++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 39 deletions(-)

--
2.25.1

Comments

Mickaël Salaün Sept. 6, 2022, 8:07 a.m. UTC | #1 
On 29/08/2022 19:03, Konstantin Meskhidze wrote:
> Refactors merge_ruleset() and inherit_ruleset() functions to support
> new rule types. This patch adds merge_tree() and inherit_tree()
> helpers. Each has key_type argument to choose a particular rb_tree
> structure in a ruleset.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v6:
> * Refactors merge_ruleset() and inherit_ruleset() functions to support
>    new rule types.
> * Renames tree_merge() to merge_tree() (and reorder arguments), and
>    tree_copy() to inherit_tree().
> 
> Changes since v5:
> * Refactors some logic errors.
> * Formats code with clang-format-14.
> 
> Changes since v4:
> * None
> 
> ---
>   security/landlock/ruleset.c | 108 +++++++++++++++++++++++-------------
>   1 file changed, 69 insertions(+), 39 deletions(-)
> 
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 41de17d1869e..3a5ef356aaa3 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -302,36 +302,18 @@ static void put_hierarchy(struct landlock_hierarchy *hierarchy)
>   	}
>   }
> 
> -static int merge_ruleset(struct landlock_ruleset *const dst,
> -			 struct landlock_ruleset *const src)
> +static int merge_tree(struct landlock_ruleset *const dst,
> +		      struct landlock_ruleset *const src,
> +		      const enum landlock_key_type key_type)
>   {
>   	struct landlock_rule *walker_rule, *next_rule;
>   	struct rb_root *src_root;
>   	int err = 0;
> 
> -	might_sleep();
> -	/* Should already be checked by landlock_merge_ruleset() */
> -	if (WARN_ON_ONCE(!src))
> -		return 0;
> -	/* Only merge into a domain. */
> -	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
> -		return -EINVAL;
> -
> -	src_root = get_root(src, LANDLOCK_KEY_INODE);

This hunk is a bit misleading, but please add a might_sleep() call here 
because of the insert_rule() call, and some lock asserts:

might_sleep();
lockdep_assert_held(&dst->lock);
lockdep_assert_held(&src->lock);


> +	src_root = get_root(src, key_type);
>   	if (IS_ERR(src_root))
>   		return PTR_ERR(src_root);
> 
> -	/* Locks @dst first because we are its only owner. */
> -	mutex_lock(&dst->lock);
> -	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
> -
> -	/* Stacks the new layer. */
> -	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
> -		err = -EINVAL;
> -		goto out_unlock;
> -	}
> -	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
> -
>   	/* Merges the @src tree. */
>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
>   					     node) {
> @@ -340,7 +322,7 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>   		} };
>   		const struct landlock_id id = {
>   			.key = walker_rule->key,
> -			.type = LANDLOCK_KEY_INODE,
> +			.type = key_type,
>   		};
> 
>   		if (WARN_ON_ONCE(walker_rule->num_layers != 1))
> @@ -351,8 +333,39 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
> 
>   		err = insert_rule(dst, id, &layers, ARRAY_SIZE(layers));
>   		if (err)
> -			goto out_unlock;
> +			return err;
> +	}
> +	return err;
> +}
> +
> +static int merge_ruleset(struct landlock_ruleset *const dst,
> +			 struct landlock_ruleset *const src)
> +{
> +	int err = 0;
> +
> +	might_sleep();
> +	/* Should already be checked by landlock_merge_ruleset() */
> +	if (WARN_ON_ONCE(!src))
> +		return 0;
> +	/* Only merge into a domain. */
> +	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
> +		return -EINVAL;
> +
> +	/* Locks @dst first because we are its only owner. */
> +	mutex_lock(&dst->lock);
> +	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
> +
> +	/* Stacks the new layer. */
> +	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
> +		err = -EINVAL;
> +		goto out_unlock;
>   	}
> +	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;
> 
>   out_unlock:
>   	mutex_unlock(&src->lock);
> @@ -360,43 +373,60 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>   	return err;
>   }
> 
> -static int inherit_ruleset(struct landlock_ruleset *const parent,
> -			   struct landlock_ruleset *const child)
> +static int inherit_tree(struct landlock_ruleset *const parent,
> +			struct landlock_ruleset *const child,
> +			const enum landlock_key_type key_type)
>   {
>   	struct landlock_rule *walker_rule, *next_rule;
>   	struct rb_root *parent_root;
>   	int err = 0;
> 
> -	might_sleep();
> -	if (!parent)
> -		return 0;
> -
> -	parent_root = get_root(parent, LANDLOCK_KEY_INODE);

This hunk is a bit misleading, but please add a might_sleep() call here 
because of the insert_rule() call, and some lock asserts:

might_sleep();
lockdep_assert_held(&parent->lock);
lockdep_assert_held(&child->lock);


> +	parent_root = get_root(parent, key_type);
>   	if (IS_ERR(parent_root))
>   		return PTR_ERR(parent_root);
> 
> -	/* Locks @child first because we are its only owner. */
> -	mutex_lock(&child->lock);
> -	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
> -
> -	/* Copies the @parent tree. */
> +	/* Copies the @parent inode or network tree. */
>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>   					     parent_root, node) {
>   		const struct landlock_id id = {
>   			.key = walker_rule->key,
> -			.type = LANDLOCK_KEY_INODE,
> +			.type = key_type,
>   		};
> +
>   		err = insert_rule(child, id, &walker_rule->layers,
>   				  walker_rule->num_layers);
>   		if (err)
> -			goto out_unlock;
> +			return err;
>   	}
> +	return err;
> +}
> +
> +static int inherit_ruleset(struct landlock_ruleset *const parent,
> +			   struct landlock_ruleset *const child)
> +{
> +	int err = 0;
> +
> +	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);
> +
> +	/* Copies the @parent inode tree. */
> +	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
> +	if (err)
> +		goto out_unlock;
> 
>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>   		err = -EINVAL;
>   		goto out_unlock;
>   	}
> -	/* Copies the parent layer stack and leaves a space for the new layer. */
> +	/*
> +	 * 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));
> 
> --
> 2.25.1
>
Konstantin Meskhidze (A) Sept. 9, 2022, 2:53 p.m. UTC | #2 
9/6/2022 11:07 AM, Mickaël Salaün пишет:
> 
> On 29/08/2022 19:03, Konstantin Meskhidze wrote:
>> Refactors merge_ruleset() and inherit_ruleset() functions to support
>> new rule types. This patch adds merge_tree() and inherit_tree()
>> helpers. Each has key_type argument to choose a particular rb_tree
>> structure in a ruleset.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v6:
>> * Refactors merge_ruleset() and inherit_ruleset() functions to support
>>    new rule types.
>> * Renames tree_merge() to merge_tree() (and reorder arguments), and
>>    tree_copy() to inherit_tree().
>> 
>> Changes since v5:
>> * Refactors some logic errors.
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * None
>> 
>> ---
>>   security/landlock/ruleset.c | 108 +++++++++++++++++++++++-------------
>>   1 file changed, 69 insertions(+), 39 deletions(-)
>> 
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index 41de17d1869e..3a5ef356aaa3 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -302,36 +302,18 @@ static void put_hierarchy(struct landlock_hierarchy *hierarchy)
>>   	}
>>   }
>> 
>> -static int merge_ruleset(struct landlock_ruleset *const dst,
>> -			 struct landlock_ruleset *const src)
>> +static int merge_tree(struct landlock_ruleset *const dst,
>> +		      struct landlock_ruleset *const src,
>> +		      const enum landlock_key_type key_type)
>>   {
>>   	struct landlock_rule *walker_rule, *next_rule;
>>   	struct rb_root *src_root;
>>   	int err = 0;
>> 
>> -	might_sleep();
>> -	/* Should already be checked by landlock_merge_ruleset() */
>> -	if (WARN_ON_ONCE(!src))
>> -		return 0;
>> -	/* Only merge into a domain. */
>> -	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
>> -		return -EINVAL;
>> -
>> -	src_root = get_root(src, LANDLOCK_KEY_INODE);
> 
> This hunk is a bit misleading, but please add a might_sleep() call here
> because of the insert_rule() call, and some lock asserts:
> 
> might_sleep();
> lockdep_assert_held(&dst->lock);
> lockdep_assert_held(&src->lock);

   it was moved into merge_ruleset() function,
   please check below.

> 
> 
>> +	src_root = get_root(src, key_type);
>>   	if (IS_ERR(src_root))
>>   		return PTR_ERR(src_root);
>> 
>> -	/* Locks @dst first because we are its only owner. */
>> -	mutex_lock(&dst->lock);
>> -	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
>> -
>> -	/* Stacks the new layer. */
>> -	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
>> -		err = -EINVAL;
>> -		goto out_unlock;
>> -	}
>> -	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
>> -
>>   	/* Merges the @src tree. */
>>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
>>   					     node) {
>> @@ -340,7 +322,7 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   		} };
>>   		const struct landlock_id id = {
>>   			.key = walker_rule->key,
>> -			.type = LANDLOCK_KEY_INODE,
>> +			.type = key_type,
>>   		};
>> 
>>   		if (WARN_ON_ONCE(walker_rule->num_layers != 1))
>> @@ -351,8 +333,39 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>> 
>>   		err = insert_rule(dst, id, &layers, ARRAY_SIZE(layers));
>>   		if (err)
>> -			goto out_unlock;
>> +			return err;
>> +	}
>> +	return err;
>> +}
>> +
>> +static int merge_ruleset(struct landlock_ruleset *const dst,
>> +			 struct landlock_ruleset *const src)
>> +{
>> +	int err = 0;
>> +
>> +	might_sleep();
>> +	/* Should already be checked by landlock_merge_ruleset() */
>> +	if (WARN_ON_ONCE(!src))
>> +		return 0;
>> +	/* Only merge into a domain. */
>> +	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
>> +		return -EINVAL;
>> +
>> +	/* Locks @dst first because we are its only owner. */
>> +	mutex_lock(&dst->lock);
>> +	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
>> +
>> +	/* Stacks the new layer. */
>> +	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
>> +		err = -EINVAL;
>> +		goto out_unlock;
>>   	}
>> +	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;
>> 
>>   out_unlock:
>>   	mutex_unlock(&src->lock);
>> @@ -360,43 +373,60 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   	return err;
>>   }
>> 
>> -static int inherit_ruleset(struct landlock_ruleset *const parent,
>> -			   struct landlock_ruleset *const child)
>> +static int inherit_tree(struct landlock_ruleset *const parent,
>> +			struct landlock_ruleset *const child,
>> +			const enum landlock_key_type key_type)
>>   {
>>   	struct landlock_rule *walker_rule, *next_rule;
>>   	struct rb_root *parent_root;
>>   	int err = 0;
>> 
>> -	might_sleep();
>> -	if (!parent)
>> -		return 0;
>> -
>> -	parent_root = get_root(parent, LANDLOCK_KEY_INODE);
> 
> This hunk is a bit misleading, but please add a might_sleep() call here
> because of the insert_rule() call, and some lock asserts:
> 
> might_sleep();
> lockdep_assert_held(&parent->lock);
> lockdep_assert_held(&child->lock);
> 
   it was moved into inherit_ruleset() function,
   please check below.
> 
>> +	parent_root = get_root(parent, key_type);
>>   	if (IS_ERR(parent_root))
>>   		return PTR_ERR(parent_root);
>> 
>> -	/* Locks @child first because we are its only owner. */
>> -	mutex_lock(&child->lock);
>> -	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
>> -
>> -	/* Copies the @parent tree. */
>> +	/* Copies the @parent inode or network tree. */
>>   	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
>>   					     parent_root, node) {
>>   		const struct landlock_id id = {
>>   			.key = walker_rule->key,
>> -			.type = LANDLOCK_KEY_INODE,
>> +			.type = key_type,
>>   		};
>> +
>>   		err = insert_rule(child, id, &walker_rule->layers,
>>   				  walker_rule->num_layers);
>>   		if (err)
>> -			goto out_unlock;
>> +			return err;
>>   	}
>> +	return err;
>> +}
>> +
>> +static int inherit_ruleset(struct landlock_ruleset *const parent,
>> +			   struct landlock_ruleset *const child)
>> +{
>> +	int err = 0;
>> +
>> +	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);
>> +
>> +	/* Copies the @parent inode tree. */
>> +	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
>> +	if (err)
>> +		goto out_unlock;
>> 
>>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>>   		err = -EINVAL;
>>   		goto out_unlock;
>>   	}
>> -	/* Copies the parent layer stack and leaves a space for the new layer. */
>> +	/*
>> +	 * 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));
>> 
>> --
>> 2.25.1
>> 
> .
Mickaël Salaün Sept. 12, 2022, 5:17 p.m. UTC | #3 
On 09/09/2022 16:53, Konstantin Meskhidze (A) wrote:
> 
> 
> 9/6/2022 11:07 AM, Mickaël Salaün пишет:
>>
>> On 29/08/2022 19:03, Konstantin Meskhidze wrote:
>>> Refactors merge_ruleset() and inherit_ruleset() functions to support
>>> new rule types. This patch adds merge_tree() and inherit_tree()
>>> helpers. Each has key_type argument to choose a particular rb_tree
>>> structure in a ruleset.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---
>>>
>>> Changes since v6:
>>> * Refactors merge_ruleset() and inherit_ruleset() functions to support
>>>     new rule types.
>>> * Renames tree_merge() to merge_tree() (and reorder arguments), and
>>>     tree_copy() to inherit_tree().
>>>
>>> Changes since v5:
>>> * Refactors some logic errors.
>>> * Formats code with clang-format-14.
>>>
>>> Changes since v4:
>>> * None
>>>
>>> ---
>>>    security/landlock/ruleset.c | 108 +++++++++++++++++++++++-------------
>>>    1 file changed, 69 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>>> index 41de17d1869e..3a5ef356aaa3 100644
>>> --- a/security/landlock/ruleset.c
>>> +++ b/security/landlock/ruleset.c
>>> @@ -302,36 +302,18 @@ static void put_hierarchy(struct landlock_hierarchy *hierarchy)
>>>    	}
>>>    }
>>>
>>> -static int merge_ruleset(struct landlock_ruleset *const dst,
>>> -			 struct landlock_ruleset *const src)
>>> +static int merge_tree(struct landlock_ruleset *const dst,
>>> +		      struct landlock_ruleset *const src,
>>> +		      const enum landlock_key_type key_type)
>>>    {
>>>    	struct landlock_rule *walker_rule, *next_rule;
>>>    	struct rb_root *src_root;
>>>    	int err = 0;
>>>
>>> -	might_sleep();
>>> -	/* Should already be checked by landlock_merge_ruleset() */
>>> -	if (WARN_ON_ONCE(!src))
>>> -		return 0;
>>> -	/* Only merge into a domain. */
>>> -	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
>>> -		return -EINVAL;
>>> -
>>> -	src_root = get_root(src, LANDLOCK_KEY_INODE);
>>
>> This hunk is a bit misleading, but please add a might_sleep() call here
>> because of the insert_rule() call, and some lock asserts:
>>
>> might_sleep();
>> lockdep_assert_held(&dst->lock);
>> lockdep_assert_held(&src->lock);
> 
>     it was moved into merge_ruleset() function,
>     please check below.

I know but you still need to add these asserts.


> 
>>
>>
>>> +	src_root = get_root(src, key_type);
>>>    	if (IS_ERR(src_root))
>>>    		return PTR_ERR(src_root);
>>>
>>> -	/* Locks @dst first because we are its only owner. */
>>> -	mutex_lock(&dst->lock);
>>> -	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
>>> -
>>> -	/* Stacks the new layer. */
>>> -	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
>>> -		err = -EINVAL;
>>> -		goto out_unlock;
>>> -	}
>>> -	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
>>> -
>>>    	/* Merges the @src tree. */
>>>    	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
>>>    					     node) {
>>> @@ -340,7 +322,7 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>>    		} };
>>>    		const struct landlock_id id = {
>>>    			.key = walker_rule->key,
>>> -			.type = LANDLOCK_KEY_INODE,
>>> +			.type = key_type,
>>>    		};
>>>
>>>    		if (WARN_ON_ONCE(walker_rule->num_layers != 1))
>>> @@ -351,8 +333,39 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>>
>>>    		err = insert_rule(dst, id, &layers, ARRAY_SIZE(layers));
>>>    		if (err)
>>> -			goto out_unlock;
>>> +			return err;
>>> +	}
>>> +	return err;
>>> +}
>>> +
>>> +static int merge_ruleset(struct landlock_ruleset *const dst,
>>> +			 struct landlock_ruleset *const src)
>>> +{
>>> +	int err = 0;
>>> +
>>> +	might_sleep();
>>> +	/* Should already be checked by landlock_merge_ruleset() */
>>> +	if (WARN_ON_ONCE(!src))
>>> +		return 0;
>>> +	/* Only merge into a domain. */
>>> +	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
>>> +		return -EINVAL;
>>> +
>>> +	/* Locks @dst first because we are its only owner. */
>>> +	mutex_lock(&dst->lock);
>>> +	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
>>> +
>>> +	/* Stacks the new layer. */
>>> +	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
>>> +		err = -EINVAL;
>>> +		goto out_unlock;
>>>    	}
>>> +	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;
>>>
>>>    out_unlock:
>>>    	mutex_unlock(&src->lock);
>>> @@ -360,43 +373,60 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>>    	return err;
>>>    }
>>>
>>> -static int inherit_ruleset(struct landlock_ruleset *const parent,
>>> -			   struct landlock_ruleset *const child)
>>> +static int inherit_tree(struct landlock_ruleset *const parent,
>>> +			struct landlock_ruleset *const child,
>>> +			const enum landlock_key_type key_type)
>>>    {
>>>    	struct landlock_rule *walker_rule, *next_rule;
>>>    	struct rb_root *parent_root;
>>>    	int err = 0;
>>>
>>> -	might_sleep();
>>> -	if (!parent)
>>> -		return 0;
>>> -
>>> -	parent_root = get_root(parent, LANDLOCK_KEY_INODE);
>>
>> This hunk is a bit misleading, but please add a might_sleep() call here
>> because of the insert_rule() call, and some lock asserts:
>>
>> might_sleep();
>> lockdep_assert_held(&parent->lock);
>> lockdep_assert_held(&child->lock);
>>
>     it was moved into inherit_ruleset() function,
>     please check below.

same
diff mbox series

Patch

diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 41de17d1869e..3a5ef356aaa3 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -302,36 +302,18 @@  static void put_hierarchy(struct landlock_hierarchy *hierarchy)
 	}
 }

-static int merge_ruleset(struct landlock_ruleset *const dst,
-			 struct landlock_ruleset *const src)
+static int merge_tree(struct landlock_ruleset *const dst,
+		      struct landlock_ruleset *const src,
+		      const enum landlock_key_type key_type)
 {
 	struct landlock_rule *walker_rule, *next_rule;
 	struct rb_root *src_root;
 	int err = 0;

-	might_sleep();
-	/* Should already be checked by landlock_merge_ruleset() */
-	if (WARN_ON_ONCE(!src))
-		return 0;
-	/* Only merge into a domain. */
-	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
-		return -EINVAL;
-
-	src_root = get_root(src, LANDLOCK_KEY_INODE);
+	src_root = get_root(src, key_type);
 	if (IS_ERR(src_root))
 		return PTR_ERR(src_root);

-	/* Locks @dst first because we are its only owner. */
-	mutex_lock(&dst->lock);
-	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
-
-	/* Stacks the new layer. */
-	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
-	dst->access_masks[dst->num_layers - 1] = src->access_masks[0];
-
 	/* Merges the @src tree. */
 	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
 					     node) {
@@ -340,7 +322,7 @@  static int merge_ruleset(struct landlock_ruleset *const dst,
 		} };
 		const struct landlock_id id = {
 			.key = walker_rule->key,
-			.type = LANDLOCK_KEY_INODE,
+			.type = key_type,
 		};

 		if (WARN_ON_ONCE(walker_rule->num_layers != 1))
@@ -351,8 +333,39 @@  static int merge_ruleset(struct landlock_ruleset *const dst,

 		err = insert_rule(dst, id, &layers, ARRAY_SIZE(layers));
 		if (err)
-			goto out_unlock;
+			return err;
+	}
+	return err;
+}
+
+static int merge_ruleset(struct landlock_ruleset *const dst,
+			 struct landlock_ruleset *const src)
+{
+	int err = 0;
+
+	might_sleep();
+	/* Should already be checked by landlock_merge_ruleset() */
+	if (WARN_ON_ONCE(!src))
+		return 0;
+	/* Only merge into a domain. */
+	if (WARN_ON_ONCE(!dst || !dst->hierarchy))
+		return -EINVAL;
+
+	/* Locks @dst first because we are its only owner. */
+	mutex_lock(&dst->lock);
+	mutex_lock_nested(&src->lock, SINGLE_DEPTH_NESTING);
+
+	/* Stacks the new layer. */
+	if (WARN_ON_ONCE(src->num_layers != 1 || dst->num_layers < 1)) {
+		err = -EINVAL;
+		goto out_unlock;
 	}
+	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;

 out_unlock:
 	mutex_unlock(&src->lock);
@@ -360,43 +373,60 @@  static int merge_ruleset(struct landlock_ruleset *const dst,
 	return err;
 }

-static int inherit_ruleset(struct landlock_ruleset *const parent,
-			   struct landlock_ruleset *const child)
+static int inherit_tree(struct landlock_ruleset *const parent,
+			struct landlock_ruleset *const child,
+			const enum landlock_key_type key_type)
 {
 	struct landlock_rule *walker_rule, *next_rule;
 	struct rb_root *parent_root;
 	int err = 0;

-	might_sleep();
-	if (!parent)
-		return 0;
-
-	parent_root = get_root(parent, LANDLOCK_KEY_INODE);
+	parent_root = get_root(parent, key_type);
 	if (IS_ERR(parent_root))
 		return PTR_ERR(parent_root);

-	/* Locks @child first because we are its only owner. */
-	mutex_lock(&child->lock);
-	mutex_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
-
-	/* Copies the @parent tree. */
+	/* Copies the @parent inode or network tree. */
 	rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
 					     parent_root, node) {
 		const struct landlock_id id = {
 			.key = walker_rule->key,
-			.type = LANDLOCK_KEY_INODE,
+			.type = key_type,
 		};
+
 		err = insert_rule(child, id, &walker_rule->layers,
 				  walker_rule->num_layers);
 		if (err)
-			goto out_unlock;
+			return err;
 	}
+	return err;
+}
+
+static int inherit_ruleset(struct landlock_ruleset *const parent,
+			   struct landlock_ruleset *const child)
+{
+	int err = 0;
+
+	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);
+
+	/* Copies the @parent inode tree. */
+	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
+	if (err)
+		goto out_unlock;

 	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
 		err = -EINVAL;
 		goto out_unlock;
 	}
-	/* Copies the parent layer stack and leaves a space for the new layer. */
+	/*
+	 * 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));