Re: [PATCH] ovl: fix WARN_ON nlink drop to zero

[Miklos Szeredi <miklos@szeredi.hu>]

On Mon, Mar 23, 2020 at 8:08 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Changes to underlying layers should not cause WARN_ON(), but this repro
> does:
>
>  mkdir w l u mnt
>  sudo mount -t overlay -o workdir=w,lowerdir=l,upperdir=u overlay mnt
>  touch mnt/h
>  ln u/h u/k
>  rm -rf mnt/k
>  rm -rf mnt/h
>  dmesg
>
>  ------------[ cut here ]------------
>  WARNING: CPU: 1 PID: 116244 at fs/inode.c:302 drop_nlink+0x28/0x40
>
> After upper hardlinks were added while overlay is mounted, unlinking all
> overlay hardlinks drops overlay nlink to zero before all upper inodes
> are unlinked.
>
> Detect too low i_nlink before unlink/rename and set the overlay nlink
> to the upper inode nlink (minus one for an index entry).
>
> Reported-by: Phasip <phasip@gmail.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/util.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> Miklos,
>
> This fix passed the reported reproducers (with index=off),
> overlay/034 with (index=on) and overlay/034 with s/LOWER/UPPER:
>
>  -lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_LOWER
>  +lowerdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
>   workdir=$OVL_BASE_SCRATCH_MNT/$OVL_WORK
>
> As well as the rest of overlay/quick group.
>
> I will post the overlay/034 fork as a separate test later.
>
> Thanks,
> Amir.
>
> diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
> index 36b60788ee47..e894a14857c7 100644
> --- a/fs/overlayfs/util.c
> +++ b/fs/overlayfs/util.c
> @@ -734,7 +734,9 @@ static void ovl_cleanup_index(struct dentry *dentry)
>  int ovl_nlink_start(struct dentry *dentry)
>  {
>         struct inode *inode = d_inode(dentry);
> +       struct inode *iupper = ovl_inode_upper(inode);
>         const struct cred *old_cred;
> +       int index_nlink;
>         int err;
>
>         if (WARN_ON(!inode))
> @@ -764,7 +766,26 @@ int ovl_nlink_start(struct dentry *dentry)
>         if (err)
>                 return err;
>
> -       if (d_is_dir(dentry) || !ovl_test_flag(OVL_INDEX, inode))
> +       if (d_is_dir(dentry))
> +               goto out;
> +
> +       /* index adds +1 to upper nlink */
> +       index_nlink = !!ovl_test_flag(OVL_INDEX, inode);
> +       if (iupper && (iupper->i_nlink - index_nlink) > inode->i_nlink) {

Racy with link/unlink directly on upper.  Possibly our original nlink
calculation is also racy in a similar way, need to look at that.

But that doesn't matter, as long as we don't get to zero nlink with
hashed aliases.  Since inode lock is held on overlay inode, the number
of hashed aliases cannot change, so that's a better way to address
this issue, I think.

> +               pr_warn_ratelimited("inode nlink too low (%pd2, ino=%lu, nlink=%u, upper_nlink=%u)\n",
> +                                   dentry, inode->i_ino, inode->i_nlink,
> +                                   iupper->i_nlink - index_nlink);

Why warn?  This is user triggerable, so the point is to not warn in this case.

Thanks,
Miklos