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

[James Morris]

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 
> Cc: Jann Horn 
> Cc: Serge E. Hallyn 
> Signed-off-by: Mickaël Salaün 


Reviewed-by: James Morris 

> ---
>  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(>lock);
>   spin_unlock(>lock);
> @@ -107,21 +107,21 @@ static struct landlock_object *get_inode_object(struct 
> inode *const inode)
>   lockdep_is_held(>i_lock));
>   if (unlikely(object)) {
>   /* Someone else just created the object, bail out and retry. */
> - kfree(new_object);
>   spin_unlock(>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(>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(>i_lock);
> + return new_object;
>  }
>  
>  /* All access rights which can be tied to files. */
> 

-- 
James Morris

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

[Mickaël Salaün]

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 
Cc: Jann Horn 
Cc: Serge E. Hallyn 
Signed-off-by: Mickaël Salaün 
---
 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(>lock);
spin_unlock(>lock);
@@ -107,21 +107,21 @@ static struct landlock_object *get_inode_object(struct 
inode *const inode)
lockdep_is_held(>i_lock));
if (unlikely(object)) {
/* Someone else just created the object, bail out and retry. */
-   kfree(new_object);
spin_unlock(>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(>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(>i_lock);
+   return new_object;
 }
 
 /* All access rights which can be tied to files. */
-- 
2.29.2