fs/9p: Reuse inode based on path (in addition to qid)

Hi 9p-fs maintainers,

(CC'ing Landlock and Fanotify/inotify people as this affects both use cases, although most of my investigation has been on Landlock)

Previously [1], I noticed that when using 9pfs filesystems, the Landlock LSM is blocking access even for files / directories allowed by rules, and that this has something to do with 9pfs creating new inodes despite Landlock holding a reference to the existing one. Because Landlock uses inodes' in-memory state (i_security) to identify allowed fs objects/hierarchies, this causes Landlock to partially break on 9pfs, at least in uncached mode, which is the default:

    # mount -t 9p -o trans=virtio test /mnt
    # env LL_FS_RO=/etc:/usr:/bin:/lib:/mnt/readme LL_FS_RW= /sandboxer bash
    Executing the sandboxed command...
    bash: cannot set terminal process group (164): Inappropriate ioctl for device
    bash: no job control in this shell
    # cat /mnt/readme
    cat: /mnt/readme: Permission denied

This, however, works if somebody is holding onto the dentry (and it also works with cache=loose), as in both cases the inode is reused:

    # tail -f /mnt/readme &
    [1] 196
    # env LL_FS_RO=/etc:/usr:/bin:/lib:/mnt/readme LL_FS_RW= /sandboxer bash
    Executing the sandboxed command...
    bash: cannot set terminal process group (164): Inappropriate ioctl for device
    bash: no job control in this shell
    # cat /mnt/readme
    aa

It also works on directories if one have a shell that cd into the directory. Note that this means only certain usage of Landlock are affected - for example, sandboxing applications that takes a list of files to allow, landlocks itself, then evecve. On the other hand, this does not affect applications that opens a file, then Landlocks itself while keeping the file it needs open.

While the above is a very simple example, this is problematic in real-world use cases if Landlock is used to sandox applications on system that has files mounted via 9pfs, or use 9pfs as the root filesystem. In addition, this also affects fanotify / inotify when using inode mark (for local access):

    root@d8c28a676d72:/# ./fanotify-basic-open /readme & # on virtiofs
    [1] 173
    root@d8c28a676d72:/# cat readme
    aa
    FAN_OPEN: File /readme
    root@d8c28a676d72:/# mount -t 9p -o trans=virtio test /mnt
    root@d8c28a676d72:/# ./fanotify-basic-open /mnt/readme & # on 9pfs
    [2] 176
    root@d8c28a676d72:/# cat /mnt/readme
    aa
    root@d8c28a676d72:/#

Same can be demonstrated with inotifywait. The source code for fanotify-basic-open, adopted from the fanotify man page, is available at the end of this email.

Note that this is not a security bug for Landlock since it can only cause legitimate access to be denied, but might be a problem for fanotify perm (although I do recognize that using perm on individual inodes is already perhaps a bit unreliable?)

It seems that there was an attempt at making 9pfs reuse inodes, based on qid.path, however it was reverted [2] due to issues with servers that present duplicate qids, for example on a QEMU host that has multiple filesystems mounted under a single 9pfs export without multidevs=remap, or in the case of other servers that doesn't necessarily support remapping qids ([3] and more). I've done some testing on v6.12-rc4 which has the simplified 9pfs inode code before it was reverted, and found that Landlock works (however, we of course then have the issue demonstrated in [2]).

Unrelated to the above problem, it also seems like even with the revert in [2], because in cached mode inode are still reused based on qid (and type, version (aka mtime), etc), the setup mentioned in [2] still causes problems in th latest kernel with cache=loose:

host # cd /tmp/linux-test
host # mkdir m1 m2
host # mount -t tmpfs tmpfs m1
host # mount -t tmpfs tmpfs m2
host # mkdir m1/dir m2/dir # needs to be done together so that they have the same mtime host # echo foo > m1/dir/foo
host # echo bar > m2/dir/bar

guest # mount -t 9p -o trans=virtio,cache=loose test /mnt
guest # cd /mnt/m1/dir
qemu-system-x86_64: warning: 9p: Multiple devices detected in same VirtFS export, which might lead to file ID collisions and severe misbehaviours on guest! You should either use a separate export for each device shared from host or use virtfs option 'multidevs=remap'! guest # ls
foo
guest # ls /mnt/m2/dir
foo # <- should be bar
guest # uname -a
Linux d8c28a676d72 6.14.0-dev #92 SMP PREEMPT_DYNAMIC Sun Apr 6 18:47:54 BST 2025 x86_64 GNU/Linux

With the above in mind, I have a proposal for 9pfs to:
1. Reuse inodes even in uncached mode
2. However, reuse them based on qid.path AND the actual pathname, by doing the appropriate testing in v9fs_test(_new)?_inode(_dotl)?

The main problem here is how to store the pathname in a sensible way and tie it to the inode. For now I opted with an array of names acquired with take_dentry_name_snapshot, which reuses the same memory as the dcache to store the actual strings, but doesn't tie the lifetime of the dentry with the inode (I thought about holding a reference to the dentry in the v9fs_inode, but it seemed like a wrong approach and would cause dentries to not be evicted/released).

Maybe ideally there could be a general way for filesystems to tell VFS/dcache to "pin" a dentry as long as the inode is alive, but still allows the dentry and inode to be evicted based on memory pressure? In fact, if the dentry is alive, we might not even need to do this in 9pfs, as we will automatically get the same inode pointed to by the dentry.

Currently this pathname is immutable once attached to an inode, and therefore it is not protected by locks or RCU, but this might have to change for us to support renames that preserve the inode on next access. This is not in this patchset yet. Also let me know if I've missed any locks etc (I'm new to VFS, or for that matter, the kernel).

