diff mbox series

[v1,2/9] landlock: Cosmetic fixes for filesystem management

Message ID 20201111213442.434639-3-mic@digikod.net (mailing list archive)
State New, archived
Headers show
Series Landlock fixes | expand

Commit Message

Mickaël Salaün Nov. 11, 2020, 9:34 p.m. UTC
Improve comments and make get_inode_object() more readable.  The kfree()
call is correct but we should mimimize as much as possible lock windows.

Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 security/landlock/fs.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

James Morris Nov. 20, 2020, 1:37 a.m. UTC | #1
On Wed, 11 Nov 2020, Mickaël Salaün wrote:

> Improve comments and make get_inode_object() more readable.  The kfree()
> call is correct but we should mimimize as much as possible lock windows.
> 
> Cc: James Morris <jmorris@namei.org>
> Cc: Jann Horn <jannh@google.com>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>

> ---
>  security/landlock/fs.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index b67c821bb40b..33fc7ae17c7f 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -85,8 +85,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  			return object;
>  		}
>  		/*
> -		 * We're racing with release_inode(), the object is going away.
> -		 * Wait for release_inode(), then retry.
> +		 * We are racing with release_inode(), the object is going
> +		 * away.  Wait for release_inode(), then retry.
>  		 */
>  		spin_lock(&object->lock);
>  		spin_unlock(&object->lock);
> @@ -107,21 +107,21 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  			lockdep_is_held(&inode->i_lock));
>  	if (unlikely(object)) {
>  		/* Someone else just created the object, bail out and retry. */
> -		kfree(new_object);
>  		spin_unlock(&inode->i_lock);
> +		kfree(new_object);
>  
>  		rcu_read_lock();
>  		goto retry;
> -	} else {
> -		rcu_assign_pointer(inode_sec->object, new_object);
> -		/*
> -		 * @inode will be released by hook_sb_delete() on its
> -		 * superblock shutdown.
> -		 */
> -		ihold(inode);
> -		spin_unlock(&inode->i_lock);
> -		return new_object;
>  	}
> +
> +	rcu_assign_pointer(inode_sec->object, new_object);
> +	/*
> +	 * @inode will be released by hook_sb_delete() on its superblock
> +	 * shutdown.
> +	 */
> +	ihold(inode);
> +	spin_unlock(&inode->i_lock);
> +	return new_object;
>  }
>  
>  /* All access rights which can be tied to files. */
>
diff mbox series

Patch

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index b67c821bb40b..33fc7ae17c7f 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -85,8 +85,8 @@  static struct landlock_object *get_inode_object(struct inode *const inode)
 			return object;
 		}
 		/*
-		 * We're racing with release_inode(), the object is going away.
-		 * Wait for release_inode(), then retry.
+		 * We are racing with release_inode(), the object is going
+		 * away.  Wait for release_inode(), then retry.
 		 */
 		spin_lock(&object->lock);
 		spin_unlock(&object->lock);
@@ -107,21 +107,21 @@  static struct landlock_object *get_inode_object(struct inode *const inode)
 			lockdep_is_held(&inode->i_lock));
 	if (unlikely(object)) {
 		/* Someone else just created the object, bail out and retry. */
-		kfree(new_object);
 		spin_unlock(&inode->i_lock);
+		kfree(new_object);
 
 		rcu_read_lock();
 		goto retry;
-	} else {
-		rcu_assign_pointer(inode_sec->object, new_object);
-		/*
-		 * @inode will be released by hook_sb_delete() on its
-		 * superblock shutdown.
-		 */
-		ihold(inode);
-		spin_unlock(&inode->i_lock);
-		return new_object;
 	}
+
+	rcu_assign_pointer(inode_sec->object, new_object);
+	/*
+	 * @inode will be released by hook_sb_delete() on its superblock
+	 * shutdown.
+	 */
+	ihold(inode);
+	spin_unlock(&inode->i_lock);
+	return new_object;
 }
 
 /* All access rights which can be tied to files. */