From nobody Mon Jan 03 05:31:57 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 55111193D6FA; Mon, 3 Jan 2022 05:31:58 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JS4B60P6zz3mLD; Mon, 3 Jan 2022 05:31:58 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id E16F31FE6D; Mon, 3 Jan 2022 05:31:57 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 2035VvJn097060; Mon, 3 Jan 2022 05:31:57 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 2035Vv5j097059; Mon, 3 Jan 2022 05:31:57 GMT (envelope-from git) Date: Mon, 3 Jan 2022 05:31:57 GMT Message-Id: <202201030531.2035Vv5j097059@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Alan Somers Subject: git: f09df4b05cf4 - stable/12 - fusefs: correctly handle an inode that changes file types List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: asomers X-Git-Repository: src X-Git-Refname: refs/heads/stable/12 X-Git-Reftype: branch X-Git-Commit: f09df4b05cf4b9f065e3db642666355a95c036e4 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1641187918; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=UeSo88PJtzRIY+1E7l/q5+tf7N2rOx8u1WKsfrnjFBo=; b=ThRPWWhgaoPsTjdlAxxAGEi/kFB1VXhKP+NP3H4781Z5yeJVQnkLGKDO7SHP3v6TIyqxia rd4YJiS+7i2ottdgfdmJBFKzeQYpWP+snPpxLwEwI9Zd0Kkm5C8MUm+coceOi5LiZzV+Ha 9wOZ1WE/OFyzBfviAVYCMWiIz4M/WL7WUI8bMAaGnU13YAhdqYZAaGSZkXpSJ8eutDCe3v CYw8SdFkZR8SME38ncdYtLxLB++a+RJIIH2XjIu/XHT7+SsCaAjJRzLIOVls4PRYiAOFSw j+cwaHBBtCoswIseofn9uQw9MhvFtKZN652SzpTr1LyA6fg/7p1PHoCx6DnUAg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1641187918; a=rsa-sha256; cv=none; b=UhPOeLbjduDcpy7Lgs+IgKq7NKLrBKXyMcz05aok+XBi9G1YEBSGM/svsTzZPrOHENo/A1 QJrp6MwFBWLH5a2o+yiNeFC3noL6vXbKYQ7nYXmuCEzotwrgUwytCrwsUmzgXrzvT5FpG8 TVKcapsMUI3qefdRZNx59B7AsYIO8gJB8V9kDBCmMRvhojCDokpT/VMgYiRjVOx9zUgbqe c7c5MEI97Yw5vLQPEM3hQZ8DWYNSgg+gldBZOTYAkhYEgmyqtIRNJEADFlHjovTYjrL5sZ tx93Zmw1cirfkMTQ/pDRS7WK5wOD+d7iEPyx6Anl/jSwiL5WF0UEpWf0zFk4xA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch stable/12 has been updated by asomers: URL: https://cgit.FreeBSD.org/src/commit/?id=f09df4b05cf4b9f065e3db642666355a95c036e4 commit f09df4b05cf4b9f065e3db642666355a95c036e4 Author: Alan Somers AuthorDate: 2021-12-06 05:43:17 +0000 Commit: Alan Somers CommitDate: 2022-01-03 05:15:38 +0000 fusefs: correctly handle an inode that changes file types Correctly handle the situation where a FUSE server unlinks a file, then creates a new file of a different type but with the same inode number. Previously fuse_vnop_lookup in this situation would return EAGAIN. But since it didn't call vgone(), the vnode couldn't be reused right away. Fix this by immediately calling vgone() and reallocating a new vnode. This problem can occur in three code paths, during VOP_LOOKUP, VOP_SETATTR, or following FUSE_GETATTR, which usually happens during VOP_GETATTR but can occur during other vops, too. Note that the correct response actually doesn't depend on whether the entry cache has expired. In fact, during VOP_LOOKUP, we can't even tell. Either it has expired already, or else the vnode got reclaimed by vnlru. Also, correct the error code during the VOP_SETATTR path. PR: 258022 Reported by: chogata@moosefs.pro Reviewed by: pfg Differential Revision: https://reviews.freebsd.org/D33283 (cherry picked from commit 25927e068fcbcac0a5111a881de723bd984b04b3) --- sys/fs/fuse/fuse_internal.c | 9 +++++--- sys/fs/fuse/fuse_node.c | 25 +++++++++++---------- tests/sys/fs/fusefs/getattr.cc | 50 ++++++++++++++++++++++++++++++++++++++++++ tests/sys/fs/fusefs/lookup.cc | 32 +++++++++++++++++++++------ tests/sys/fs/fusefs/setattr.cc | 47 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 21 deletions(-) diff --git a/sys/fs/fuse/fuse_internal.c b/sys/fs/fuse/fuse_internal.c index 90383759f1bd..47f641a046de 100644 --- a/sys/fs/fuse/fuse_internal.c +++ b/sys/fs/fuse/fuse_internal.c @@ -1232,12 +1232,15 @@ int fuse_internal_setattr(struct vnode *vp, struct vattr *vap, * STALE vnode, ditch * * The vnode has changed its type "behind our back". + * This probably means that the file got deleted and + * recreated on the server, with the same inode. * There's nothing really we can do, so let us just - * force an internal revocation and tell the caller to - * try again, if interested. + * return ENOENT. After all, the entry must not have + * existed in the recent past. If the user tries + * again, it will work. */ fuse_internal_vnode_disappear(vp); - err = EAGAIN; + err = ENOENT; } } if (err == 0) { diff --git a/sys/fs/fuse/fuse_node.c b/sys/fs/fuse/fuse_node.c index d616d834de28..368a031fc383 100644 --- a/sys/fs/fuse/fuse_node.c +++ b/sys/fs/fuse/fuse_node.c @@ -211,24 +211,27 @@ fuse_vnode_alloc(struct mount *mp, return (err); if (*vpp) { - if ((*vpp)->v_type != vtyp) { + if ((*vpp)->v_type == vtyp) { + /* Reuse a vnode that hasn't yet been reclaimed */ + MPASS((*vpp)->v_data != NULL); + MPASS(VTOFUD(*vpp)->nid == nodeid); + SDT_PROBE2(fusefs, , node, trace, 1, + "vnode taken from hash"); + return (0); + } else { /* - * STALE vnode! This probably indicates a buggy - * server, but it could also be the result of a race - * between FUSE_LOOKUP and another client's - * FUSE_UNLINK/FUSE_CREATE + * The inode changed types! If we get here, we can't + * tell whether the inode's entry cache had expired + * yet. So this could be the result of a buggy server, + * but more likely the server just reused an inode + * number following an entry cache expiration. */ SDT_PROBE3(fusefs, , node, stale_vnode, *vpp, vtyp, nodeid); fuse_internal_vnode_disappear(*vpp); + vgone(*vpp); lockmgr((*vpp)->v_vnlock, LK_RELEASE, NULL); - *vpp = NULL; - return (EAGAIN); } - MPASS((*vpp)->v_data != NULL); - MPASS(VTOFUD(*vpp)->nid == nodeid); - SDT_PROBE2(fusefs, , node, trace, 1, "vnode taken from hash"); - return (0); } fvdat = malloc(sizeof(*fvdat), M_FUSEVN, M_WAITOK | M_ZERO); switch (vtyp) { diff --git a/tests/sys/fs/fusefs/getattr.cc b/tests/sys/fs/fusefs/getattr.cc index fb91f8c049d0..6bca7e0af7c7 100644 --- a/tests/sys/fs/fusefs/getattr.cc +++ b/tests/sys/fs/fusefs/getattr.cc @@ -256,6 +256,56 @@ TEST_F(Getattr, ok) //FUSE can't set st_blksize until protocol 7.9 } +/* + * FUSE_GETATTR returns a different file type, even though the entry cache + * hasn't expired. This is a server bug! It probably means that the server + * removed the file and recreated it with the same inode but a different vtyp. + * The best thing fusefs can do is return ENOENT to the caller. After all, the + * entry must not have existed recently. + */ +TEST_F(Getattr, vtyp_conflict) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const uint64_t ino = 42; + struct stat sb; + sem_t sem; + + ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno); + + EXPECT_LOOKUP(FUSE_ROOT_ID, RELPATH) + .WillOnce(Invoke( + ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFREG | 0644; + out.body.entry.nodeid = ino; + out.body.entry.attr.nlink = 1; + out.body.entry.attr_valid = 0; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_CALL(*m_mock, process( + ResultOf([](auto in) { + return (in.header.opcode == FUSE_GETATTR && + in.body.getattr.getattr_flags == 0 && + in.header.nodeid == ino); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([](auto i __unused, auto& out) { + SET_OUT_HEADER_LEN(out, attr); + out.body.attr.attr.ino = ino; // Must match nodeid + out.body.attr.attr.mode = S_IFDIR | 0755; // Changed! + out.body.attr.attr.nlink = 2; + }))); + // We should reclaim stale vnodes + expect_forget(ino, 1, &sem); + + ASSERT_NE(0, stat(FULLPATH, &sb)); + EXPECT_EQ(errno, ENOENT); + + sem_wait(&sem); + sem_destroy(&sem); +} + TEST_F(Getattr_7_8, ok) { const char FULLPATH[] = "mountpoint/some_file.txt"; diff --git a/tests/sys/fs/fusefs/lookup.cc b/tests/sys/fs/fusefs/lookup.cc index cb9d0bb27527..d301990c2048 100644 --- a/tests/sys/fs/fusefs/lookup.cc +++ b/tests/sys/fs/fusefs/lookup.cc @@ -342,9 +342,10 @@ TEST_F(Lookup, subdir) ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); } -/* - * The server returns two different vtypes for the same nodeid. This is a bad - * server! But we shouldn't crash. +/* + * The server returns two different vtypes for the same nodeid. This is + * technically allowed if the entry's cache has already expired. + * https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=258022 */ TEST_F(Lookup, vtype_conflict) { @@ -354,12 +355,29 @@ TEST_F(Lookup, vtype_conflict) const char SECONDRELPATH[] = "bar"; uint64_t ino = 42; - expect_lookup(FIRSTRELPATH, ino, S_IFREG | 0644, 0, 1, UINT64_MAX); - expect_lookup(SECONDRELPATH, ino, S_IFDIR | 0755, 0, 1, UINT64_MAX); + EXPECT_LOOKUP(FUSE_ROOT_ID, FIRSTRELPATH) + .WillOnce(Invoke( + ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0644; + out.body.entry.nodeid = ino; + out.body.entry.attr.nlink = 1; + }))); + expect_lookup(SECONDRELPATH, ino, S_IFREG | 0755, 0, 1, UINT64_MAX); + // VOP_FORGET happens asynchronously, so it may or may not arrive + // before the test completes. + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_FORGET && + in.header.nodeid == ino && + in.body.forget.nlookup == 1); + }, Eq(true)), + _) + ).Times(AtMost(1)) + .WillOnce(Invoke([=](auto in __unused, auto &out __unused) { })); ASSERT_EQ(0, access(FIRSTFULLPATH, F_OK)) << strerror(errno); - ASSERT_EQ(-1, access(SECONDFULLPATH, F_OK)); - ASSERT_EQ(EAGAIN, errno); + EXPECT_EQ(0, access(SECONDFULLPATH, F_OK)) << strerror(errno); } TEST_F(Lookup_7_8, ok) diff --git a/tests/sys/fs/fusefs/setattr.cc b/tests/sys/fs/fusefs/setattr.cc index 48aa8385517f..e4458db9f8ee 100644 --- a/tests/sys/fs/fusefs/setattr.cc +++ b/tests/sys/fs/fusefs/setattr.cc @@ -724,6 +724,53 @@ TEST_F(Setattr, utimensat_utime_now) { EXPECT_EQ(now[1].tv_nsec, sb.st_mtim.tv_nsec); } +/* + * FUSE_SETATTR returns a different file type, even though the entry cache + * hasn't expired. This is a server bug! It probably means that the server + * removed the file and recreated it with the same inode but a different vtyp. + * The best thing fusefs can do is return ENOENT to the caller. After all, the + * entry must not have existed recently. + */ +TEST_F(Setattr, vtyp_conflict) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const uint64_t ino = 42; + uid_t newuser = 12345; + sem_t sem; + + ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno); + + EXPECT_LOOKUP(FUSE_ROOT_ID, RELPATH) + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFREG | 0777; + out.body.entry.nodeid = ino; + out.body.entry.entry_valid = UINT64_MAX; + }))); + + EXPECT_CALL(*m_mock, process( + ResultOf([](auto in) { + return (in.header.opcode == FUSE_SETATTR && + in.header.nodeid == ino); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, attr); + out.body.attr.attr.ino = ino; + out.body.attr.attr.mode = S_IFDIR | 0777; // Changed! + out.body.attr.attr.uid = newuser; + }))); + // We should reclaim stale vnodes + expect_forget(ino, 1, &sem); + + EXPECT_NE(0, chown(FULLPATH, newuser, -1)); + EXPECT_EQ(ENOENT, errno); + + sem_wait(&sem); + sem_destroy(&sem); +} + /* On a read-only mount, no attributes may be changed */ TEST_F(RofsSetattr, erofs) {