NeilBrown <[email protected]> wrote: > From: NeilBrown <[email protected]> > > 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 <[email protected]> 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 <[email protected]> > 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().
