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@xxxxxxxxx>
> ---
> 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
>
