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

[Amir Goldstein <amir73il@gmail.com>]

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.