[RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Al Viro]

	AFAICS, the main reason for exposing those used to be the need
to store ovl_entry in allocated dentry; we needed to do that before it
gets attached to inode, so the guts of d_obtain_alias() had to be
exposed.

	These days overlayfs is stashing ovl_entry in the inode, so
we are left with this:
        dentry = d_find_any_alias(inode);
        if (dentry)
                goto out_iput;

        dentry = d_alloc_anon(inode->i_sb);
        if (unlikely(!dentry))
                goto nomem;

        if (upper_alias)
                ovl_dentry_set_upper_alias(dentry);

        ovl_dentry_init_reval(dentry, upper, OVL_I_E(inode));

        return d_instantiate_anon(dentry, inode);

ovl_dentry_init_reval() can bloody well be skipped, AFAICS - all it does
is potentially clearing DCACHE_OP_{,WEAK_}REVALIDATE.  That's also done
in ovl_lookup(), and in case we have d_splice_alias() return a non-NULL
dentry we can simply copy it there.  Sure, somebody might race with
us, pick dentry from hash and call ->d_revalidate() before we notice that
DCACHE_OP_REVALIDATE could be cleaned.  So what?  That call of ->d_revalidate()
will find nothing to do and return 1.  Which is the effect of having
DCACHE_OP_REVALIDATE cleared, except for pointless method call.  Anyone
who finds that dentry after the flag is cleared will skip the call.
IOW, that race is harmless.

And as for the ovl_dentry_set_upper_alias()... that information used to
live in ovl_entry until the need to trim the thing down.  These days
it's in a bit in dentry->d_fsdata.

How painful would it be to switch to storing that in LSB of ovl_entry::__numlower,
turning ovl_numlower() into
	return oe ? oe->__numlower>>1 : 0
and ovl_lowerdata() into
	return lowerstack ? &lowerstack[(oe->__numlower>>1) - 1] : NULL
with obvious adjustment to ovl_alloc_entry().

An entry is coallocated with an array of struct ovl_path, with
numlower elements.  More than 2G layers doesn't seem to be plausible -
there are fat 64bit boxen, but 32Gb (kmalloc'ed, at that) just in
the root ovl_entry alone feels somewhat over the top ;-)

So stealing that bit shouldn't be a problem.  Is there anything I'm
missing?

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Amir Goldstein]

On Sat, Nov 11, 2023 at 10:04 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         AFAICS, the main reason for exposing those used to be the need
> to store ovl_entry in allocated dentry; we needed to do that before it
> gets attached to inode, so the guts of d_obtain_alias() had to be
> exposed.
>
>         These days overlayfs is stashing ovl_entry in the inode, so
> we are left with this:
>         dentry = d_find_any_alias(inode);
>         if (dentry)
>                 goto out_iput;
>
>         dentry = d_alloc_anon(inode->i_sb);
>         if (unlikely(!dentry))
>                 goto nomem;
>
>         if (upper_alias)
>                 ovl_dentry_set_upper_alias(dentry);
>
>         ovl_dentry_init_reval(dentry, upper, OVL_I_E(inode));
>
>         return d_instantiate_anon(dentry, inode);
>
> ovl_dentry_init_reval() can bloody well be skipped, AFAICS - all it does
> is potentially clearing DCACHE_OP_{,WEAK_}REVALIDATE.  That's also done
> in ovl_lookup(), and in case we have d_splice_alias() return a non-NULL
> dentry we can simply copy it there.  Sure, somebody might race with
> us, pick dentry from hash and call ->d_revalidate() before we notice that
> DCACHE_OP_REVALIDATE could be cleaned.  So what?  That call of ->d_revalidate()
> will find nothing to do and return 1.  Which is the effect of having
> DCACHE_OP_REVALIDATE cleared, except for pointless method call.  Anyone
> who finds that dentry after the flag is cleared will skip the call.
> IOW, that race is harmless.

Just a minute.
Do you know that ovl_obtain_alias() is *only* used to obtain a disconnected
non-dir overlayfs dentry?

I think that makes all the analysis regarding race with d_splice_alias()
moot. Right?

Do DCACHE_OP_*REVALIDATE even matter for a disconnected
non-dir dentry?

