[PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes

[PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes

[Amir Goldstein]

Miklos,Vivek,

These patches are part of the new overlay "fsnotify snapshot" series
I have been working on.

Conterary to the trend to disallow underlying offline changes with more
configurations, I have seen that some people do want to be able to make
some "careful" underlying online changes and survive [1].

In the following patches, I argue for improving the robustness of
overlayfs in the face of online underlying changes, but I have not
really proved my claims, so feel free to challenge them.

I also remember we discussed several times about the conversion of
zero return value to -ESTALE, including in the linked thread.
I did not change this behavior, but I left a boolean 'strict', which
controls this behavior. I am using this boolean to relax strict behavior
for snapshot mount later in my snapshot series. Relaxing the strict
behavior for other use cases can be considered if someone comes up with
a valid use case.

Thoughts?

Thanks,
Amir.

[1] https://lore.kernel.org/linux-unionfs/CAOQ4uxiBmFdcueorKV7zwPLCDq4DE+H8x=8H1f7+3v3zysW9qA@mail.gmail.com/

Amir Goldstein (2):
  ovl: invalidate dentry with deleted real dir
  ovl: invalidate dentry if lower was renamed

 fs/overlayfs/export.c    |  1 +
 fs/overlayfs/namei.c     |  4 +++-
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     | 48 ++++++++++++++++++++++++++++++----------
 4 files changed, 42 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Amir Goldstein]

Changes to underlying layers while overlay in mounted result in
undefined behavior.  Therefore, we can change the behavior to
invalidate the overlay dentry on dcache lookup if one of the
underlying dentries was deleted since the dentry was composed.

Negative underlying dentries are not expected in overlay upper and
lower dentries.  If they are found it is probably dcache lookup racing
with an overlay unlink, before d_drop() was called on the overlay dentry.
IS_DEADDIR directories may be caused by underlying rmdir, so invalidate
overlay dentry on dcache lookup if we find those.

We preserve the legacy behaior of returning -ESTALE on invalid cache
for lower dentries, but we relax this behavior for upper dentries
that may be invalidated by a race with overlay unlink/rmdir.

This doesn't make live changes to underlying layers valid, because
invalid dentry stacks may still be referenced by open files, but it
reduces the window for possible bugs caused by underlying delete,
because lookup cannot return those invalid dentry stacks.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 06ec3cb977e6..f2c74387e05b 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -113,21 +113,42 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	return dentry;
 }
 
-static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
+static bool ovl_dentry_is_dead(struct dentry *d)
 {
+	return unlikely(!d->d_inode || IS_DEADDIR(d->d_inode));
+}
+
+static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
+			       bool is_upper)
+{
+	bool strict = !weak;
 	int ret = 1;
 
-	if (weak) {
+	/* Invalidate dentry if real was deleted since we found it */
+	if (ovl_dentry_is_dead(d)) {
+		ret = 0;
+		/* Raced with overlay unlink/rmdir? */
+		if (is_upper)
+			strict = false;
+	} else if (weak) {
 		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
-			ret =  d->d_op->d_weak_revalidate(d, flags);
+			ret = d->d_op->d_weak_revalidate(d, flags);
 	} else if (d->d_flags & DCACHE_OP_REVALIDATE) {
 		ret = d->d_op->d_revalidate(d, flags);
-		if (!ret) {
-			if (!(flags & LOOKUP_RCU))
-				d_invalidate(d);
-			ret = -ESTALE;
-		}
 	}
+
+	/*
+	 * Legacy overlayfs strict behavior is to return an error to user on
+	 * non-weak revalidate rather than retry the lookup, because underlying
+	 * layer changes are not expected. We may want to relax this in the
+	 * future either for upper only or also for lower.
+	 */
+	if (strict && !ret) {
+		if (!(flags & LOOKUP_RCU))
+			d_invalidate(d);
+		ret = -ESTALE;
+	}
+
 	return ret;
 }
 
@@ -141,11 +162,11 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
 
 	upper = ovl_dentry_upper(dentry);
 	if (upper)
-		ret = ovl_revalidate_real(upper, flags, weak);
+		ret = ovl_revalidate_real(upper, flags, weak, true);
 
 	for (i = 0; ret > 0 && i < oe->numlower; i++) {
 		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
-					  weak);
+					  weak, false);
 	}
 	return ret;
 }
-- 
2.17.1

Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Vivek Goyal]

On Mon, Jul 13, 2020 at 01:57:31PM +0300, Amir Goldstein wrote:
> Changes to underlying layers while overlay in mounted result in
> undefined behavior.  Therefore, we can change the behavior to
> invalidate the overlay dentry on dcache lookup if one of the
> underlying dentries was deleted since the dentry was composed.
> 
> Negative underlying dentries are not expected in overlay upper and
> lower dentries.  If they are found it is probably dcache lookup racing
> with an overlay unlink, before d_drop() was called on the overlay dentry.
> IS_DEADDIR directories may be caused by underlying rmdir, so invalidate
> overlay dentry on dcache lookup if we find those.

