On Mon, Jun 9, 2025 at 1:10 AM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> Proposed changes to directory-op locking will lock the dentry rather
> than the whole directory. So the dentry will need to be unlocked.
>
> vfs_mkdir() consumes the dentry on error, so there will be no dentry to
> be unlocked.
>
> So change vfs_mkdir() to unlock on error as well as releasing the
> dentry. This requires various other functions in various callers to
> also unlock on error.
>
That's a scary subtle API change to make.
If the change to mkdir API wasn't only in v6.15, that would
have been a lethal backporting bug landmine.
Anyway, a shiny porting.rst comment is due.
> At present this results in some clumsy code. Once the transition to
> dentry locking is complete the clumsiness will be gone.
>
> overlayfs looks particularly clumsy as in some cases a double-directory
> rename lock is taken, and a mkdir is then performed in one of the
> directories. If that fails the other directory must be unlocked.
Can some of this mess be abstracted with a helper like
unlock_new_dir(struct dentry *newdir)
which is tolerant to PTR_ERR?
I will refrain from reviewing the ovl patch because you said you found
a bug in it and because I hope it may be easier to review with the
proposed cleanup helper.
>
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
...
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 8baaba0a3fe5..44df3a2449e7 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -248,6 +248,7 @@ static inline struct dentry *ovl_do_mkdir(struct ovl_fs *ofs,
> {
> dentry = vfs_mkdir(ovl_upper_mnt_idmap(ofs), dir, dentry, mode);
> pr_debug("mkdir(%pd2, 0%o) = %i\n", dentry, mode, PTR_ERR_OR_ZERO(dentry));
> + /* Note: dir will have been unlocked on failure */
> return dentry;
> }
>
Your previous APi change introduced a regression here.
The name will not be printed in the error case.
I will post a fix.
Thanks,
Amir.
