On Wed, Aug 13, 2025 at 12:57:49AM +0100, Tingmao Wang wrote:

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)

On 8/8/25 09:32, Mickaël Salaün wrote:

quoted

[...]

quoted

On 7/5/25 01:25, Al Viro wrote:

quoted

On Sun, Apr 06, 2025 at 09:43:02PM +0100, Tingmao Wang wrote:

quoted

+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();
+			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...?)

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.

I wrote this comment before reading your new version beneath, which
already have this lockdep, so no need to change anything. :)

Looks good

quoted

I like this new version.