On Thu, 26 Jun 2025, Amir Goldstein wrote:
> On Wed, Jun 25, 2025 at 1:07 AM NeilBrown <neil@xxxxxxxxxx> wrote:
> >
> > Rather than using lock_rename(), use the more obvious is_subdir() for
> > ensuring that neither upper nor workdir contain the other.
> > Also be explicit in the comment that the two directories cannot be the
> > same.
> >
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> > fs/overlayfs/super.c | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index cf99b276fdfb..db046b0d6a68 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -438,18 +438,12 @@ static int ovl_lower_dir(const char *name, struct path *path,
> > return 0;
> > }
> >
> > -/* Workdir should not be subdir of upperdir and vice versa */
> > +/* Workdir should not be subdir of upperdir and vice versa, and
> > + * they should not be the same.
> > + */
> > static bool ovl_workdir_ok(struct dentry *workdir, struct dentry *upperdir)
> > {
> > - bool ok = false;
> > -
> > - if (workdir != upperdir) {
> > - struct dentry *trap = lock_rename(workdir, upperdir);
> > - if (!IS_ERR(trap))
> > - unlock_rename(workdir, upperdir);
> > - ok = (trap == NULL);
> > - }
> > - return ok;
> > + return !is_subdir(workdir, upperdir) && !is_subdir(upperdir, workdir);
>
> I am not at ease with this simplification to an unlocked test.
> In the cover letter you wrote that this patch is not like the other patches.
> Is this really necessary for your work?
> If not, please leave it out.
I assume that by "unlocked" you mean that there are two separate calls
to is_subdir() which are not guaranteed to be coherent. I don't see how
that could be a problem. The directory hierarchy could certainly change
between the calls, but it could equally change immediately after a fully
locked call, and thereby invalidate the result. It is not possible to
provide a permanent guarantee that there is never a subdir relationship
between the two.
I don't absolutely need the patch. I plan for lock_rename() to go away.
It will be replaced by lookup_and_lock_rename() which will lock two
dentries and protect the paths from them to their common ancestor from
any renames. lookup_and_lock_rename() can be given the two dentries
rather than having it look them up given parents and names. So it could
be used for this test. It just seems to me to be unnecessary
complexity. I will drop it (for now) if you like.
NeilBrown.