>
> And as for the ovl_dentry_set_upper_alias()... that information used to
> live in ovl_entry until the need to trim the thing down.  These days
> it's in a bit in dentry->d_fsdata.
>
> How painful would it be to switch to storing that in LSB of ovl_entry::__numlower,
> turning ovl_numlower() into
>         return oe ? oe->__numlower>>1 : 0
> and ovl_lowerdata() into
>         return lowerstack ? &lowerstack[(oe->__numlower>>1) - 1] : NULL
> with obvious adjustment to ovl_alloc_entry().
>
> An entry is coallocated with an array of struct ovl_path, with
> numlower elements.  More than 2G layers doesn't seem to be plausible -
> there are fat 64bit boxen, but 32Gb (kmalloc'ed, at that) just in
> the root ovl_entry alone feels somewhat over the top ;-)
>
> So stealing that bit shouldn't be a problem.  Is there anything I'm
> missing?

You are missing that the OVL_E_UPPER_ALIAS flag is a property of
the overlay dentry, not a property of the inode.

N lower hardlinks, the first copy up created an upper inode
all the rest of the N upper aliases to that upper inode are
created lazily.

However, for obvious reasons, OVL_E_UPPER_ALIAS is not
well defined for a disconnected overlay dentry.
There should not be any code (I hope) that cares about
OVL_E_UPPER_ALIAS for a disconnected overlay dentry,
so I *think* ovl_dentry_set_upper_alias() in this code is moot.

I need to look closer to verify, but please confirm my assumption
regarding the irrelevance of  DCACHE_OP_*REVALIDATE for a
disconnected non-dir dentry.

Thanks,
Amir.

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Al Viro]

On Sat, Nov 11, 2023 at 08:31:11PM +0200, Amir Goldstein wrote:
> > in ovl_lookup(), and in case we have d_splice_alias() return a non-NULL
> > dentry we can simply copy it there.  Sure, somebody might race with
> > us, pick dentry from hash and call ->d_revalidate() before we notice that
> > DCACHE_OP_REVALIDATE could be cleaned.  So what?  That call of ->d_revalidate()
> > will find nothing to do and return 1.  Which is the effect of having
> > DCACHE_OP_REVALIDATE cleared, except for pointless method call.  Anyone
> > who finds that dentry after the flag is cleared will skip the call.
> > IOW, that race is harmless.
> 
> Just a minute.
> Do you know that ovl_obtain_alias() is *only* used to obtain a disconnected
> non-dir overlayfs dentry?

D'oh...

> I think that makes all the analysis regarding race with d_splice_alias()
> moot. Right?

Right you are.

> Do DCACHE_OP_*REVALIDATE even matter for a disconnected
> non-dir dentry?

As long as nothing picks it via d_find_any_alias() and moves it somewhere
manually.  The former might happen, the latter, AFAICS, doesn't - nothing
like d_move() anywhere in sight...

> You are missing that the OVL_E_UPPER_ALIAS flag is a property of
> the overlay dentry, not a property of the inode.
> 
> N lower hardlinks, the first copy up created an upper inode
> all the rest of the N upper aliases to that upper inode are
> created lazily.
> 
> However, for obvious reasons, OVL_E_UPPER_ALIAS is not
> well defined for a disconnected overlay dentry.
> There should not be any code (I hope) that cares about
> OVL_E_UPPER_ALIAS for a disconnected overlay dentry,
> so I *think* ovl_dentry_set_upper_alias() in this code is moot.
> 
> I need to look closer to verify, but please confirm my assumption
> regarding the irrelevance of  DCACHE_OP_*REVALIDATE for a
> disconnected non-dir dentry.

Correct; we only care if it gets reconnected to the main tree.
The fact that it's only for non-directories simplifies life a lot
there.  Sorry, got confused by the work you do with ->d_flags
and hadn't stopped to ask whether it's needed in the first place
in there.

OK, so... are there any reasons why simply calling d_obtain_alias()
wouldn't do the right thing these days?

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Amir Goldstein]