Can you elaborate a bit more on this race. Doesn't inode_lock_nested(dir)
protect against that. I see that both vfs_rmdir() and vfs_unlink()
happen with parent directory inode mutex held exclusively. And IIUC,
that should mean no further lookup()/->revalidate() must be in progress
on that dentry? I might very well be wrong, hence asking for more
details.

Thanks
Vivek

> 
> We preserve the legacy behaior of returning -ESTALE on invalid cache
> for lower dentries, but we relax this behavior for upper dentries
> that may be invalidated by a race with overlay unlink/rmdir.
> 
> This doesn't make live changes to underlying layers valid, because
> invalid dentry stacks may still be referenced by open files, but it
> reduces the window for possible bugs caused by underlying delete,
> because lookup cannot return those invalid dentry stacks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 06ec3cb977e6..f2c74387e05b 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -113,21 +113,42 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
>  	return dentry;
>  }
>  
> -static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
> +static bool ovl_dentry_is_dead(struct dentry *d)
>  {
> +	return unlikely(!d->d_inode || IS_DEADDIR(d->d_inode));
> +}
> +
> +static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
> +			       bool is_upper)
> +{
> +	bool strict = !weak;
>  	int ret = 1;
>  
> -	if (weak) {
> +	/* Invalidate dentry if real was deleted since we found it */
> +	if (ovl_dentry_is_dead(d)) {
> +		ret = 0;
> +		/* Raced with overlay unlink/rmdir? */
> +		if (is_upper)
> +			strict = false;

> +	} else if (weak) {
>  		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
> -			ret =  d->d_op->d_weak_revalidate(d, flags);
> +			ret = d->d_op->d_weak_revalidate(d, flags);
>  	} else if (d->d_flags & DCACHE_OP_REVALIDATE) {
>  		ret = d->d_op->d_revalidate(d, flags);
> -		if (!ret) {
> -			if (!(flags & LOOKUP_RCU))
> -				d_invalidate(d);
> -			ret = -ESTALE;
> -		}
>  	}
> +
> +	/*
> +	 * Legacy overlayfs strict behavior is to return an error to user on
> +	 * non-weak revalidate rather than retry the lookup, because underlying
> +	 * layer changes are not expected. We may want to relax this in the
> +	 * future either for upper only or also for lower.
> +	 */
> +	if (strict && !ret) {
> +		if (!(flags & LOOKUP_RCU))
> +			d_invalidate(d);
> +		ret = -ESTALE;
> +	}
> +
>  	return ret;
>  }
>  
> @@ -141,11 +162,11 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
>  
>  	upper = ovl_dentry_upper(dentry);
>  	if (upper)
> -		ret = ovl_revalidate_real(upper, flags, weak);
> +		ret = ovl_revalidate_real(upper, flags, weak, true);
>  
>  	for (i = 0; ret > 0 && i < oe->numlower; i++) {
>  		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
> -					  weak);
> +					  weak, false);
>  	}
>  	return ret;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Amir Goldstein]

On Mon, Jul 13, 2020 at 10:25 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Jul 13, 2020 at 01:57:31PM +0300, Amir Goldstein wrote:
> > Changes to underlying layers while overlay in mounted result in
> > undefined behavior.  Therefore, we can change the behavior to
> > invalidate the overlay dentry on dcache lookup if one of the
> > underlying dentries was deleted since the dentry was composed.
> >
> > Negative underlying dentries are not expected in overlay upper and
> > lower dentries.  If they are found it is probably dcache lookup racing
> > with an overlay unlink, before d_drop() was called on the overlay dentry.
> > IS_DEADDIR directories may be caused by underlying rmdir, so invalidate
> > overlay dentry on dcache lookup if we find those.
>
> Can you elaborate a bit more on this race. Doesn't inode_lock_nested(dir)
> protect against that. I see that both vfs_rmdir() and vfs_unlink()
> happen with parent directory inode mutex held exclusively. And IIUC,
> that should mean no further lookup()/->revalidate() must be in progress
> on that dentry? I might very well be wrong, hence asking for more
> details.
>

lookup_fast() looks in dcache without dir inode lock.
d_revalidate() is called to check if the found cached dentry is valid.

For example, ovl_remove_upper() can make an upper dentry negative
or upper dir inode S_DEAD (i.e. vfs_rmdir) just before calling d_drop()
to prevent overlay dentry from being found in fast cache lookup.

Unless I am missing something, that leaves a small window where
lookup_fast() can return an overlay dentry with negative/S_DEAD
upper dentry, which was not caused by illegitimate underlying fs
changes, so we must gracefully invalidate the dcache lookup
(return 0 from revalidate) in order to fallback to fs lookup.

Thanks,
Amir.

Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Vivek Goyal]

