From: Eric Van Hensbergen <ericvh@kernel.org>
To: linux-kernel@vger.kernel.org
Cc: v9fs@lists.linux.dev, linux_oss@crudebyte.com,
asmadeus@codewreck.org, rminnich@gmail.com, lucho@ionkov.net,
Eric Van Hensbergen <ericvh@kernel.org>
Subject: [PATCH] fs/9p: fix inode nlink accounting
Date: Sun, 07 Jan 2024 19:07:52 +0000 [thread overview]
Message-ID: <20240107-fix-nlink-handling-v1-1-8b1f65ebc9b2@kernel.org> (raw)
I was running some regressions and noticed a (race-y) kernel warning that
happens when nlink becomes less than zero. Looking through the code
it looks like we aren't good about protecting the inode lock when
manipulating nlink and some code that was added several years ago to
protect against bugs in underlying file systems nlink handling didn't
look quite right either. I took a look at what NFS was doing and tried to
follow similar approaches in the 9p code.
Signed-off-by: Eric Van Hensbergen <ericvh@kernel.org>
---
fs/9p/vfs_inode.c | 32 ++++++++++++++++++++++++--------
fs/9p/vfs_inode_dotl.c | 2 ++
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index b845ee18a80b..9723c3cbae38 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -508,9 +508,12 @@ static int v9fs_at_to_dotl_flags(int flags)
/**
* v9fs_dec_count - helper functon to drop i_nlink.
*
- * If a directory had nlink <= 2 (including . and ..), then we should not drop
- * the link count, which indicates the underlying exported fs doesn't maintain
- * nlink accurately. e.g.
+ * Put a guards around this so we are only dropping nlink
+ * if it would be valid. This prevents bugs from an underlying
+ * filesystem implementations from triggering kernel WARNs. We'll
+ * still print 9p debug messages if the underlying filesystem is wrong.
+ *
+ * known underlying filesystems which might exhibit this issue:
* - overlayfs sets nlink to 1 for merged dir
* - ext4 (with dir_nlink feature enabled) sets nlink to 1 if a dir has more
* than EXT4_LINK_MAX (65000) links.
@@ -519,8 +522,13 @@ static int v9fs_at_to_dotl_flags(int flags)
*/
static void v9fs_dec_count(struct inode *inode)
{
- if (!S_ISDIR(inode->i_mode) || inode->i_nlink > 2)
+ spin_lock(&inode->i_lock);
+ if (inode->i_nlink > 0)
drop_nlink(inode);
+ else
+ p9_debug(P9_DEBUG_ERROR, "WARNING: nlink is already 0 inode %p\n",
+ inode);
+ spin_unlock(&inode->i_lock);
}
/**
@@ -566,8 +574,9 @@ static int v9fs_remove(struct inode *dir, struct dentry *dentry, int flags)
* link count
*/
if (flags & AT_REMOVEDIR) {
+ spin_lock(&inode->i_lock);
clear_nlink(inode);
- v9fs_dec_count(dir);
+ spin_unlock(&inode->i_lock);
} else
v9fs_dec_count(inode);
@@ -713,7 +722,9 @@ static int v9fs_vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
err = PTR_ERR(fid);
fid = NULL;
} else {
+ spin_lock(&dir->i_lock);
inc_nlink(dir);
+ spin_unlock(&dir->i_lock);
v9fs_invalidate_inode_attr(dir);
}
@@ -962,14 +973,19 @@ v9fs_vfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
error_locked:
if (!retval) {
if (new_inode) {
- if (S_ISDIR(new_inode->i_mode))
+ if (S_ISDIR(new_inode->i_mode)) {
+ spin_lock(&new_inode->i_lock);
clear_nlink(new_inode);
- else
+ spin_unlock(&new_inode->i_lock);
+ } else
v9fs_dec_count(new_inode);
}
if (S_ISDIR(old_inode->i_mode)) {
- if (!new_inode)
+ if (!new_inode) {
+ spin_lock(&new_dir->i_lock);
inc_nlink(new_dir);
+ spin_unlock(&new_dir->i_lock);
+ }
v9fs_dec_count(old_dir);
}
v9fs_invalidate_inode_attr(old_inode);
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index c7319af2f471..6cc037f726e7 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -427,7 +427,9 @@ static int v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap,
v9fs_set_create_acl(inode, fid, dacl, pacl);
d_instantiate(dentry, inode);
}
+ spin_lock(&dir->i_lock);
inc_nlink(dir);
+ spin_unlock(&dir->i_lock);
v9fs_invalidate_inode_attr(dir);
error:
p9_fid_put(fid);
---
base-commit: 5254c0cbc92d2a08e75443bdb914f1c4839cdf5a
change-id: 20240107-fix-nlink-handling-3c0646f5d927
Best regards,
--
Eric Van Hensbergen <ericvh@kernel.org>
next reply other threads:[~2024-01-07 19:08 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-07 19:07 Eric Van Hensbergen [this message]
2024-01-08 11:19 ` [PATCH] fs/9p: fix inode nlink accounting asmadeus
2024-01-08 12:08 ` Christian Schoenebeck
2024-01-08 14:12 ` Eric Van Hensbergen
2024-01-08 14:55 ` Christian Schoenebeck
2024-01-08 21:37 ` asmadeus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240107-fix-nlink-handling-v1-1-8b1f65ebc9b2@kernel.org \
--to=ericvh@kernel.org \
--cc=asmadeus@codewreck.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux_oss@crudebyte.com \
--cc=lucho@ionkov.net \
--cc=rminnich@gmail.com \
--cc=v9fs@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).