On Sat, Nov 11, 2023 at 8:50 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Nov 11, 2023 at 08:31:11PM +0200, Amir Goldstein wrote:
> > > in ovl_lookup(), and in case we have d_splice_alias() return a non-NULL
> > > dentry we can simply copy it there.  Sure, somebody might race with
> > > us, pick dentry from hash and call ->d_revalidate() before we notice that
> > > DCACHE_OP_REVALIDATE could be cleaned.  So what?  That call of ->d_revalidate()
> > > will find nothing to do and return 1.  Which is the effect of having
> > > DCACHE_OP_REVALIDATE cleared, except for pointless method call.  Anyone
> > > who finds that dentry after the flag is cleared will skip the call.
> > > IOW, that race is harmless.
> >
> > Just a minute.
> > Do you know that ovl_obtain_alias() is *only* used to obtain a disconnected
> > non-dir overlayfs dentry?
>
> D'oh...
>
> > I think that makes all the analysis regarding race with d_splice_alias()
> > moot. Right?
>
> Right you are.
>
> > Do DCACHE_OP_*REVALIDATE even matter for a disconnected
> > non-dir dentry?
>
> As long as nothing picks it via d_find_any_alias() and moves it somewhere
> manually.  The former might happen, the latter, AFAICS, doesn't - nothing
> like d_move() anywhere in sight...
>
> > You are missing that the OVL_E_UPPER_ALIAS flag is a property of
> > the overlay dentry, not a property of the inode.
> >
> > N lower hardlinks, the first copy up created an upper inode
> > all the rest of the N upper aliases to that upper inode are
> > created lazily.
> >
> > However, for obvious reasons, OVL_E_UPPER_ALIAS is not
> > well defined for a disconnected overlay dentry.
> > There should not be any code (I hope) that cares about
> > OVL_E_UPPER_ALIAS for a disconnected overlay dentry,
> > so I *think* ovl_dentry_set_upper_alias() in this code is moot.
> >
> > I need to look closer to verify, but please confirm my assumption
> > regarding the irrelevance of  DCACHE_OP_*REVALIDATE for a
> > disconnected non-dir dentry.
>
> Correct; we only care if it gets reconnected to the main tree.
> The fact that it's only for non-directories simplifies life a lot
> there.  Sorry, got confused by the work you do with ->d_flags
> and hadn't stopped to ask whether it's needed in the first place
> in there.
>
> OK, so... are there any reasons why simply calling d_obtain_alias()
> wouldn't do the right thing these days?

None that I can think of.

I will try it out and run the tests to see if I have missed something.

Thanks,
Amir.

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Amir Goldstein]

On Sat, Nov 11, 2023 at 10:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Nov 11, 2023 at 8:50 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Sat, Nov 11, 2023 at 08:31:11PM +0200, Amir Goldstein wrote:
> > > > in ovl_lookup(), and in case we have d_splice_alias() return a non-NULL
> > > > dentry we can simply copy it there.  Sure, somebody might race with
> > > > us, pick dentry from hash and call ->d_revalidate() before we notice that
> > > > DCACHE_OP_REVALIDATE could be cleaned.  So what?  That call of ->d_revalidate()
> > > > will find nothing to do and return 1.  Which is the effect of having
> > > > DCACHE_OP_REVALIDATE cleared, except for pointless method call.  Anyone
> > > > who finds that dentry after the flag is cleared will skip the call.
> > > > IOW, that race is harmless.
> > >
> > > Just a minute.
> > > Do you know that ovl_obtain_alias() is *only* used to obtain a disconnected
> > > non-dir overlayfs dentry?
> >
> > D'oh...
> >
> > > I think that makes all the analysis regarding race with d_splice_alias()
> > > moot. Right?
> >
> > Right you are.
> >
> > > Do DCACHE_OP_*REVALIDATE even matter for a disconnected
> > > non-dir dentry?
> >
> > As long as nothing picks it via d_find_any_alias() and moves it somewhere
> > manually.  The former might happen, the latter, AFAICS, doesn't - nothing
> > like d_move() anywhere in sight...
> >
> > > You are missing that the OVL_E_UPPER_ALIAS flag is a property of
> > > the overlay dentry, not a property of the inode.
> > >
> > > N lower hardlinks, the first copy up created an upper inode
> > > all the rest of the N upper aliases to that upper inode are
> > > created lazily.
> > >
> > > However, for obvious reasons, OVL_E_UPPER_ALIAS is not
> > > well defined for a disconnected overlay dentry.
> > > There should not be any code (I hope) that cares about
> > > OVL_E_UPPER_ALIAS for a disconnected overlay dentry,
> > > so I *think* ovl_dentry_set_upper_alias() in this code is moot.
> > >
> > > I need to look closer to verify, but please confirm my assumption
> > > regarding the irrelevance of  DCACHE_OP_*REVALIDATE for a
> > > disconnected non-dir dentry.
> >
> > Correct; we only care if it gets reconnected to the main tree.
> > The fact that it's only for non-directories simplifies life a lot
> > there.  Sorry, got confused by the work you do with ->d_flags
> > and hadn't stopped to ask whether it's needed in the first place
> > in there.
> >
> > OK, so... are there any reasons why simply calling d_obtain_alias()
> > wouldn't do the right thing these days?
>
> None that I can think of.
>
> I will try it out and run the tests to see if I have missed something.
>

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.