Storing one pathname per inode also means we don't reuse the same inode for hardlinks -- maybe this can be fixed as well in a future version, if this approach sounds good?

From some QEMU documentation I read [4] it seems like there is a plan to resolve these kind of problems in a new version of the protocol, by expanding the qid to include the filesystem identifier of a file on the host, so maybe this can be disabled after a successful protocol version check with the host? For now this is implemented as a default option, inodeident=path, which can be set to 'none' to instead get the previous behaviour.

This patchset is still a bit of a work in progress, and I've not tested the performance impact. It currently uses strncmp to compare paths but this might be able to be optimized into a hash comparison first. It should also normally only need to do it for one pair of filenames, as the test is only done if qid.path matches in the first place.

Also, currently the inode is not reused in cached mode if mtime changed AND the dentry was evicted -- I considered removing the qid.version test in v9fs_test_inode_dotl but I think perhaps care needs to be taken to ensure we can refresh an inode that potentially has data cached? This is a TODO for this patchset.

Another TODO is to maybe add support for case-insensitive matching?

For this patch series I've tested modifying files on both host and guest, changing a file from regular file to dir then back while preserving ino, keeping a file open in the guest with a fd, and using Landlock (which results in an ihold but does not keep the dentry) then trying to access the file from both inside and outside the Landlocked shell.

Let me know what's your thought on this -- if this is a viable approach I'm happy to work on it more and do more testing. The main motivation behind this is getting Landlock to work "out of the box" on 9pfs.

This patch series was based on, and tested on v6.14 + [5]

Kind regards,
Tingmao

[1]: https://github.com/landlock-lsm/linux/issues/45
[2]: https://lore.kernel.org/all/[email protected]/
[3]: https://lore.kernel.org/all/20240923100508.GA32066@willie-the-truck/
[4]: https://wiki.qemu.org/Documentation/9p#Protocol_Plans
[5]: https://lore.kernel.org/all/[email protected]/

fanotify-basic-open.c:

    #define _GNU_SOURCE /* Needed to get O_LARGEFILE definition */
    #include <errno.h>
    #include <fcntl.h>
    #include <limits.h>
    #include <poll.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <sys/fanotify.h>
    #include <unistd.h>

    /* Read all available fanotify events from the file descriptor 'fd'. */

    static void handle_events(int fd)
    {
        const struct fanotify_event_metadata *metadata;
        struct fanotify_event_metadata buf[200];
        ssize_t size;
        char path[PATH_MAX];
        ssize_t path_len;
        char procfd_path[PATH_MAX];
        struct fanotify_response response;

        for (;;) {
            size = read(fd, buf, sizeof(buf));
            if (size == -1 && errno != EAGAIN) {
                perror("read");
                exit(EXIT_FAILURE);
            }

            /* Check if end of available data reached. */

            if (size <= 0)
                break;

            /* Point to the first event in the buffer. */

            metadata = buf;

            /* Loop over all events in the buffer. */

            while (FAN_EVENT_OK(metadata, size)) {
                if (metadata->fd >= 0) {
                    if (metadata->mask & FAN_OPEN) {
                        printf("FAN_OPEN: ");
                    }

                    /* Retrieve and print pathname of the accessed file. */

                    snprintf(procfd_path, sizeof(procfd_path),
                        "/proc/self/fd/%d", metadata->fd);
                    path_len = readlink(procfd_path, path,
                                sizeof(path) - 1);
                    if (path_len == -1) {
                        perror("readlink");
                        exit(EXIT_FAILURE);
                    }

                    path[path_len] = '\0';
                    printf("File %s\n", path);

                    /* Close the file descriptor of the event. */

                    close(metadata->fd);
                }

                /* Advance to next event. */

                metadata = FAN_EVENT_NEXT(metadata, size);
            }
        }
    }

    int main(int argc, char *argv[])
    {
        char buf;
        int fd, poll_num;
        nfds_t nfds;
        struct pollfd fds[2];

        /* Check mount point is supplied. */

        if (argc != 2) {
            fprintf(stderr, "Usage: %s FILE\n", argv[0]);
            exit(EXIT_FAILURE);
        }

        fd = fanotify_init(0, O_RDONLY | O_LARGEFILE);
        if (fd == -1) {
            perror("fanotify_init");
            exit(EXIT_FAILURE);
        }

        if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_INODE, FAN_OPEN, AT_FDCWD,
                argv[1]) == -1) {
            perror("fanotify_mark");
            exit(EXIT_FAILURE);
        }

        while (1) {
            handle_events(fd);
        }
    }

