On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote: > From: NeilBrown <[email protected]> > > The primary purpose of this patch is to remove the locking from > ovl_lookup_real_one() as part of centralising all locking of directories > for name operations. > > The locking here isn't needed. By performing consistency tests after > the lookup we can be sure that the result of the lookup was valid at > least for a moment, which is all the original code promised. > > lookup_noperm_unlocked() is used for the lookup and it will take the > lock if needed only where it is needed. > > Also: > - don't take a reference to real->d_parent. The parent is > only use for a pointer comparison, and no reference is needed for > that. > - Several "if" statements have a "goto" followed by "else" - the > else isn't needed: the following statement can directly follow > the "if" as a new statement > - Use a consistent pattern of setting "err" before performing a test > and possibly going to "fail". > - remove the "out" label (now that we don't need to dput(parent) or > unlock) and simply return from fail:. > > Signed-off-by: NeilBrown <[email protected]> > --- > fs/overlayfs/export.c | 61 ++++++++++++++++++------------------------- > 1 file changed, 26 insertions(+), 35 deletions(-) > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c > index 83f80fdb1567..dcd28ffc4705 100644 > --- a/fs/overlayfs/export.c > +++ b/fs/overlayfs/export.c > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry > *connected, > struct dentry *real, > const struct ovl_layer *layer) > { > - struct inode *dir = d_inode(connected); > - struct dentry *this, *parent = NULL; > + struct dentry *this; > struct name_snapshot name; > int err; > > /* > - * Lookup child overlay dentry by real name. The dir mutex protects us > - * from racing with overlay rename. If the overlay dentry that is above > - * real has already been moved to a parent that is not under the > - * connected overlay dir, we return -ECHILD and restart the lookup of > - * connected real path from the top. > + * @connected is a directory in the overlay and @real is an object > + * on @layer which is expected to be a child of @connected. > + * The goal is to return a dentry from the overlay which corresponds > + * to @real. This is done by looking up the name from @real in > + * @connected and checking that the result meets expectations. > + * > + * Return %-ECHILD if the parent of @real no-longer corresponds to > + * @connected, and %-ESTALE if the dentry found by lookup doesn't > + * correspond to @real. > */ > - inode_lock_nested(dir, I_MUTEX_PARENT); > - err = -ECHILD; > - parent = dget_parent(real); > - if (ovl_dentry_real_at(connected, layer->idx) != parent) > - goto fail; > > - /* > - * We also need to take a snapshot of real dentry name to protect us > - * from racing with underlying layer rename. In this case, we don't > - * care about returning ESTALE, only from dereferencing a free name > - * pointer because we hold no lock on the real dentry. > - */ > take_dentry_name_snapshot(&name, real); > - /* > - * No idmap handling here: it's an internal lookup. > - */ > - this = lookup_noperm(&name.name, connected); > + this = lookup_noperm_unlocked(&name.name, connected); > release_dentry_name_snapshot(&name); > + > + err = -ECHILD; > + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent) > + goto fail; > + > err = PTR_ERR(this); > - if (IS_ERR(this)) { > + if (IS_ERR(this)) > goto fail; > - } else if (!this || !this->d_inode) { > - dput(this); > - err = -ENOENT; > + > + err = -ENOENT; > + if (!this || !this->d_inode) > goto fail; > - } else if (ovl_dentry_real_at(this, layer->idx) != real) { > - dput(this); > - err = -ESTALE; > + > + err = -ESTALE; > + if (ovl_dentry_real_at(this, layer->idx) != real) > goto fail; > - } > > -out: > - dput(parent); > - inode_unlock(dir); > return this; > > fail: > pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, > connected=%pd2, err=%i)\n", > real, layer->idx, connected, err); > - this = ERR_PTR(err); > - goto out; > + if (!IS_ERR(this)) > + dput(this); > + return ERR_PTR(err); > } > > static struct dentry *ovl_lookup_real(struct super_block *sb,
Reviewed-by: Jeff Layton <[email protected]>
