[PATCH] ovl: fix WARN_ON nlink drop to zero

[PATCH] ovl: fix WARN_ON nlink drop to zero

[Amir Goldstein]

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) {
+		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);
+		/*
+		 * Upper hardlinks were added while overlay is mounted.
+		 * Unlinking all overlay hardlinks would drop overlay nlink to
+		 * zero before all upper inodes are unlinked.  As a safety
+		 * measure, when that situation is detected, set the overlay
+		 * nlink to the upper inode nlink minus one for the index entry.
+		 */
+		set_nlink(d_inode(dentry), iupper->i_nlink - index_nlink);
+	}
+
+	if (!index_nlink)
 		goto out;
 
 	old_cred = ovl_override_creds(dentry->d_sb);
-- 
2.17.1

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

[Miklos Szeredi]

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

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

[Amir Goldstein]

On Tue, Mar 24, 2020 at 11:48 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> 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.
>

OK. Just as long as there is sufficient commentary in ovl_drop_nlink().

> > +               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.
>

I thought the point was that user cannot trigger WARN_ON().
I though pr_warn on non fatal filesystem inconsistency, like the one in
ovl_cleanup_index() is fare game.
The purpose of the warning is to alert the admin of a corrupted overlayfs
and possibly run fsck.overlay (when it becomes an official tool).

Thanks,
Amir.

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

[Miklos Szeredi]

On Tue, Mar 24, 2020 at 11:20 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Mar 24, 2020 at 11:48 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, Mar 23, 2020 at 8:08 PM Amir Goldstein <amir73il@gmail.com> wrote:

> > > +               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.
> >
>
> I thought the point was that user cannot trigger WARN_ON().
> I though pr_warn on non fatal filesystem inconsistency, like the one in
> ovl_cleanup_index() is fare game.
> The purpose of the warning is to alert the admin of a corrupted overlayfs
> and possibly run fsck.overlay (when it becomes an official tool).

Right, warning is okay if kernel detects an fsck'able inconsistency.
But it's probably better not warn in the !OVL_INDEX case...

Thanks,
Miklos