Tingmao Wang (6):
fs/9p: Add ability to identify inode by path for .L fs/9p: add default option for path-based inodes
fs/9p: Hide inodeident=path from show_options as it is the default fs/9p: Add ability to identify inode by path for non-.L fs/9p: .L: Refresh stale inodes on reuse
fs/9p: non-.L: Refresh stale inodes on reuse

 fs/9p/Makefile         |   3 +-
 fs/9p/ino_path.c       | 114 ++++++++++++++++++++++++++++++++++++++
 fs/9p/v9fs.c           |  33 ++++++++++-
 fs/9p/v9fs.h           |  63 +++++++++++++++------
 fs/9p/vfs_inode.c      | 122 +++++++++++++++++++++++++++++++++++------
 fs/9p/vfs_inode_dotl.c | 108 +++++++++++++++++++++++++++++++++---
 fs/9p/vfs_super.c      |  10 +++-
 7 files changed, 406 insertions(+), 47 deletions(-)
 create mode 100644 fs/9p/ino_path.c

base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557
prerequisite-patch-id: 12dc6676db52ff32eed082b1e5d273f297737f61
prerequisite-patch-id: 93ab54c52a41fa44b8d0baf55df949d0ad27e99a
prerequisite-patch-id: 5f558bf969e6eaa3d011c98de0806ca8ad369efe --
2.39.5

fs/9p/ino_path.c new file +13 -0