Thanks,
Amir.

From 3415a62597161b03b2db48ca195af34d25afc5d5 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Sun, 12 Nov 2023 08:55:57 +0200
Subject: [PATCH] ovl: stop using d_alloc_anon()/d_instantiate_anon()

Commit f9c34674bc60 ("vfs: factor out helpers d_instantiate_anon() and
d_alloc_anon()") was introduced so overlayfs could initialize a non-dir
disconnected overlay dentry before overlay inode is attached to it.

Since commit ("0af950f57fef ovl: move ovl_entry into ovl_inode"), all
ovl_obtain_alias() can do is set DCACHE_OP_*REVALIDATE flags in ->d_flags
and OVL_E_UPPER_ALIAS flag in ->d_fsdata.

The DCACHE_OP_*REVALIDATE flags and OVL_E_UPPER_ALIAS flag are irrelevant
for a disconnected non-dir dentry, so it is better to use d_obtain_alias()
instead of open coding it.

Since it has no more users, remove d_instantiate_anon() and do not export
d_alloc_anon().

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/dcache.c            |  7 -------
 fs/overlayfs/export.c  | 22 +---------------------
 include/linux/dcache.h |  1 -
 3 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index c82ae731df9a..8afa9d2b636f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1866,7 +1866,6 @@ struct dentry *d_alloc_anon(struct super_block *sb)
 {
        return __d_alloc(sb, NULL);
 }
-EXPORT_SYMBOL(d_alloc_anon);

 struct dentry *d_alloc_cursor(struct dentry * parent)
 {
@@ -2115,12 +2114,6 @@ static struct dentry
*__d_instantiate_anon(struct dentry *dentry,
        return res;
 }

-struct dentry *d_instantiate_anon(struct dentry *dentry, struct inode *inode)
-{
-       return __d_instantiate_anon(dentry, inode, true);
-}
-EXPORT_SYMBOL(d_instantiate_anon);
-
 static struct dentry *__d_obtain_alias(struct inode *inode, bool disconnected)
 {
        struct dentry *tmp;
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 7e16bbcad95e..f2f41d4fb5d4 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -320,27 +320,7 @@ static struct dentry *ovl_obtain_alias(struct
super_block *sb,
        if (upper)
                ovl_set_flag(OVL_UPPERDATA, inode);

-       dentry = d_find_any_alias(inode);
-       if (dentry)
-               goto out_iput;
-
-       dentry = d_alloc_anon(inode->i_sb);
-       if (unlikely(!dentry))
-               goto nomem;
-
-       if (upper_alias)
-               ovl_dentry_set_upper_alias(dentry);
-
-       ovl_dentry_init_reval(dentry, upper, OVL_I_E(inode));
-
-       return d_instantiate_anon(dentry, inode);
-
-nomem:
-       dput(dentry);
-       dentry = ERR_PTR(-ENOMEM);
-out_iput:
-       iput(inode);
-       return dentry;
+       return d_obtain_alias(inode);
 }

 /* Get the upper or lower dentry in stack whose on layer @idx */
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 3da2f0545d5d..c760553fb88a 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -221,7 +221,6 @@ extern seqlock_t rename_lock;
 extern void d_instantiate(struct dentry *, struct inode *);
 extern void d_instantiate_new(struct dentry *, struct inode *);
 extern struct dentry * d_instantiate_unique(struct dentry *, struct inode *);
-extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *);
 extern void __d_drop(struct dentry *dentry);
 extern void d_drop(struct dentry *dentry);
 extern void d_delete(struct dentry *);
-- 
2.34.1

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Al Viro]

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.

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.

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Al Viro]

On Sat, Nov 18, 2023 at 08:02:47PM +0000, Al Viro 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.
> 
> 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.

See #no-rebase-overlayfs.

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Amir Goldstein]

On Sat, Nov 18, 2023 at 10:02 PM Al Viro <viro@zeniv.linux.org.uk> 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.

Thanks,
Amir.

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Al Viro]

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@zeniv.linux.org.uk> 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?

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...

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Amir Goldstein]

On Sun, Nov 19, 2023 at 9:26 AM Al Viro <viro@zeniv.linux.org.uk> 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@zeniv.linux.org.uk> 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.

Re: [RFC][overlayfs] do we still need d_instantiate_anon() and export of d_alloc_anon()?

[Amir Goldstein]

> > 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.