On Sun, Nov 19, 2023 at 9:26 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, Nov 19, 2023 at 08:57:25AM +0200, Amir Goldstein wrote:
> > On Sat, Nov 18, 2023 at 10:02 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Sun, Nov 12, 2023 at 09:26:28AM +0200, Amir Goldstein wrote:
> > >
> > > > Tested the patch below.
> > > > If you want to apply it as part of dcache cleanup, it's fine by me.
> > > > Otherwise, I will queue it for the next overlayfs update.
> > >
> > > OK... Let's do it that way - overlayfs part goes into never-rebased branch
> > > (no matter which tree), pulled into dcache series and into your overlayfs
> > > update, with removal of unused stuff done in a separate patch in dcache
> > > series.
> > >
> >
> > Sounds good.
> >
> > > That way we won't step on each other's toes when reordering, etc.
> > > Does that work for you? I can put the overlayfs part into #no-rebase-overlayfs
> > > in vfs.git, or you could do it in a v6.7-rc1-based branch in your tree -
> > > whatever's more convenient for you.
> >
> > I've reset overlayfs-next to no-rebase-overlayfs, as it had my version
> > with removal so far.
> >
> > For the final update, I doubt I will need to include it at all, because
> > the chances of ovl_obtain_alias() colliding with anything for the next
> > cycle are pretty slim, but it's good that I have the option and I will
> > anyway make sure to always test the next update with this change.
>
> OK... Several overlayfs locking questions:
> ovl_indexdir_cleanup()
> {
> ...
> inode_lock_nested(dir, I_MUTEX_PARENT);
> ...
> index = ovl_lookup_upper(ofs, p->name, indexdir, p->len);
> ...
> err = ovl_cleanup_and_whiteout(ofs, dir, index);
>
> with ovl_cleanup_and_whiteout() moving stuff between workdir and parent of index.
> Where do you do lock_rename()? It's a cross-directory rename, so it *must*
> lock both (and take ->s_vfs_rename_mutex as well). How can that possibly
> work?
62a8a85be835 ovl: index dir act as work dir
If ofs->indexdir exists, then ofs->wokdir == ofs->indexdir.
That's not very nice. I know.
I will kill ofs->indexdir and change ovl_indexdir() to do:
struct dentry *ovl_indexdir(struct super_block *sb)
{
struct ovl_fs *ofs = OVL_FS(sb);
return ofs->config.index ? ofs->workdir : NULL;
}
>
> Similar in ovl_cleanup_index() - you lock indexdir, then call
> ovl_cleanup_and_whiteout(), with the same locking issues.
>
> 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?
Miklos, am I missing something?
Thanks,
Amir.