On Tue, Jul 14, 2020 at 06:28:41AM +0300, Amir Goldstein wrote:
> On Mon, Jul 13, 2020 at 10:25 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, Jul 13, 2020 at 01:57:31PM +0300, Amir Goldstein wrote:
> > > Changes to underlying layers while overlay in mounted result in
> > > undefined behavior.  Therefore, we can change the behavior to
> > > invalidate the overlay dentry on dcache lookup if one of the
> > > underlying dentries was deleted since the dentry was composed.
> > >
> > > Negative underlying dentries are not expected in overlay upper and
> > > lower dentries.  If they are found it is probably dcache lookup racing
> > > with an overlay unlink, before d_drop() was called on the overlay dentry.
> > > IS_DEADDIR directories may be caused by underlying rmdir, so invalidate
> > > overlay dentry on dcache lookup if we find those.
> >
> > Can you elaborate a bit more on this race. Doesn't inode_lock_nested(dir)
> > protect against that. I see that both vfs_rmdir() and vfs_unlink()
> > happen with parent directory inode mutex held exclusively. And IIUC,
> > that should mean no further lookup()/->revalidate() must be in progress
> > on that dentry? I might very well be wrong, hence asking for more
> > details.
> >
> 
> lookup_fast() looks in dcache without dir inode lock.
> d_revalidate() is called to check if the found cached dentry is valid.

Got it.

> 
> For example, ovl_remove_upper() can make an upper dentry negative
> or upper dir inode S_DEAD (i.e. vfs_rmdir) just before calling d_drop()
> to prevent overlay dentry from being found in fast cache lookup.
> 
> Unless I am missing something, that leaves a small window where
> lookup_fast() can return an overlay dentry with negative/S_DEAD
> upper dentry, which was not caused by illegitimate underlying fs
> changes, so we must gracefully invalidate the dcache lookup
> (return 0 from revalidate) in order to fallback to fs lookup.

So what's the side affect of this? I mean one even you make this change,
it is possible that on a cpu parallel unlink is going on and right
after d_revalidate() finishes, upper is marked negative (or directory
S_DEAD).

So this change will not plug the hole. It will just narrow it a bit?

/me is failing to see complete picture that what's the problem at
macro level and how this patch fixes it.

Thanks
Vivek

Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Amir Goldstein]

On Tue, Jul 14, 2020 at 4:41 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 06:28:41AM +0300, Amir Goldstein wrote:
> > On Mon, Jul 13, 2020 at 10:25 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > On Mon, Jul 13, 2020 at 01:57:31PM +0300, Amir Goldstein wrote:
> > > > Changes to underlying layers while overlay in mounted result in
> > > > undefined behavior.  Therefore, we can change the behavior to
> > > > invalidate the overlay dentry on dcache lookup if one of the
> > > > underlying dentries was deleted since the dentry was composed.
> > > >
> > > > Negative underlying dentries are not expected in overlay upper and
> > > > lower dentries.  If they are found it is probably dcache lookup racing
> > > > with an overlay unlink, before d_drop() was called on the overlay dentry.
> > > > IS_DEADDIR directories may be caused by underlying rmdir, so invalidate
> > > > overlay dentry on dcache lookup if we find those.
> > >
> > > Can you elaborate a bit more on this race. Doesn't inode_lock_nested(dir)
> > > protect against that. I see that both vfs_rmdir() and vfs_unlink()
> > > happen with parent directory inode mutex held exclusively. And IIUC,
> > > that should mean no further lookup()/->revalidate() must be in progress
> > > on that dentry? I might very well be wrong, hence asking for more
> > > details.
> > >
> >
> > lookup_fast() looks in dcache without dir inode lock.
> > d_revalidate() is called to check if the found cached dentry is valid.
>
> Got it.
>
> >
> > For example, ovl_remove_upper() can make an upper dentry negative
> > or upper dir inode S_DEAD (i.e. vfs_rmdir) just before calling d_drop()
> > to prevent overlay dentry from being found in fast cache lookup.
> >
> > Unless I am missing something, that leaves a small window where
> > lookup_fast() can return an overlay dentry with negative/S_DEAD
> > upper dentry, which was not caused by illegitimate underlying fs
> > changes, so we must gracefully invalidate the dcache lookup
> > (return 0 from revalidate) in order to fallback to fs lookup.
>
> So what's the side affect of this? I mean one even you make this change,
> it is possible that on a cpu parallel unlink is going on and right
> after d_revalidate() finishes, upper is marked negative (or directory
> S_DEAD).

No parallelism required.
The side effect is to reduce the attack/fuzzing vector for creating
bad things by deleting/renaming lower dentries.

>
> So this change will not plug the hole. It will just narrow it a bit?

Yes, but it has nothing to do with races it has only to do with
use cases (see blow).

>
> /me is failing to see complete picture that what's the problem at
> macro level and how this patch fixes it.
>

Today, if a user deletes/renames underlying lower that leaves
the overlayfs dentry in a vulnerable state.
I do not have a reproducer to OOPS the kernel with that, but
syzbot has created some crashes of similar nature in the past.

The fact is that all we can say about this scenario is:
"If the underlying filesystem is changed, the behavior of the overlay
 is undefined, though it will not result in a crash or deadlock."
and that second part cannot be proven to be true.

With the set of these two patches, a whole class of attacks is
pruned, because every attack that needs to get the vulnerable
denry via lookup will not get it. Attacks that use a fd to access
a vulnerable dentry may still work.

