On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote: > > Normally it is ok to include a lookup with the subsequent operation on > the result. However in this case ovl_cleanup_and_whiteout() already > (potentially) creates a whiteout inode so we need separate locking. The change itself looks fine and simple, but I didn't understand the text above. Can you please explain? Thanks, Amir. > > Signed-off-by: NeilBrown <neil@xxxxxxxxxx> > --- > fs/overlayfs/dir.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index d01e83f9d800..8580cd5c61e4 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -769,15 +769,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, > goto out; > } > > - err = ovl_lock_rename_workdir(workdir, NULL, upperdir, NULL); > - if (err) > - goto out_dput; > - > - upper = ovl_lookup_upper(ofs, dentry->d_name.name, upperdir, > - dentry->d_name.len); > + upper = ovl_lookup_upper_unlocked(ofs, dentry->d_name.name, upperdir, > + dentry->d_name.len); > err = PTR_ERR(upper); > if (IS_ERR(upper)) > - goto out_unlock; > + goto out_dput; > > err = -ESTALE; > if ((opaquedir && upper != opaquedir) || > @@ -786,6 +782,10 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, > goto out_dput_upper; > } > > + err = ovl_lock_rename_workdir(workdir, NULL, upperdir, upper); > + if (err) > + goto out_dput_upper; > + > err = ovl_cleanup_and_whiteout(ofs, upperdir, upper); > if (err) > goto out_d_drop; > @@ -793,10 +793,9 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, > ovl_dir_modified(dentry->d_parent, true); > out_d_drop: > d_drop(dentry); > + unlock_rename(workdir, upperdir); > out_dput_upper: > dput(upper); > -out_unlock: > - unlock_rename(workdir, upperdir); > out_dput: > dput(opaquedir); > out: > -- > 2.49.0 >
