On Fri, 07 Feb 2025, Jeff Layton wrote:
> On Thu, 2025-02-06 at 16:42 +1100, NeilBrown wrote:
> > vfs_mkdir() does not guarantee to make the child dentry positive on
> > success. It may leave it negative and then the caller needs to perform a
> > lookup to find the target dentry.
> >
> > This patch introduced vfs_mkdir_return() which performs the lookup if
> > needed so that this code is centralised.
> >
> > This prepares for a new inode operation which will perform mkdir and
> > returns the correct dentry.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> > fs/cachefiles/namei.c | 7 +---
> > fs/namei.c | 69 ++++++++++++++++++++++++++++++++++++++++
> > fs/nfsd/vfs.c | 21 ++----------
> > fs/overlayfs/dir.c | 33 +------------------
> > fs/overlayfs/overlayfs.h | 10 +++---
> > fs/overlayfs/super.c | 2 +-
> > fs/smb/server/vfs.c | 24 +++-----------
> > include/linux/fs.h | 2 ++
> > 8 files changed, 86 insertions(+), 82 deletions(-)
> >
> > diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> > index 7cf59713f0f7..3c866c3b9534 100644
> > --- a/fs/cachefiles/namei.c
> > +++ b/fs/cachefiles/namei.c
> > @@ -95,7 +95,6 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
> > /* search the current directory for the element name */
> > inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
> >
> > -retry:
> > ret = cachefiles_inject_read_error();
> > if (ret == 0)
> > subdir = lookup_one_len(dirname, dir, strlen(dirname));
> > @@ -130,7 +129,7 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
> > goto mkdir_error;
> > ret = cachefiles_inject_write_error();
> > if (ret == 0)
> > - ret = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), subdir, 0700);
> > + ret = vfs_mkdir_return(&nop_mnt_idmap, d_inode(dir), &subdir, 0700);
> > if (ret < 0) {
> > trace_cachefiles_vfs_error(NULL, d_inode(dir), ret,
> > cachefiles_trace_mkdir_error);
> > @@ -138,10 +137,6 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
> > }
> > trace_cachefiles_mkdir(dir, subdir);
> >
> > - if (unlikely(d_unhashed(subdir))) {
> > - cachefiles_put_directory(subdir);
> > - goto retry;
> > - }
> > ASSERT(d_backing_inode(subdir));
> >
> > _debug("mkdir -> %pd{ino=%lu}",
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 3ab9440c5b93..d98caf36e867 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4317,6 +4317,75 @@ int vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> > }
> > EXPORT_SYMBOL(vfs_mkdir);
> >
> > +/**
> > + * vfs_mkdir_return - create directory returning correct dentry
> > + * @idmap: idmap of the mount the inode was found from
> > + * @dir: inode of the parent directory
> > + * @dentryp: pointer to dentry of the child directory
> > + * @mode: mode of the child directory
> > + *
> > + * Create a directory.
> > + *
> > + * If the inode has been found through an idmapped mount the idmap of
> > + * the vfsmount must be passed through @idmap. This function will then take
> > + * care to map the inode according to @idmap before checking permissions.
> > + * On non-idmapped mounts or if permission checking is to be performed on the
> > + * raw inode simply pass @nop_mnt_idmap.
> > + *
> > + * The filesystem may not use the dentry that was passed in. In that case
> > + * the passed-in dentry is put and a new one is placed in *@dentryp;
>
> This sounds like the filesystem is not allowed to use the dentry that
> we're passing it. Maybe something like this:
>
> "In the event that the filesystem doesn't use *@dentryp, the dentry is
> put and a new one is placed in *@dentryp;"
Good catch - thanks.
I've updated my patch you use your test, except I decided on "dput()"
rather than "put".
Thanks,
NeilBrown
