Re: Kernel warnings in fs/inode.c:302 drop_nlink+0x28/0x40

* Re: Kernel warnings in fs/inode.c:302 drop_nlink+0x28/0x40
       [not found] <CAFkON1U3cXdXFQYdkoQ3OQU+14GX7C88U6qre58vyfhrrFgKXw@mail.gmail.com>
@ 2020-03-23 12:52 ` Miklos Szeredi
  2020-03-23 13:23   ` Amir Goldstein
  2020-03-23 14:27 ` Miklos Szeredi
  1 sibling, 1 reply; 8+ messages in thread
From: Miklos Szeredi @ 2020-03-23 12:52 UTC (permalink / raw)
  To: Phasip; +Cc: overlayfs

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

On Mon, Mar 23, 2020 at 9:50 AM Phasip <phasip@gmail.com> wrote:
>
> Hello!
>
> I have stumbled upon two ways of producing kernel warnings when using the overlayfs, both seem to be results of the same issue.
>
> The issue seems to be related to handling of hard links that are created directly in the upperdir.
> Below is my system details and then two samples with a list of commands to reproduce and the corresponding kernel warning

Hi,

Thanks for the report.

The problem is that i_nlink is not kept in sync with changes to
underlying layers.   That would not in itself be an issue, since
modification of the underlying layers may result in
undefined/unexpected behavior.  The problem is that this manifests
itself as a kernel warning.

Since unlink/rename is synchronized on the victim inode (the one that
is getting removed) it is possible to detect this condition and
prevent drop_nlink() from being called.

Attached patch fixes both of your testcases.

We'll need an xfstests case for this as well.

Thanks,
Miklos

[-- Attachment #2: ovl-prevent-negative-nlink.patch --]
[-- Type: text/x-patch, Size: 1162 bytes --]

---
 fs/overlayfs/dir.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -819,6 +819,20 @@ static bool ovl_pure_upper(struct dentry
 	       !ovl_test_flag(OVL_WHITEOUTS, d_inode(dentry));
 }
 
+static void ovl_drop_nlink(struct inode *inode)
+{
+	struct dentry *alias = d_find_alias(inode);
+
+	dput(alias);
+	/*
+	 * Changes to underlying layers may cause i_nlink to lose sync with
+	 * reality.  In this case prevent the link count from going to zero
+	 * prematurely.
+	 */
+	if (inode->i_nlink > !!alias)
+		drop_nlink(inode);
+}
+
 static int ovl_do_remove(struct dentry *dentry, bool is_dir)
 {
 	int err;
@@ -856,7 +870,7 @@ static int ovl_do_remove(struct dentry *
 		if (is_dir)
 			clear_nlink(dentry->d_inode);
 		else
-			drop_nlink(dentry->d_inode);
+			ovl_drop_nlink(dentry->d_inode);
 	}
 	ovl_nlink_end(dentry);
 
@@ -1201,7 +1215,7 @@ static int ovl_rename(struct inode *oldd
 		if (new_is_dir)
 			clear_nlink(d_inode(new));
 		else
-			drop_nlink(d_inode(new));
+			ovl_drop_nlink(d_inode(new));
 	}
 
 	ovl_dir_modified(old->d_parent, ovl_type_origin(old) ||

^ permalink raw reply	[flat|nested] 8+ messages in thread