> > Another fun question: ovl_copy_up_one() has
> > if (parent) {
> > ovl_path_upper(parent, &parentpath);
> > ctx.destdir = parentpath.dentry;
> > ctx.destname = dentry->d_name;
> >
> > err = vfs_getattr(&parentpath, &ctx.pstat,
> > STATX_ATIME | STATX_MTIME,
> > AT_STATX_SYNC_AS_STAT);
> > if (err)
> > return err;
> > }
> > What stabilizes dentry->d_name here? I might be missing something about the
> > locking environment here, so it might be OK, but...
>
> Honestly, I don't think that anything stabilizes it...
> As long as this cannot result in UAF, we don't care,
> because messing with upper fs directly yields undefined results.
> But I suspect that we do need to take_dentry_name_snapshot()
> to protect against UAF. Right?
>
Sorry, I got confused. It is not about the stability of d_name in the
underlying layer. dentry is the overlayfs dentry that is being copied up.
In principle, dentry->d_name is stable "during copy up" due to the fact
that ovl_rename() calls ovl_copy_up(old) and ovl_copy_up(new) before
starting to rename.
If ovl_dentry_has_upper_alias(dentry), as is the case if ovl_rename()
has already started, then ctx.destname will not actually be dereferenced
and racing with future ovl_rename() is not an issue.
If dentry does need to be copied up, then if ovl_rename() starts after
ovl_copy_up_start(), either ovl_copy_up(old) or ovl_copy_up(new)
will block until ovl_copy_up_end().
I think this would be easier to document and nicer to follow if we read
dentry->d_name inside ovl_do_copy_up() only if we are actually going
to need to use it.
I will try to write it up.
Thanks,
Amir.
