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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.LRH.2.21.2011201236530.15634@namei.org>
Date:   Fri, 20 Nov 2020 12:37:01 +1100 (AEDT)
From:   James Morris <jmorris@...ei.org>
To:     Mickaël Salaün <mic@...ikod.net>
cc:     Jann Horn <jannh@...gle.com>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Shuah Khan <shuah@...nel.org>,
        Vincent Dagonneau <vincent.dagonneau@....gouv.fr>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v1 2/9] landlock: Cosmetic fixes for filesystem
 management

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@...ei.org>
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Serge E. Hallyn <serge@...lyn.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>


Reviewed-by: James Morris <jamorris@...ux.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. */
> 

-- 
James Morris
<jmorris@...ei.org>