The confusing part about racing with ovl_unlink()/ovl_rmdir()
is not really important. It explains that an overlay dentry in the
middle of ovl_rmdir() (after vfs_rmdir() and before d_drop()) can
be found by parallel dcache lookup and "appear" like an underlying
change (upper was removed under the overlay).

I only mentioned that because we must not return -ESTALE in
that case, but if we remove the -ESTALE conversion, nothing bad will
happen. dcache lookup will get 0 from ovl_revalidate on that dentry
and re-do the lookup, which is exactly what would have happened
if lookup took place a few ms later after ovl_rmdir() called d_drop().

Thanks,
Amir.

Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Miklos Szeredi]

On Tue, Jul 14, 2020 at 4:05 PM Amir Goldstein <amir73il@gmail.com> wrote:

> Today, if a user deletes/renames underlying lower that leaves
> the overlayfs dentry in a vulnerable state.
> I do not have a reproducer to OOPS the kernel with that, but
> syzbot has created some crashes of similar nature in the past.

Can you back that up with references?

Don't misunderstand me, I'm all for making behavior more
deterministic, but I'd also like to fully understand the current
behavior.

Thanks,
Miklos

Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir

[Amir Goldstein]

On Wed, Jul 15, 2020 at 11:57 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Jul 14, 2020 at 4:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Today, if a user deletes/renames underlying lower that leaves
> > the overlayfs dentry in a vulnerable state.
> > I do not have a reproducer to OOPS the kernel with that, but
> > syzbot has created some crashes of similar nature in the past.
>
> Can you back that up with references?

What I meant by "similar nature" is the overlapping layers
shenanigans.
So no, I do not have any concrete evidence to reproducible
OOPS, but we both know that the bugs are there somewhere.
If not a proper OOPS then some WARN_ON must be possible.

>
> Don't misunderstand me, I'm all for making behavior more
> deterministic, but I'd also like to fully understand the current
> behavior.
>

So as I said, I needed those local fs change invalidations for the
snapshot use case and those patches are now in my branch
passing the snapshot tests.

I posted them for consideration, because they *seem* to
slightly improve things, even if not by a lot.

I can claim that they will buy us some more time before syzbot
evolves to finding an OOPS triggered by an underlying change,
but I do not have any real evidence to support this claim.

If you want me to take this one step further and verify overlay
dentry before ovl_lookup() and ovl_iterate() (anything else?)
I can do that.

ovl_lookup() on parent dentry with mangled lowerstack sounds
like a possible source of trouble.

Thanks,
Amir.

[PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed

[Amir Goldstein]

Changes to lower layer while overlay in mounted result in undefined
behavior.  Therefore, we can change the behavior to invalidate the
overlay dentry on dcache lookup if one of the dentries in the lowerstack
was renamed since the lowerstack was composed.

To be absolute certain that lower dentry was not renamed we would need to
know the redirect path that lead to it, but that is not necessary.
Instead, we just store the hash of the parent/name from when we composed
the stack, which gives a good enough probablity to detect a lower rename
and is much less complexity.

We do not provide this protection for upper dentries, because that would
require updating the hash on overlay initiated renames and that is harder
to implement with lockless lookup.

This doesn't make live changes to underlying layers valid, because
invalid dentry stacks may still be referenced by open files, but it
reduces the window for possible bugs caused by lower rename, because
lookup cannot return those invalid dentry stacks.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/export.c    |  1 +
 fs/overlayfs/namei.c     |  4 +++-
 fs/overlayfs/ovl_entry.h |  2 ++
 fs/overlayfs/super.c     | 17 ++++++++++-------
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 0e696f72cf65..7221b6226e26 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -319,6 +319,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	if (lower) {
 		oe->lowerstack->dentry = dget(lower);
 		oe->lowerstack->layer = lowerpath->layer;
+		oe->lowerstack->hash = lower->d_name.hash_len;
 	}
 	dentry->d_fsdata = oe;
 	if (upper_alias)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 3566282a9199..ae1c1216a038 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -375,7 +375,8 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
 	}
 	**stackp = (struct ovl_path){
 		.dentry = origin,
-		.layer = &ofs->layers[i]
+		.layer = &ofs->layers[i],
+		.hash = origin->d_name.hash_len,
 	};
 
 	return 0;
@@ -968,6 +969,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		} else {
 			stack[ctr].dentry = this;
 			stack[ctr].layer = lower.layer;
+			stack[ctr].hash = this->d_name.hash_len;
 			ctr++;
 		}
 
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index b429c80879ee..557f1782f53b 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -42,6 +42,8 @@ struct ovl_layer {
 struct ovl_path {
 	const struct ovl_layer *layer;
 	struct dentry *dentry;
+	/* Hash of the lower parent/name when we found it */
+	u64 hash;
 };
 
 /* private information held for overlayfs's superblock */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f2c74387e05b..4b7cb2d98203 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -119,13 +119,13 @@ static bool ovl_dentry_is_dead(struct dentry *d)
 }
 
 static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
