* [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes @ 2020-07-13 10:57 Amir Goldstein 2020-07-13 10:57 ` [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir Amir Goldstein ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Amir Goldstein @ 2020-07-13 10:57 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Vivek Goyal, Josh England, linux-unionfs 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-13 10:57 [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein @ 2020-07-13 10:57 ` Amir Goldstein 2020-07-13 19:25 ` Vivek Goyal 2020-07-13 10:57 ` [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed Amir Goldstein 2020-07-14 9:29 ` [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein 2 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2020-07-13 10:57 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Vivek Goyal, Josh England, linux-unionfs 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-13 10:57 ` [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir Amir Goldstein @ 2020-07-13 19:25 ` Vivek Goyal 2020-07-14 3:28 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Vivek Goyal @ 2020-07-13 19:25 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Josh England, linux-unionfs 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-13 19:25 ` Vivek Goyal @ 2020-07-14 3:28 ` Amir Goldstein 2020-07-14 13:41 ` Vivek Goyal 0 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2020-07-14 3:28 UTC (permalink / raw) To: Vivek Goyal; +Cc: Miklos Szeredi, Josh England, overlayfs 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-14 3:28 ` Amir Goldstein @ 2020-07-14 13:41 ` Vivek Goyal 2020-07-14 14:05 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Vivek Goyal @ 2020-07-14 13:41 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Josh England, overlayfs 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-14 13:41 ` Vivek Goyal @ 2020-07-14 14:05 ` Amir Goldstein 2020-07-15 8:57 ` Miklos Szeredi 0 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2020-07-14 14:05 UTC (permalink / raw) To: Vivek Goyal; +Cc: Miklos Szeredi, Josh England, overlayfs 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-14 14:05 ` Amir Goldstein @ 2020-07-15 8:57 ` Miklos Szeredi 2020-07-15 9:12 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Miklos Szeredi @ 2020-07-15 8:57 UTC (permalink / raw) To: Amir Goldstein; +Cc: Vivek Goyal, Josh England, overlayfs 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir 2020-07-15 8:57 ` Miklos Szeredi @ 2020-07-15 9:12 ` Amir Goldstein 0 siblings, 0 replies; 14+ messages in thread From: Amir Goldstein @ 2020-07-15 9:12 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Vivek Goyal, Josh England, overlayfs 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed 2020-07-13 10:57 [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein 2020-07-13 10:57 ` [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir Amir Goldstein @ 2020-07-13 10:57 ` Amir Goldstein 2020-07-13 20:05 ` Vivek Goyal 2020-07-14 9:29 ` [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein 2 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2020-07-13 10:57 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Vivek Goyal, Josh England, linux-unionfs 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed 2020-07-13 10:57 ` [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed Amir Goldstein @ 2020-07-13 20:05 ` Vivek Goyal 2020-07-14 2:55 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Vivek Goyal @ 2020-07-13 20:05 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Josh England, linux-unionfs 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed 2020-07-13 20:05 ` Vivek Goyal @ 2020-07-14 2:55 ` Amir Goldstein 0 siblings, 0 replies; 14+ messages in thread From: Amir Goldstein @ 2020-07-14 2:55 UTC (permalink / raw) To: Vivek Goyal; +Cc: Miklos Szeredi, Josh England, overlayfs 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes 2020-07-13 10:57 [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein 2020-07-13 10:57 ` [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir Amir Goldstein 2020-07-13 10:57 ` [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed Amir Goldstein @ 2020-07-14 9:29 ` Amir Goldstein 2020-07-14 16:22 ` Vivek Goyal 2 siblings, 1 reply; 14+ messages in thread From: Amir Goldstein @ 2020-07-14 9:29 UTC (permalink / raw) To: Miklos Szeredi; +Cc: Vivek Goyal, Josh England, overlayfs 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes 2020-07-14 9:29 ` [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein @ 2020-07-14 16:22 ` Vivek Goyal 2020-07-14 16:57 ` Amir Goldstein 0 siblings, 1 reply; 14+ messages in thread From: Vivek Goyal @ 2020-07-14 16:22 UTC (permalink / raw) To: Amir Goldstein; +Cc: Miklos Szeredi, Josh England, overlayfs 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes 2020-07-14 16:22 ` Vivek Goyal @ 2020-07-14 16:57 ` Amir Goldstein 0 siblings, 0 replies; 14+ messages in thread From: Amir Goldstein @ 2020-07-14 16:57 UTC (permalink / raw) To: Vivek Goyal; +Cc: Miklos Szeredi, Josh England, overlayfs 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-15 9:12 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-13 10:57 [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein 2020-07-13 10:57 ` [PATCH RFC 1/2] ovl: invalidate dentry with deleted real dir Amir Goldstein 2020-07-13 19:25 ` Vivek Goyal 2020-07-14 3:28 ` Amir Goldstein 2020-07-14 13:41 ` Vivek Goyal 2020-07-14 14:05 ` Amir Goldstein 2020-07-15 8:57 ` Miklos Szeredi 2020-07-15 9:12 ` Amir Goldstein 2020-07-13 10:57 ` [PATCH RFC 2/2] ovl: invalidate dentry if lower was renamed Amir Goldstein 2020-07-13 20:05 ` Vivek Goyal 2020-07-14 2:55 ` Amir Goldstein 2020-07-14 9:29 ` [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes Amir Goldstein 2020-07-14 16:22 ` Vivek Goyal 2020-07-14 16:57 ` Amir Goldstein
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).