+struct v9fs_ino_path *make_ino_path(struct dentry *dentry)
+{
+	struct v9fs_ino_path *path;
+	size_t path_components = 0;
+	struct dentry *curr = dentry;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+
+    /* Don't include the root dentry */
+	while (curr->d_parent != curr) {

fs/9p/ino_path.c new file +20 -0

+bool ino_path_compare(struct v9fs_ino_path *ino_path,
+			     struct dentry *dentry)
+{
+	struct dentry *curr = dentry;
+	struct qstr *curr_name;
+	struct name_snapshot *compare;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+	for (i = ino_path->nr_components - 1; i >= 0; i--) {
+		if (curr->d_parent == curr) {
+			/* We're supposed to have more components to walk */
+			rcu_read_unlock();
+			return false;
+		}
+		curr_name = &curr->d_name;
+		compare = &ino_path->names[i];
+		/*

Quoted 9 lines · Tingmao Wang

fs/9p/ino_path.c new file +9 -0

+struct v9fs_ino_path *make_ino_path(struct dentry *dentry)
+{
+	struct v9fs_ino_path *path;
+	size_t path_components = 0;
+	struct dentry *curr = dentry;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+

fs/9p/ino_path.c line 38

Quoted 8 lines

fs/9p/ino_path.c new file +8 -0

+	rcu_read_lock();
+
+    /* Don't include the root dentry */
+	while (curr->d_parent != curr) {
+		path_components++;
+		curr = curr->d_parent;
+	}
+	if (WARN_ON(path_components > SSIZE_MAX)) {

fs/9p/ino_path.c line 44

Quoted 6 lines

fs/9p/ino_path.c new file +6 -0

+		rcu_read_unlock();
+		return NULL;
+	}
+
+	path = kmalloc(struct_size(path, names, path_components),
+		       GFP_KERNEL);

Blocking allocation under rcu_read_lock().

Quoted 26 lines · Tingmao Wang

fs/9p/ino_path.c new file +26 -0

+bool ino_path_compare(struct v9fs_ino_path *ino_path,
+			     struct dentry *dentry)
+{
+	struct dentry *curr = dentry;
+	struct qstr *curr_name;
+	struct name_snapshot *compare;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+	for (i = ino_path->nr_components - 1; i >= 0; i--) {
+		if (curr->d_parent == curr) {
+			/* We're supposed to have more components to walk */
+			rcu_read_unlock();
+			return false;
+		}
+		curr_name = &curr->d_name;
+		compare = &ino_path->names[i];
+		/*
+		 * We can't use hash_len because it is salted with the parent
+		 * dentry pointer.  We could make this faster by pre-computing our
+		 * own hashlen for compare and ino_path outside, probably.
+		 */
+		if (curr_name->len != compare->name.len) {
+			rcu_read_unlock();

fs/9p/ino_path.c line 102

Quoted 4 lines

fs/9p/ino_path.c new file +4 -0

+			return false;
+		}
+		if (strncmp(curr_name->name, compare->name.name,
+			    curr_name->len) != 0) {

... without any kind of protection for curr_name. Incidentally, what about rename()? Not a cross-directory one, just one that changes the name of a subdirectory within the same parent?

[...]

fs/9p/vfs_inode_dotl.c +10 -1

+struct iget_data {
+	struct p9_stat_dotl *st;
+	struct dentry *dentry;
+};
+
 static int v9fs_test_inode_dotl(struct inode *inode, void *data)
 {
 	struct v9fs_inode *v9inode = V9FS_I(inode);
-	struct p9_stat_dotl *st = (struct p9_stat_dotl *)data;
+	struct p9_stat_dotl *st = ((struct iget_data *)data)->st;
+	struct dentry *dentry = ((struct iget_data *)data)->dentry;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
 
 	/* don't match inode of different type */
 	if (inode_wrong_type(inode, st->st_mode))

@@ -74,22 +81,74 @@ static int v9fs_test_inode_dotl(struct inode *inode, void *data)
 
 	if (v9inode->qid.path != st->qid.path)
 		return 0;
+
+	if (v9fs_inode_ident_path(v9ses)) {

fs/9p/vfs_inode_dotl.c +36 -1

 {
+	struct v9fs_inode *v9inode = V9FS_I(inode);
+	struct p9_stat_dotl *st = ((struct iget_data *)data)->st;
+	struct dentry *dentry = ((struct iget_data *)data)->dentry;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
+
+	/*
+	 * Don't reuse inode of different type, even if we have
+	 * inodeident=path and path matches.
+	 */
+	if (inode_wrong_type(inode, st->st_mode))
+		return 0;
+
+	/*
+	 * We're only getting here if QID2INO stays the same anyway, so
+	 * mirroring the qid checks in v9fs_test_inode_dotl
+	 * (but maybe that check is unnecessary anyway? at least on 64bit)
+	 */
+
+	if (v9inode->qid.type != st->qid.type)
+		return 0;
+
+	if (v9inode->qid.path != st->qid.path)
+		return 0;
+
+	if (v9fs_inode_ident_path(v9ses) && dentry && v9inode->path) {
+		if (ino_path_compare(V9FS_I(inode)->path, dentry)) {
+			p9_debug(P9_DEBUG_VFS,
+				 "Reusing inode %p based on path match", inode);
+			return 1;
+		}
+	}
+
 	return 0;
 }
 
 static int v9fs_set_inode_dotl(struct inode *inode,  void *data)
 {
 	struct v9fs_inode *v9inode = V9FS_I(inode);
-	struct p9_stat_dotl *st = (struct p9_stat_dotl *)data;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
+	struct iget_data *idata = data;
+	struct p9_stat_dotl *st = idata->st;
+	struct dentry *dentry = idata->dentry;
 
 	memcpy(&v9inode->qid, &st->qid, sizeof(st->qid));

fs/9p/vfs_inode_dotl.c +0 -0

 	return 0;
 }
 
[...]

[...]

fs/9p/vfs_inode.c +5 -2

-static int v9fs_set_inode(struct inode *inode,  void *data)
+static int v9fs_set_inode(struct inode *inode, void *data)
 {
 	struct v9fs_inode *v9inode = V9FS_I(inode);
-	struct p9_wstat *st = (struct p9_wstat *)data;
+	struct v9fs_session_info *v9ses = v9fs_inode2v9ses(inode);
+	struct iget_data *idata = data;
+	struct p9_wstat *st = idata->st;
+	struct dentry *dentry = idata->dentry;
 

return 0;
}

[...]

Hi Al, thanks for the review :) I haven't had the chance to properly think about this until today, so apologies for the delay.

Quoted 17 lines · Al Viro

Quoted 15 lines · Tingmao Wang

fs/9p/ino_path.c new file +15 -0

+struct v9fs_ino_path *make_ino_path(struct dentry *dentry)
+{
+	struct v9fs_ino_path *path;
+	size_t path_components = 0;
+	struct dentry *curr = dentry;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+
+    /* Don't include the root dentry */
+	while (curr->d_parent != curr) {
+		path_components++;
+		curr = curr->d_parent;

Quoted 2 lines

fs/9p/ino_path.c line 38

Quoted 2 lines

fs/9p/ino_path.c new file +2 -0

+	}
+	if (WARN_ON(path_components > SSIZE_MAX)) {

(Looking at this again I think this check is a bit bogus. I don't know how would it be possible at all for us to have > SSIZE_MAX deep directories especially since each level requires a dentry allocation, but even if this check is actually useful, it should be in the while loop, before each path_components++)

Quoted 9 lines

fs/9p/ino_path.c line 44

Quoted 7 lines

fs/9p/ino_path.c new file +6 -0

+		rcu_read_unlock();
+		return NULL;
+	}
+
+	path = kmalloc(struct_size(path, names, path_components),
+		       GFP_KERNEL);

Blocking allocation under rcu_read_lock().

I think my first instinct of how to fix this, if the original code is correct barring this allocation issue, would be to take rcu read lock twice (first walk to calculate how much to allocate, then second walk to actually take the snapshots). We should be safe to rcu_read_unlock() in the middle as long as caller has a reference to the target dentry (this needs to be true even if we just do one rcu_read_lock() anyway), and we can start a parent walk again. The v9fs rename_sem should ensure we see the same path again.

Alternatively, we can use dget_parent to do the walk, and not lock RCU at all. We still need to walk twice tho, to know how much to allocate. But for now I will keep the current approach.

New version:

/*
* Must hold rename_sem due to traversing parents. Caller must hold
* reference to dentry.
*/
struct v9fs_ino_path *make_ino_path(struct dentry *dentry)

{
	struct v9fs_ino_path *path;
	size_t path_components = 0;
	struct dentry *curr = dentry;
	ssize_t i;

	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
	might_sleep(); /* Allocation below might block */

	rcu_read_lock();

	/* Don't include the root dentry */
	while (curr->d_parent != curr) {
		if (WARN_ON(path_components >= SSIZE_MAX)) {
			rcu_read_unlock();
			return NULL;
		}
		path_components++;
		curr = curr->d_parent;
	}

	/*
	 * Allocation can block so don't do it in RCU (and because the

	 * allocation might be large, since name_snapshot leaves space for
	 * inline str, not worth trying GFP_ATOMIC)
	 */
	rcu_read_unlock();

	path = kmalloc(struct_size(path, names, path_components), GFP_KERNEL);
	if (!path) {
		rcu_read_unlock();

		return NULL;
	}

	path->nr_components = path_components;
	curr = dentry;

	rcu_read_lock();
	for (i = path_components - 1; i >= 0; i--) {
		take_dentry_name_snapshot(&path->names[i], curr);
		curr = curr->d_parent;
	}
	WARN_ON(curr != curr->d_parent);
	rcu_read_unlock();
	return path;
}

How does this look?

Quoted 27 lines · Tingmao Wang

Quoted 26 lines

fs/9p/ino_path.c new file +26 -0

+bool ino_path_compare(struct v9fs_ino_path *ino_path,
+			     struct dentry *dentry)
+{
+	struct dentry *curr = dentry;
+	struct qstr *curr_name;
+	struct name_snapshot *compare;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+	for (i = ino_path->nr_components - 1; i >= 0; i--) {
+		if (curr->d_parent == curr) {
+			/* We're supposed to have more components to walk */
+			rcu_read_unlock();
+			return false;
+		}
+		curr_name = &curr->d_name;
+		compare = &ino_path->names[i];
+		/*
+		 * We can't use hash_len because it is salted with the parent
+		 * dentry pointer.  We could make this faster by pre-computing our
+		 * own hashlen for compare and ino_path outside, probably.
+		 */
+		if (curr_name->len != compare->name.len) {
+			rcu_read_unlock();

Quoted 8 lines

fs/9p/ino_path.c line 102

Quoted 4 lines

fs/9p/ino_path.c new file +4 -0

+			return false;
+		}
+		if (strncmp(curr_name->name, compare->name.name,
+			    curr_name->len) != 0) {

... without any kind of protection for curr_name. Incidentally, what about rename()? Not a cross-directory one, just one that changes the name of a subdirectory within the same parent?

As far as I can tell, in v9fs_vfs_rename, v9ses->rename_sem is taken for both same-parent and different parent renames, so I think we're safe here (and hopefully for any v9fs dentries, nobody should be causing d_name to change except for ourselves when we call d_move in v9fs_vfs_rename? If yes then because we also take v9ses->rename_sem, in theory we should be fine here...?)

(Let me know if I missed anything. I'm assuming only the filesystem "owning" a dentry should d_move/d_exchange the dentry.)

However, I see that there is a d_same_name function in dcache.c which is slightly more careful (but still requires the caller to check the dentry seqcount, which we do not need to because of the reasoning above), and in hindsight I think that is probably the more proper way to do this comparison (and will also handle case-insensitivity, although I've not explored if this is applicable to 9pfs).

New version:

/*
* Must hold rename_sem due to traversing parents
*/
bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)

{
	struct dentry *curr = dentry;
	struct name_snapshot *compare;
	ssize_t i;

	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);

	rcu_read_lock();
	for (i = ino_path->nr_components - 1; i >= 0; i--) {
		if (curr->d_parent == curr) {
			/* We're supposed to have more components to walk */
			rcu_read_unlock();
			return false;
		}
		compare = &ino_path->names[i];
		if (!d_same_name(curr, curr->d_parent, &compare->name)) {
			rcu_read_unlock();
			return false;
		}
		curr = curr->d_parent;

	}
	rcu_read_unlock();
	if (curr != curr->d_parent) {
		/* dentry is deeper than ino_path */
		return false;
	}
	return true;
}

If you think this is not enough, can you suggest what would be needed? I'm thinking maybe we can check dentry seqcount to be safe, but from earlier discussion in "bpf path iterator" my impression is that that is VFS internal data - can we use it here (if needed)?

(I assume, from looking at the code, just having a reference does not prevent d_name from changing)

Unrelated to the above problem, it also seems like even with the revert in [2], because in cached mode inode are still reused based on qid (and type, version (aka mtime), etc), the setup mentioned in [2] still causes problems in th latest kernel with cache=loose:

With the above in mind, I have a proposal for 9pfs to:
1. Reuse inodes even in uncached mode
2. However, reuse them based on qid.path AND the actual pathname, by doing the appropriate testing in v9fs_test(_new)?_inode(_dotl)?

The main problem here is how to store the pathname in a sensible way and tie it to the inode. For now I opted with an array of names acquired with take_dentry_name_snapshot, which reuses the same memory as the dcache to store the actual strings, but doesn't tie the lifetime of the dentry with the inode (I thought about holding a reference to the dentry in the v9fs_inode, but it seemed like a wrong approach and would cause dentries to not be evicted/released).

Storing one pathname per inode also means we don't reuse the same inode for hardlinks -- maybe this can be fixed as well in a future version, if this approach sounds good?

Sorry for the delay...

Tingmao Wang wrote on Sun, Apr 06, 2025 at 09:43:01PM +0100:

Quoted 4 lines

Unrelated to the above problem, it also seems like even with the revert in [2], because in cached mode inode are still reused based on qid (and type, version (aka mtime), etc), the setup mentioned in [2] still causes problems in th latest kernel with cache=loose:

cache=loose is "you're on your own", I think it's fine to keep as is, especially given qemu can handle it with multidevs=remap if required

Sorry for the delay...

Tingmao Wang wrote on Sun, Apr 06, 2025 at 09:43:01PM +0100:

Quoted 4 lines

Unrelated to the above problem, it also seems like even with the revert in [2], because in cached mode inode are still reused based on qid (and type, version (aka mtime), etc), the setup mentioned in [2] still causes problems in th latest kernel with cache=loose:

cache=loose is "you're on your own", I think it's fine to keep as is, especially given qemu can handle it with multidevs=remap if required

Quoted 1 line

[...]

As of QEMU 10.0, multidevs=remap (i.e. remapping inodes from host to guest) is now the default behaviour, since inode collisions were constantly causing issues and confusion among 9p users.

And yeah, cache=loose means 9p client is blind for whatever changes on 9p server side.

/Christian

Tingmao Wang wrote on Sun, Apr 06, 2025 at 09:43:01PM +0100:
[...]

Quoted 4 lines

With the above in mind, I have a proposal for 9pfs to:
1. Reuse inodes even in uncached mode
2. However, reuse them based on qid.path AND the actual pathname, by doing the appropriate testing in v9fs_test(_new)?_inode(_dotl)?

I think that's fine for cache=none, but it breaks hardlinks on cache=loose so I think this ought to only be done without cache (I haven't really played with the cache flag bits, not check pathname if any of loose, writeback or metadata are set?)

Quoted 4 lines

The main problem here is how to store the pathname in a sensible way and tie it to the inode. For now I opted with an array of names acquired with take_dentry_name_snapshot, which reuses the same memory as the dcache to store the actual strings

Quoted 4 lines

, but doesn't tie the lifetime of the dentry with
the inode (I thought about holding a reference to the dentry in the v9fs_inode, but it seemed like a wrong approach and would cause dentries to not be evicted/released).

That's pretty hard to get right and I wish we had more robust testing there... But I guess that's appropriate enough.

I know Atos has done an implementation that keeps the full path somewhere to re-open fids in case of server reconnections, but that code has never been submitted upstream that I can see so I can't check how they used to store the path :/ Ohwell.

Quoted 3 lines

Storing one pathname per inode also means we don't reuse the same inode for hardlinks -- maybe this can be fixed as well in a future version, if this approach sounds good?

Ah, you pointed it out yourself. I don't see how we could fix that, as we have no way other than the qid to identify hard links; so this really ought to depend on cache level if you want to support landlock/*notify in cache=none.

Frankly the *notify use-case is difficult to support properly, as files can change from under us (e.g. modifying the file directly on the host in qemu case, or just multiple mounts of the same directory), so it can't be relied on in the general case anyway -- 9p doesn't have anything like NFSv4 leases to get notified when other clients write a file we "own", so whatever we do will always be limited... But I guess it can make sense for limited monitoring e.g. rebuilding something on change and things like that?

[...]

Quoted 37 lines · Al Viro

Quoted 35 lines · Tingmao Wang

Quoted 20 lines

(quoted diff) +20 -0

+bool ino_path_compare(struct v9fs_ino_path *ino_path,
+			     struct dentry *dentry)
+{
+	struct dentry *curr = dentry;
+	struct qstr *curr_name;
+	struct name_snapshot *compare;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+	for (i = ino_path->nr_components - 1; i >= 0; i--) {
+		if (curr->d_parent == curr) {
+			/* We're supposed to have more components to walk */
+			rcu_read_unlock();
+			return false;
+		}
+		curr_name = &curr->d_name;
+		compare = &ino_path->names[i];
+		/*

Review

Quoted 10 lines

(quoted diff) +10 -0

+		 * We can't use hash_len because it is salted with the parent
+		 * dentry pointer.  We could make this faster by pre-computing our
+		 * own hashlen for compare and ino_path outside, probably.
+		 */
+		if (curr_name->len != compare->name.len) {
+			rcu_read_unlock();
+			return false;
+		}
+		if (strncmp(curr_name->name, compare->name.name,
+			    curr_name->len) != 0) {

... without any kind of protection for curr_name. Incidentally, what about rename()? Not a cross-directory one, just one that changes the name of a subdirectory within the same parent?

Quoted 6 lines

As far as I can tell, in v9fs_vfs_rename, v9ses->rename_sem is taken for both same-parent and different parent renames, so I think we're safe here (and hopefully for any v9fs dentries, nobody should be causing d_name to change except for ourselves when we call d_move in v9fs_vfs_rename? If yes then because we also take v9ses->rename_sem, in theory we should be fine here...?)

A lockdep_assert_held() or similar and a comment would make this clear.

[...]

Quoted 11 lines

/*
* Must hold rename_sem due to traversing parents
*/
bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)

{
	struct dentry *curr = dentry;
	struct name_snapshot *compare;
	ssize_t i;

	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);

Quoted 8 lines

	rcu_read_lock();
	for (i = ino_path->nr_components - 1; i >= 0; i--) {
		if (curr->d_parent == curr) {
			/* We're supposed to have more components to walk */
			rcu_read_unlock();
			return false;
		}
		compare = &ino_path->names[i];

Quoted 8 lines

		if (!d_same_name(curr, curr->d_parent, &compare->name)) {
			rcu_read_unlock();
			return false;
		}
		curr = curr->d_parent;
	}
	rcu_read_unlock();
	if (curr != curr->d_parent) {

Quoted 5 lines

		/* dentry is deeper than ino_path */
		return false;
	}
	return true;
}

I like this new version.

My understanding of cache=loose was that it only assumes that the server side can't change the fs under the client, but otherwise things like inodes should work identically, even though currently it works differently due to (only) cached mode re-using inodes - was this deliberate?

Quoted 4 lines

I think that's fine for cache=none, but it breaks hardlinks on cache=loose so I think this ought to only be done without cache (I haven't really played with the cache flag bits, not check pathname if any of loose, writeback or metadata are set?)

I think currently 9pfs reuse inodes if cache is either loose or metadata (but not writeback), given:

	else if (v9ses->cache & (CACHE_META|CACHE_LOOSE))
		inode = v9fs_get_inode_from_fid(v9ses, fid, dir->i_sb);
	else
		inode = v9fs_get_new_inode_from_fid(v9ses, fid, dir->i_sb);

in v9fs_vfs_lookup, so maybe we keep this pattern and not check pathname if (loose|metadata) is set (but not writeback)?

Quoted 4 lines

Quoted 4 lines

The main problem here is how to store the pathname in a sensible way and tie it to the inode. For now I opted with an array of names acquired with take_dentry_name_snapshot, which reuses the same memory as the dcache to store the actual strings

(Self correction: technically, the space is only shared if they are long enough to not be inlined, which is DNAME_INLINE_LEN = 40 for 64bit or 20 for 32bit, so in most cases the names would probably be copied. Maybe it would be more compact in terms of memory usage to just store the path as a string, with '/' separating components? But then the code would be more complex and we can't easily use d_same_name anymore, so maybe it's not worth doing, unless this will prove useful for other purposes, like the re-opening of fid mentioned below?)

Quoted 8 lines

Frankly the *notify use-case is difficult to support properly, as files can change from under us (e.g. modifying the file directly on the host in qemu case, or just multiple mounts of the same directory), so it can't be relied on in the general case anyway -- 9p doesn't have anything like NFSv4 leases to get notified when other clients write a file we "own", so whatever we do will always be limited... But I guess it can make sense for limited monitoring e.g. rebuilding something on change and things like that?

One of the first use case I can think of here is IDE/code editors reloading state (e.g. the file tree) on change, which I think didn't work for 9pfs folders opened with vscode if I remembered correctly (but I haven't tested this recently). Even though we can't monitor for remote changes, having this work for local changes would still be nice.

Thanks for the review :) I will try to send a v2 in the coming weeks with the two changes you suggested and the changes to cached mode as suggested by Dominique (plus rename handling). (will also try to figure out how to test with xfstests)

Quoted 45 lines · Mickaël Salaün

[...]

Quoted 43 lines · Al Viro

Quoted 27 lines · Tingmao Wang

Quoted 26 lines

(quoted diff) +26 -0

+bool ino_path_compare(struct v9fs_ino_path *ino_path,
+			     struct dentry *dentry)
+{
+	struct dentry *curr = dentry;
+	struct qstr *curr_name;
+	struct name_snapshot *compare;
+	ssize_t i;
+
+	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
+
+	rcu_read_lock();
+	for (i = ino_path->nr_components - 1; i >= 0; i--) {
+		if (curr->d_parent == curr) {
+			/* We're supposed to have more components to walk */
+			rcu_read_unlock();
+			return false;
+		}
+		curr_name = &curr->d_name;
+		compare = &ino_path->names[i];
+		/*
+		 * We can't use hash_len because it is salted with the parent
+		 * dentry pointer.  We could make this faster by pre-computing our
+		 * own hashlen for compare and ino_path outside, probably.
+		 */
+		if (curr_name->len != compare->name.len) {
+			rcu_read_unlock();

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+			return false;
+		}
+		if (strncmp(curr_name->name, compare->name.name,
+			    curr_name->len) != 0) {

... without any kind of protection for curr_name. Incidentally, what about rename()? Not a cross-directory one, just one that changes the name of a subdirectory within the same parent?

As far as I can tell, in v9fs_vfs_rename, v9ses->rename_sem is taken for both same-parent and different parent renames, so I think we're safe here (and hopefully for any v9fs dentries, nobody should be causing d_name to change except for ourselves when we call d_move in v9fs_vfs_rename? If yes then because we also take v9ses->rename_sem, in theory we should be fine here...?)

Quoted 1 line

A lockdep_assert_held() or similar and a comment would make this clear.

I can add a comment, but there is already a lockdep_assert_held_read of the v9fs rename sem at the top of this function.

Quoted 13 lines

[...]

Quoted 11 lines

/*
* Must hold rename_sem due to traversing parents
*/
bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)

{
	struct dentry *curr = dentry;
	struct name_snapshot *compare;
	ssize_t i;

	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);

Quoted 16 lines

Quoted 8 lines

	rcu_read_lock();
	for (i = ino_path->nr_components - 1; i >= 0; i--) {
		if (curr->d_parent == curr) {
			/* We're supposed to have more components to walk */
			rcu_read_unlock();
			return false;
		}
		compare = &ino_path->names[i];

Quoted 8 lines

		if (!d_same_name(curr, curr->d_parent, &compare->name)) {
			rcu_read_unlock();
			return false;
		}
		curr = curr->d_parent;
	}
	rcu_read_unlock();
	if (curr != curr->d_parent) {

Looking at this again I think this check probably needs to be done inside RCU, will fix as below:

Quoted 5 lines

Quoted 5 lines

		/* dentry is deeper than ino_path */
		return false;
	}
	return true;
}

fs/9p/ino_path.c +3 -1

index 0000b4964df0..7264003cb087 100644
@@ -77,13 +77,15 @@ void free_ino_path(struct v9fs_ino_path *path)
 }
 
 /*
- * Must hold rename_sem due to traversing parents
+ * Must hold rename_sem due to traversing parents.  Returns whether
+ * ino_path matches with the path of a v9fs dentry.
  */
 bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
 {
 	struct dentry *curr = dentry;
 	struct name_snapshot *compare;
 	ssize_t i;
+	bool ret;
 
 	lockdep_assert_held_read(&v9fs_dentry2v9ses(dentry)->rename_sem);
 

@@ -101,10 +103,8 @@ bool ino_path_compare(struct v9fs_ino_path *ino_path, struct dentry *dentry)
 		}
 		curr = curr->d_parent;
 	}

Quoted 4 lines

I like this new version.