Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 22 Sep 2021 21:12:44 +0300
From:   Kari Argillander <kari.argillander@gmail.com>
To:     Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc:     ntfs3@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/5] fs/ntfs3: Move ni_lock_dir and ni_unlock into
 ntfs_create_inode
Message-ID: <20210922181244.tjxij5gi6xft4wwh@kari-VirtualBox>
References: <2771ff62-e612-a8ed-4b93-5534c26aef9e@paragon-software.com>
 <a269be2f-4ab6-b1c6-790c-9d3052bf22cc@paragon-software.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a269be2f-4ab6-b1c6-790c-9d3052bf22cc@paragon-software.com>
Precedence: bulk

On Wed, Sep 22, 2021 at 07:17:13PM +0300, Konstantin Komarov wrote:
> Now ntfs3 locks mutex for smaller time.

Really like this change. It was my todo list also. Thanks. Still some
comments below.

> 
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/inode.c | 17 ++++++++++++++---
>  fs/ntfs3/namei.c | 20 --------------------
>  2 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index d583b71bec50..6fc99eebd1c1 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -1198,9 +1198,13 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
>  	struct REPARSE_DATA_BUFFER *rp = NULL;
>  	bool rp_inserted = false;
>  
> +	ni_lock_dir(dir_ni);
> +
>  	dir_root = indx_get_root(&dir_ni->dir, dir_ni, NULL, NULL);
> -	if (!dir_root)
> -		return ERR_PTR(-EINVAL);
> +	if (!dir_root) {
> +		err = -EINVAL;
> +		goto out1;
> +	}
>  
>  	if (S_ISDIR(mode)) {
>  		/* Use parent's directory attributes. */
> @@ -1549,6 +1553,9 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
>  	if (err)
>  		goto out6;
>  
> +	/* Unlock parent directory before ntfs_init_acl. */
> +	ni_unlock(dir_ni);

There is err path which can go to err6 (line 1585). Then we get double
unlock situation.

> +
>  	inode->i_generation = le16_to_cpu(rec->seq);
>  
>  	dir->i_mtime = dir->i_ctime = inode->i_atime;
> @@ -1605,8 +1612,10 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
>  out7:
>  
>  	/* Undo 'indx_insert_entry'. */
> +	ni_lock_dir(dir_ni);
>  	indx_delete_entry(&dir_ni->dir, dir_ni, new_de + 1,
>  			  le16_to_cpu(new_de->key_size), sbi);
> +	/* ni_unlock(dir_ni); will be called later. */
>  out6:
>  	if (rp_inserted)
>  		ntfs_remove_reparse(sbi, IO_REPARSE_TAG_SYMLINK, &new_de->ref);
> @@ -1630,8 +1639,10 @@ struct inode *ntfs_create_inode(struct user_namespace *mnt_userns,
>  	kfree(rp);
>  
>  out1:
> -	if (err)
> +	if (err) {
> +		ni_unlock(dir_ni);

This will be double unlock if we exit with err path out6.

  Argillander

>  		return ERR_PTR(err);
> +	}
>  
>  	unlock_new_inode(inode);
>  
> diff --git a/fs/ntfs3/namei.c b/fs/ntfs3/namei.c
> index 1c475da4e19d..bc741213ad84 100644
> --- a/fs/ntfs3/namei.c
> +++ b/fs/ntfs3/namei.c
> @@ -95,16 +95,11 @@ static struct dentry *ntfs_lookup(struct inode *dir, struct dentry *dentry,
>  static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir,
>  		       struct dentry *dentry, umode_t mode, bool excl)
>  {
> -	struct ntfs_inode *ni = ntfs_i(dir);
>  	struct inode *inode;
>  
> -	ni_lock_dir(ni);
> -
>  	inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFREG | mode,
>  				  0, NULL, 0, NULL);
>  
> -	ni_unlock(ni);
> -
>  	return IS_ERR(inode) ? PTR_ERR(inode) : 0;
>  }
>  
> @@ -116,16 +111,11 @@ static int ntfs_create(struct user_namespace *mnt_userns, struct inode *dir,
>  static int ntfs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>  		      struct dentry *dentry, umode_t mode, dev_t rdev)
>  {
> -	struct ntfs_inode *ni = ntfs_i(dir);
>  	struct inode *inode;
>  
> -	ni_lock_dir(ni);
> -
>  	inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, mode, rdev,
>  				  NULL, 0, NULL);
>  
> -	ni_unlock(ni);
> -
>  	return IS_ERR(inode) ? PTR_ERR(inode) : 0;
>  }
>  
> @@ -196,15 +186,10 @@ static int ntfs_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>  {
>  	u32 size = strlen(symname);
>  	struct inode *inode;
> -	struct ntfs_inode *ni = ntfs_i(dir);
> -
> -	ni_lock_dir(ni);
>  
>  	inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFLNK | 0777,
>  				  0, symname, size, NULL);
>  
> -	ni_unlock(ni);
> -
>  	return IS_ERR(inode) ? PTR_ERR(inode) : 0;
>  }
>  
> @@ -215,15 +200,10 @@ static int ntfs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>  		      struct dentry *dentry, umode_t mode)
>  {
>  	struct inode *inode;
> -	struct ntfs_inode *ni = ntfs_i(dir);
> -
> -	ni_lock_dir(ni);
>  
>  	inode = ntfs_create_inode(mnt_userns, dir, dentry, NULL, S_IFDIR | mode,
>  				  0, NULL, 0, NULL);
>  
> -	ni_unlock(ni);
> -
>  	return IS_ERR(inode) ? PTR_ERR(inode) : 0;
>  }
>  
> -- 
> 2.33.0
> 
>