-			       bool is_upper)
+			       bool is_upper, u64 hash)
 {
 	bool strict = !weak;
 	int ret = 1;
 
-	/* Invalidate dentry if real was deleted since we found it */
-	if (ovl_dentry_is_dead(d)) {
+	/* Invalidate dentry if real was deleted/renamed since we found it */
+	if (ovl_dentry_is_dead(d) || (hash && hash != d->d_name.hash_len)) {
 		ret = 0;
 		/* Raced with overlay unlink/rmdir? */
 		if (is_upper)
@@ -156,17 +156,18 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
 					unsigned int flags, bool weak)
 {
 	struct ovl_entry *oe = dentry->d_fsdata;
+	struct ovl_path *lower = oe->lowerstack;
 	struct dentry *upper;
 	unsigned int i;
 	int ret = 1;
 
 	upper = ovl_dentry_upper(dentry);
 	if (upper)
-		ret = ovl_revalidate_real(upper, flags, weak, true);
+		ret = ovl_revalidate_real(upper, flags, weak, true, 0);
 
-	for (i = 0; ret > 0 && i < oe->numlower; i++) {
-		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
-					  weak, false);
+	for (i = 0; ret > 0 && i < oe->numlower; i++, lower++) {
+		ret = ovl_revalidate_real(lower->dentry, flags, weak, false,
+					  lower->hash);
 	}
 	return ret;
 }
@@ -1652,6 +1653,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 	for (i = 0; i < numlower; i++) {
 		oe->lowerstack[i].dentry = dget(stack[i].dentry);
 		oe->lowerstack[i].layer = &ofs->layers[i+1];
+		/* layer root should not be invalidated by rename */
+		oe->lowerstack->hash = 0;
 	}
 
 out:
-- 
2.17.1

Re: [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed

[Vivek Goyal]

On Mon, Jul 13, 2020 at 01:57:32PM +0300, Amir Goldstein wrote:
> Changes to lower layer while overlay in mounted result in undefined
> behavior.  Therefore, we can change the behavior to invalidate the
> overlay dentry on dcache lookup if one of the dentries in the lowerstack
> was renamed since the lowerstack was composed.
> 
> To be absolute certain that lower dentry was not renamed we would need to
> know the redirect path that lead to it, but that is not necessary.
> Instead, we just store the hash of the parent/name from when we composed
> the stack, which gives a good enough probablity to detect a lower rename
> and is much less complexity.
> 
> We do not provide this protection for upper dentries, because that would
> require updating the hash on overlay initiated renames and that is harder
> to implement with lockless lookup.
> 
> This doesn't make live changes to underlying layers valid, because
> invalid dentry stacks may still be referenced by open files, but it
> reduces the window for possible bugs caused by lower rename, because
> lookup cannot return those invalid dentry stacks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/export.c    |  1 +
>  fs/overlayfs/namei.c     |  4 +++-
>  fs/overlayfs/ovl_entry.h |  2 ++
>  fs/overlayfs/super.c     | 17 ++++++++++-------
>  4 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 0e696f72cf65..7221b6226e26 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -319,6 +319,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
>  	if (lower) {
>  		oe->lowerstack->dentry = dget(lower);
>  		oe->lowerstack->layer = lowerpath->layer;
> +		oe->lowerstack->hash = lower->d_name.hash_len;
>  	}
>  	dentry->d_fsdata = oe;
>  	if (upper_alias)
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 3566282a9199..ae1c1216a038 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -375,7 +375,8 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
>  	}
>  	**stackp = (struct ovl_path){
>  		.dentry = origin,
> -		.layer = &ofs->layers[i]
> +		.layer = &ofs->layers[i],
> +		.hash = origin->d_name.hash_len,
>  	};
>  
>  	return 0;
> @@ -968,6 +969,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  		} else {
>  			stack[ctr].dentry = this;
>  			stack[ctr].layer = lower.layer;
> +			stack[ctr].hash = this->d_name.hash_len;
>  			ctr++;
>  		}
>  
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b429c80879ee..557f1782f53b 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -42,6 +42,8 @@ struct ovl_layer {
>  struct ovl_path {
>  	const struct ovl_layer *layer;
>  	struct dentry *dentry;
> +	/* Hash of the lower parent/name when we found it */
> +	u64 hash;
>  };
>  
>  /* private information held for overlayfs's superblock */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index f2c74387e05b..4b7cb2d98203 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -119,13 +119,13 @@ static bool ovl_dentry_is_dead(struct dentry *d)
>  }
>  
>  static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
> -			       bool is_upper)
> +			       bool is_upper, u64 hash)
>  {
>  	bool strict = !weak;
>  	int ret = 1;
>  
> -	/* Invalidate dentry if real was deleted since we found it */
> -	if (ovl_dentry_is_dead(d)) {
> +	/* Invalidate dentry if real was deleted/renamed since we found it */
> +	if (ovl_dentry_is_dead(d) || (hash && hash != d->d_name.hash_len)) {

So if lower hash_len changes, on local filesystem we will return -ESTALE?
I am assuming we did that for remote filesystems and now we will do
that for local filesystems as well?

Thanks
Vivek

>  		ret = 0;
>  		/* Raced with overlay unlink/rmdir? */
>  		if (is_upper)
> @@ -156,17 +156,18 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
>  					unsigned int flags, bool weak)
>  {
>  	struct ovl_entry *oe = dentry->d_fsdata;
> +	struct ovl_path *lower = oe->lowerstack;
>  	struct dentry *upper;
>  	unsigned int i;
>  	int ret = 1;
>  
>  	upper = ovl_dentry_upper(dentry);
>  	if (upper)
> -		ret = ovl_revalidate_real(upper, flags, weak, true);
> +		ret = ovl_revalidate_real(upper, flags, weak, true, 0);
>  
> -	for (i = 0; ret > 0 && i < oe->numlower; i++) {
> -		ret = ovl_revalidate_real(oe->lowerstack[i].dentry, flags,
> -					  weak, false);
> +	for (i = 0; ret > 0 && i < oe->numlower; i++, lower++) {
> +		ret = ovl_revalidate_real(lower->dentry, flags, weak, false,
> +					  lower->hash);
>  	}
>  	return ret;
>  }
> @@ -1652,6 +1653,8 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>  	for (i = 0; i < numlower; i++) {
>  		oe->lowerstack[i].dentry = dget(stack[i].dentry);
>  		oe->lowerstack[i].layer = &ofs->layers[i+1];
> +		/* layer root should not be invalidated by rename */
> +		oe->lowerstack->hash = 0;
>  	}
>  
>  out:
> -- 
> 2.17.1
> 

Re: [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed

[Amir Goldstein]

On Mon, Jul 13, 2020 at 11:05 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Jul 13, 2020 at 01:57:32PM +0300, Amir Goldstein wrote:
> > Changes to lower layer while overlay in mounted result in undefined
> > behavior.  Therefore, we can change the behavior to invalidate the
> > overlay dentry on dcache lookup if one of the dentries in the lowerstack
> > was renamed since the lowerstack was composed.
> >
> > To be absolute certain that lower dentry was not renamed we would need to
> > know the redirect path that lead to it, but that is not necessary.
> > Instead, we just store the hash of the parent/name from when we composed
> > the stack, which gives a good enough probablity to detect a lower rename
> > and is much less complexity.
> >
> > We do not provide this protection for upper dentries, because that would
> > require updating the hash on overlay initiated renames and that is harder
> > to implement with lockless lookup.
> >
> > This doesn't make live changes to underlying layers valid, because
> > invalid dentry stacks may still be referenced by open files, but it
> > reduces the window for possible bugs caused by lower rename, because
> > lookup cannot return those invalid dentry stacks.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/overlayfs/export.c    |  1 +
> >  fs/overlayfs/namei.c     |  4 +++-
> >  fs/overlayfs/ovl_entry.h |  2 ++
> >  fs/overlayfs/super.c     | 17 ++++++++++-------
> >  4 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index 0e696f72cf65..7221b6226e26 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -319,6 +319,7 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
> >       if (lower) {
> >               oe->lowerstack->dentry = dget(lower);
> >               oe->lowerstack->layer = lowerpath->layer;
> > +             oe->lowerstack->hash = lower->d_name.hash_len;
> >       }
> >       dentry->d_fsdata = oe;
> >       if (upper_alias)
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 3566282a9199..ae1c1216a038 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -375,7 +375,8 @@ int ovl_check_origin_fh(struct ovl_fs *ofs, struct ovl_fh *fh, bool connected,
> >       }
> >       **stackp = (struct ovl_path){
> >               .dentry = origin,
> > -             .layer = &ofs->layers[i]
> > +             .layer = &ofs->layers[i],
> > +             .hash = origin->d_name.hash_len,
> >       };
> >
> >       return 0;
> > @@ -968,6 +969,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >               } else {
> >                       stack[ctr].dentry = this;
> >                       stack[ctr].layer = lower.layer;
> > +                     stack[ctr].hash = this->d_name.hash_len;
> >                       ctr++;
> >               }
> >
> > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> > index b429c80879ee..557f1782f53b 100644
> > --- a/fs/overlayfs/ovl_entry.h
> > +++ b/fs/overlayfs/ovl_entry.h
> > @@ -42,6 +42,8 @@ struct ovl_layer {
> >  struct ovl_path {
> >       const struct ovl_layer *layer;
> >       struct dentry *dentry;
> > +     /* Hash of the lower parent/name when we found it */
> > +     u64 hash;
> >  };
> >
> >  /* private information held for overlayfs's superblock */
> > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> > index f2c74387e05b..4b7cb2d98203 100644
> > --- a/fs/overlayfs/super.c
> > +++ b/fs/overlayfs/super.c
> > @@ -119,13 +119,13 @@ static bool ovl_dentry_is_dead(struct dentry *d)
> >  }
> >
> >  static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak,
> > -                            bool is_upper)
> > +                            bool is_upper, u64 hash)
> >  {
> >       bool strict = !weak;
> >       int ret = 1;
> >
> > -     /* Invalidate dentry if real was deleted since we found it */
> > -     if (ovl_dentry_is_dead(d)) {
> > +     /* Invalidate dentry if real was deleted/renamed since we found it */
> > +     if (ovl_dentry_is_dead(d) || (hash && hash != d->d_name.hash_len)) {
>
> So if lower hash_len changes, on local filesystem we will return -ESTALE?
> I am assuming we did that for remote filesystems and now we will do
> that for local filesystems as well?
>

That is correct.
Although I am personally in favor of the non 'strict' behavior.

Thanks,
Amir.

Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes

[Amir Goldstein]

On Mon, Jul 13, 2020 at 1:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Miklos,Vivek,
>
> These patches are part of the new overlay "fsnotify snapshot" series
> I have been working on.
>
> Conterary to the trend to disallow underlying offline changes with more
> configurations, I have seen that some people do want to be able to make
> some "careful" underlying online changes and survive [1].
>
> In the following patches, I argue for improving the robustness of
> overlayfs in the face of online underlying changes, but I have not
> really proved my claims, so feel free to challenge them.
>

This wasn't actually working unless underlying fs was remote, because
overlayfs clears the DCACHE_OP_REVALIDATE flags in that case.

I added this hunk for revalidate of local lower fs with nfs_export=on:

@@ -111,6 +111,10 @@ void ovl_dentry_update_reval(struct dentry
*dentry, struct dentry *upperdentry,
        for (i = 0; i < oe->numlower; i++)
                flags |= oe->lowerstack[i].dentry->d_flags;

+       /* Revalidate on local fs lower changes */
+       if (oe->numlower && ovl_verify_lower(dentry->d_sb))
+               flags |= mask;
+


> I also remember we discussed several times about the conversion of
> zero return value to -ESTALE, including in the linked thread.
> I did not change this behavior, but I left a boolean 'strict', which
> controls this behavior. I am using this boolean to relax strict behavior
> for snapshot mount later in my snapshot series. Relaxing the strict
> behavior for other use cases can be considered if someone comes up with
> a valid use case.
>

After giving this some more though, I came to a conclusion that it is actually
wrong to convert 0 to error because 0 could mean cache timeout expiry
or other things that do not imply anyone has made underlying changes.
I see that fuse_dentry_revalidate() handles timeout expiry internally and
other network filesystems may also do that, but there is nothing in the
"contract" about not returning 0 if entry MAY be valid.
Am I wrong?

I can even think of a network filesystem that marks its own dentry for lazy
revalidate after some local changes, so this behavior is even more dodgy
when dealing with remote upper fs.

So I added another patch to remove the conversion 0 => -ESTALE.

Pushed these patches to
https://github.com/amir73il/linux/commits/ovl-revalidate:
 ovl: invalidate dentry if lower was renamed
 ovl: invalidate dentry with deleted real dir
 ovl: do not return error on remote dentry cache expiry

Will wait for Miklos' feedback before posting.

Thanks,
Amir.

Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes

[Vivek Goyal]

On Tue, Jul 14, 2020 at 12:29:08PM +0300, Amir Goldstein wrote:
> On Mon, Jul 13, 2020 at 1:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Miklos,Vivek,
> >
> > These patches are part of the new overlay "fsnotify snapshot" series
> > I have been working on.
> >
> > Conterary to the trend to disallow underlying offline changes with more
> > configurations, I have seen that some people do want to be able to make
> > some "careful" underlying online changes and survive [1].
> >
> > In the following patches, I argue for improving the robustness of
> > overlayfs in the face of online underlying changes, but I have not
> > really proved my claims, so feel free to challenge them.
> >
> 
> This wasn't actually working unless underlying fs was remote, because
> overlayfs clears the DCACHE_OP_REVALIDATE flags in that case.
> 
> I added this hunk for revalidate of local lower fs with nfs_export=on:
> 
> @@ -111,6 +111,10 @@ void ovl_dentry_update_reval(struct dentry
> *dentry, struct dentry *upperdentry,
>         for (i = 0; i < oe->numlower; i++)
>                 flags |= oe->lowerstack[i].dentry->d_flags;
> 
> +       /* Revalidate on local fs lower changes */
> +       if (oe->numlower && ovl_verify_lower(dentry->d_sb))
> +               flags |= mask;
> +
> 
> 
> > I also remember we discussed several times about the conversion of
> > zero return value to -ESTALE, including in the linked thread.
> > I did not change this behavior, but I left a boolean 'strict', which
> > controls this behavior. I am using this boolean to relax strict behavior
> > for snapshot mount later in my snapshot series. Relaxing the strict
> > behavior for other use cases can be considered if someone comes up with
> > a valid use case.
> >
> 
> After giving this some more though, I came to a conclusion that it is actually
> wrong to convert 0 to error because 0 could mean cache timeout expiry
> or other things that do not imply anyone has made underlying changes.
> I see that fuse_dentry_revalidate() handles timeout expiry internally and
> other network filesystems may also do that, but there is nothing in the
> "contract" about not returning 0 if entry MAY be valid.
> Am I wrong?
> 
> I can even think of a network filesystem that marks its own dentry for lazy
> revalidate after some local changes, so this behavior is even more dodgy
> when dealing with remote upper fs.
> 
> So I added another patch to remove the conversion 0 => -ESTALE.
> 
> Pushed these patches to
> https://github.com/amir73il/linux/commits/ovl-revalidate:
>  ovl: invalidate dentry if lower was renamed
>  ovl: invalidate dentry with deleted real dir
>  ovl: do not return error on remote dentry cache expiry

So what's the end goal. We don't want to return error during lookup,
if underlying layer changed and instead force re-lookup. And re-lookup
might work in a slightly different way and that's allowed?

IOW, we don't want to return error if we detected lower layer change
and just continue to run. It might fail later or it might subtly
change behavior in some way (inode number reporting etc). But that's
fine?

What will documentation says. Lower layer changes are allowed? Or
we say lower layer changes are not allowed but overlay will not
flag it at runtime even if we detect it.

Thanks
Vivek

Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes

[Amir Goldstein]

On Tue, Jul 14, 2020 at 7:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jul 14, 2020 at 12:29:08PM +0300, Amir Goldstein wrote:
> > On Mon, Jul 13, 2020 at 1:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > Miklos,Vivek,
> > >
> > > These patches are part of the new overlay "fsnotify snapshot" series
> > > I have been working on.
> > >
> > > Conterary to the trend to disallow underlying offline changes with more
> > > configurations, I have seen that some people do want to be able to make
> > > some "careful" underlying online changes and survive [1].
> > >
> > > In the following patches, I argue for improving the robustness of
> > > overlayfs in the face of online underlying changes, but I have not
> > > really proved my claims, so feel free to challenge them.
> > >
> >
> > This wasn't actually working unless underlying fs was remote, because
> > overlayfs clears the DCACHE_OP_REVALIDATE flags in that case.
> >
> > I added this hunk for revalidate of local lower fs with nfs_export=on:
> >
> > @@ -111,6 +111,10 @@ void ovl_dentry_update_reval(struct dentry
> > *dentry, struct dentry *upperdentry,
> >         for (i = 0; i < oe->numlower; i++)
> >                 flags |= oe->lowerstack[i].dentry->d_flags;
> >
> > +       /* Revalidate on local fs lower changes */
> > +       if (oe->numlower && ovl_verify_lower(dentry->d_sb))
> > +               flags |= mask;
> > +
> >
> >
> > > I also remember we discussed several times about the conversion of
> > > zero return value to -ESTALE, including in the linked thread.
> > > I did not change this behavior, but I left a boolean 'strict', which
> > > controls this behavior. I am using this boolean to relax strict behavior
> > > for snapshot mount later in my snapshot series. Relaxing the strict
> > > behavior for other use cases can be considered if someone comes up with
> > > a valid use case.
> > >
> >
> > After giving this some more though, I came to a conclusion that it is actually
> > wrong to convert 0 to error because 0 could mean cache timeout expiry
> > or other things that do not imply anyone has made underlying changes.
> > I see that fuse_dentry_revalidate() handles timeout expiry internally and
> > other network filesystems may also do that, but there is nothing in the
> > "contract" about not returning 0 if entry MAY be valid.
> > Am I wrong?
> >
> > I can even think of a network filesystem that marks its own dentry for lazy
> > revalidate after some local changes, so this behavior is even more dodgy
> > when dealing with remote upper fs.
> >
> > So I added another patch to remove the conversion 0 => -ESTALE.
> >
> > Pushed these patches to
> > https://github.com/amir73il/linux/commits/ovl-revalidate:
> >  ovl: invalidate dentry if lower was renamed
> >  ovl: invalidate dentry with deleted real dir
> >  ovl: do not return error on remote dentry cache expiry
>
> So what's the end goal. We don't want to return error during lookup,
> if underlying layer changed and instead force re-lookup. And re-lookup
> might work in a slightly different way and that's allowed?
>

Correct.

> IOW, we don't want to return error if we detected lower layer change
> and just continue to run. It might fail later or it might subtly
> change behavior in some way (inode number reporting etc). But that's
> fine?
>

Correct.

> What will documentation says. Lower layer changes are allowed? Or
> we say lower layer changes are not allowed but overlay will not
> flag it at runtime even if we detect it.
>

We do not change our statement:
"Changes to the underlying filesystems while part of a mounted overlay
filesystem are not allowed.  If the underlying filesystem is changed,
the behavior of the overlay is undefined, though it will not result in
a crash or deadlock."

Only we are slightly more credible when saying won't result in
crash or deadlock...

Note that we can perform similar validations before certain operations,
for example, validate parent stack before performing ovl_lookup()
to further fortify the code against xxxat(dfd, ...) syscalls.

Another operation that works on an fd with dentry that may have lowerstack
that might be relevant is ovl_iterate(), but I wouldn't go dealing with them
one by one without a plan.

The reason I posted those RFC patches is because I wrote them anyway
for my snapshot series, so though they might be of interest.

Thanks,
Amir.