Re: [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.

[Chris Mason <clm@meta.com>]

NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
> 
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g.  vfs_create()) will be suitable but this is not guaranteed.  The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
> 

Hi everyone,

Amir suggested I run these through, and this commit was flagged:

commit 62d49d1e44667e4f93bec415faabec5526992ac0
Author: NeilBrown <neil@brown.name>

ovl: change ovl_create_real() to get a new lock when re-opening created file.

This commit changes ovl_create_real() to drop the directory lock and
reacquire a new lock for lookup when the created dentry is unhashed. It
also removes ovl_lookup_upper() which is no longer used.

Signed-off-by: NeilBrown <neil@brown.name>

> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c4feb89ad1e39..6285069ccc590 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
>  }
>
>  struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> -			       struct dentry *newdentry, struct ovl_cattr *attr)
> +			       struct dentry *newdentry, struct qstr *qname,
> +			       struct ovl_cattr *attr)
>  {
>  	struct inode *dir = parent->d_inode;
>  	int err;
> @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
>  		struct dentry *d;
>  		/*
>  		 * Some filesystems (i.e. casefolded) may return an unhashed
> -		 * negative dentry from the ovl_lookup_upper() call before
> +		 * negative dentry from the ovl_start_creating_upper() call before
>  		 * ovl_create_real().
>  		 * In that case, lookup again after making the newdentry
>  		 * positive, so ovl_create_upper() always returns a hashed
> -		 * positive dentry.
> +		 * positive dentry.  We lookup using qname which should be
> +		 * the same name as newentry, but is certain not to change.
> +		 * As we have to drop the lock before the lookup a race
> +		 * could result in a lookup failure.  In that case we return
> +		 * an error.
>  		 */
> -		d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> -				     newdentry->d_name.len);
> -		dput(newdentry);
> -		if (IS_ERR_OR_NULL(d))
> +		end_creating_keep(newdentry);
> +		d = ovl_start_creating_upper(ofs, parent, qname);
> +
> +		if (IS_ERR_OR_NULL(d)) {
>  			err = d ? PTR_ERR(d) : -ENOENT;
> +		} else if (d->d_inode != newdentry->d_inode) {
> +			err = -EIO;
> +			dput(newdentry);
> +		} else {
> +			dput(newdentry);
>  			return d;
> +		}
> +		return ERR_PTR(err);
>  	}

Does this leak the newdentry reference when IS_ERR_OR_NULL(d) is true?

The code now calls end_creating_keep(newdentry) which keeps the reference,
then calls ovl_start_creating_upper(). If that returns an error or NULL,
the function returns ERR_PTR(err) without cleaning up newdentry:

	end_creating_keep(newdentry);
	d = ovl_start_creating_upper(ofs, parent, qname);

	if (IS_ERR_OR_NULL(d)) {
		err = d ? PTR_ERR(d) : -ENOENT;
	} ...
	return ERR_PTR(err);

The old code always went through the 'out:' label which called
end_creating(newdentry) on error. The new code bypasses that cleanup.


Does this leak both the parent lock and the dentry 'd' when the lookup
returns a different inode?

When d->d_inode != newdentry->d_inode, the code sets err = -EIO and calls
dput(newdentry), but doesn't clean up 'd':

	} else if (d->d_inode != newdentry->d_inode) {
		err = -EIO;
		dput(newdentry);
	} ...
	return ERR_PTR(err);

The dentry 'd' was returned by ovl_start_creating_upper() with the parent
directory lock held. The code needs to call end_creating(d) to release the
lock via end_dirop()->inode_unlock() and release the dentry reference via
end_dirop()->dput().