On Sat, 2023-12-30 at 08:23 +0200, Amir Goldstein wrote: > On Sat, Dec 30, 2023 at 1:50 AM Trond Myklebust > <trondmy@xxxxxxxxxxxxxxx> wrote: > > > > On Fri, 2023-12-29 at 18:29 -0500, Chuck Lever wrote: > > > On Fri, Dec 29, 2023 at 07:44:20PM +0200, Amir Goldstein wrote: > > > > On Fri, Dec 29, 2023 at 4:35 PM Chuck Lever > > > > <chuck.lever@xxxxxxxxxx> wrote: > > > > > > > > > > On Fri, Dec 29, 2023 at 07:46:54AM +0200, Amir Goldstein > > > > > wrote: > > > > > > [CC: fsdevel, viro] > > > > > > > > > > Thanks for picking this up, Amir, and for copying > > > > > viro/fsdevel. I > > > > > was planning to repost this next week when more folks are > > > > > back, > > > > > but > > > > > this works too. > > > > > > > > > > Trond, if you'd like, I can handle review changes if you > > > > > don't > > > > > have > > > > > time to follow up. > > > > > > > > > > > > > > > > On Thu, Dec 28, 2023 at 10:22 PM <trondmy@xxxxxxxxxx> > > > > > > wrote: > > > > > > > > > > > > > > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> > > > > > > > > > > > > > > The fallback implementation for the get_name export > > > > > > > operation > > > > > > > uses > > > > > > > readdir() to try to match the inode number to a filename. > > > > > > > That filename > > > > > > > is then used together with lookup_one() to produce a > > > > > > > dentry. > > > > > > > A problem arises when we match the '.' or '..' entries, > > > > > > > since > > > > > > > that > > > > > > > causes lookup_one() to fail. This has sometimes been seen > > > > > > > to > > > > > > > occur for > > > > > > > filesystems that violate POSIX requirements around > > > > > > > uniqueness > > > > > > > of inode > > > > > > > numbers, something that is common for snapshot > > > > > > > directories. > > > > > > > > > > > > Ouch. Nasty. > > > > > > > > > > > > Looks to me like the root cause is "filesystems that > > > > > > violate > > > > > > POSIX > > > > > > requirements around uniqueness of inode numbers". > > > > > > This violation can cause any of the parent's children to > > > > > > wrongly match > > > > > > get_name() not only '.' and '..' and fail the d_inode > > > > > > sanity > > > > > > check after > > > > > > lookup_one(). > > > > > > > > > > > > I understand why this would be common with parent of > > > > > > snapshot > > > > > > dir, > > > > > > but the only fs that support snapshots that I know of > > > > > > (btrfs, > > > > > > bcachefs) > > > > > > do implement ->get_name(), so which filesystem did you > > > > > > encounter > > > > > > this behavior with? can it be fixed by implementing a > > > > > > snapshot > > > > > > aware ->get_name()? > > > > > > > > > > > > > This patch just ensures that we skip '.' and '..' rather > > > > > > > than > > > > > > > allowing a > > > > > > > match. > > > > > > > > > > > > I agree that skipping '.' and '..' makes sense, but... > > > > > > > > > > Does skipping '.' and '..' make sense for file systems that > > > > > do > > > > > > > > It makes sense because if the child's name in its parent would > > > > have been "." or ".." it would have been its own parent or its > > > > own > > > > grandparent (ELOOP situation). > > > > IOW, we can safely skip "." and "..", regardless of anything > > > > else. > > > > > > This new comment: > > > > > > + /* Ignore the '.' and '..' entries */ > > > > > > then seems inadequate to explain why dot and dot-dot are now > > > never > > > matched. Perhaps the function's documenting comment could expand > > > on > > > this a little. I'll give it some thought. > > > > The point of this code is to attempt to create a valid path that > > connects the inode found by the filehandle to the export point. The > > readdir() must determine a valid name for a dentry that is a > > component > > of that path, which is why '.' and '..' can never be acceptable. > > > > This is why I think we should keep the 'Fixes:' line. The commit it > > points to explains quite concisely why this patch is needed. > > > > By all means, mention this commit, just not with a fixed tag please. > IIUC, commit 21d8a15ac333 did not introduce a regression that this > patch fixes. Right? > So why insist on abusing Fixes: tag instead of a mention? I don't see it as being that straightforward. Prior to commit 21d8a15ac333, the call to lookup_one_len() could return a dentry (albeit one with an invalid name) depending on whether or not the filesystem lookup succeeds. Note that knfsd does support a lookup of "." and "..", as do several other NFS servers. With commit 21d8a15ac333 applied, however, lookup_one_len() automatically returns an EACCES error. So while I agree that there are good reasons for introducing commit 21d8a15ac333, it does change the behaviour in this code path. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@xxxxxxxxxxxxxxx
