[PATCH v3] fs: make sure to fail try_to_unlazy() and try_to_unlazy() for LOOKUP_CACHED

[PATCH v3] fs: make sure to fail try_to_unlazy() and try_to_unlazy() for LOOKUP_CACHED

[Mateusz Guzik]

Otherwise the slowpath can be taken by the caller, defeating the flag.

This regressed after calls to legitimize_links() started being
conditionally elided and stems from the routine always failing
after seeing the flag, regardless if there were any links.

In order to address both the bug and the weird semantics make it illegal
to call legitimize_links() with LOOKUP_CACHED and handle the problem at
the two callsites.

Fixes: 7c179096e77eca21 ("fs: add predicts based on nd->depth")
Reported-by: Chris Mason <clm@meta.com>
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---

v3:
- keep nd->depth = 0 out of drop_links to further simplify the patch

The thread for v2 got derailed with a discussion unrelated to the fix
itself.

Whatever future cleanups or lack thereof can be discussed after the
regression is fixed so I'm keep anything of the sort out of this patch.

The easiest way out I can think of would merely remove the ->depth
checks before calling legitimize_mnt, but it would be a bummer to
reintroduce the func call.

The second easiest way out literally copy-pastes the current body into
the 2 callers, which I implemented in this patch.

 fs/namei.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index bf0f66f0e9b9..f7a8b5b000c2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -830,11 +830,9 @@ static inline bool legitimize_path(struct nameidata *nd,
 static bool legitimize_links(struct nameidata *nd)
 {
 	int i;
-	if (unlikely(nd->flags & LOOKUP_CACHED)) {
-		drop_links(nd);
-		nd->depth = 0;
-		return false;
-	}
+
+	VFS_BUG_ON(nd->flags & LOOKUP_CACHED);
+
 	for (i = 0; i < nd->depth; i++) {
 		struct saved *last = nd->stack + i;
 		if (unlikely(!legitimize_path(nd, &last->link, last->seq))) {
@@ -883,6 +881,11 @@ static bool try_to_unlazy(struct nameidata *nd)
 
 	BUG_ON(!(nd->flags & LOOKUP_RCU));
 
+	if (unlikely(nd->flags & LOOKUP_CACHED)) {
+		drop_links(nd);
+		nd->depth = 0;
+		goto out1;
+	}
 	if (unlikely(nd->depth && !legitimize_links(nd)))
 		goto out1;
 	if (unlikely(!legitimize_path(nd, &nd->path, nd->seq)))
@@ -918,6 +921,11 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry)
 	int res;
 	BUG_ON(!(nd->flags & LOOKUP_RCU));
 
+	if (unlikely(nd->flags & LOOKUP_CACHED)) {
+		drop_links(nd);
+		nd->depth = 0;
+		goto out2;
+	}
 	if (unlikely(nd->depth && !legitimize_links(nd)))
 		goto out2;
 	res = __legitimize_mnt(nd->path.mnt, nd->m_seq);
-- 
2.48.1

Re: [PATCH v3] fs: make sure to fail try_to_unlazy() and try_to_unlazy() for LOOKUP_CACHED

[Al Viro]

On Sat, Dec 20, 2025 at 06:40:22AM +0100, Mateusz Guzik wrote:
> Otherwise the slowpath can be taken by the caller, defeating the flag.
> 
> This regressed after calls to legitimize_links() started being
> conditionally elided and stems from the routine always failing
> after seeing the flag, regardless if there were any links.
> 
> In order to address both the bug and the weird semantics make it illegal
> to call legitimize_links() with LOOKUP_CACHED and handle the problem at
> the two callsites.

I still don't get what's weird about the semantics involved, but
the only question I've got is the location of this VFS_BUG_ON().
A way to ensure that we don't forget to check LOOKUP_CACHED early,
in both (seriously similar) callers?

Anyway, it does fix the regression.

Re: [PATCH v3] fs: make sure to fail try_to_unlazy() and try_to_unlazy() for LOOKUP_CACHED

[Mateusz Guzik]

On Sat, Dec 20, 2025 at 9:57 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Sat, Dec 20, 2025 at 06:40:22AM +0100, Mateusz Guzik wrote:
> > Otherwise the slowpath can be taken by the caller, defeating the flag.
> >
> > This regressed after calls to legitimize_links() started being
> > conditionally elided and stems from the routine always failing
> > after seeing the flag, regardless if there were any links.
> >
> > In order to address both the bug and the weird semantics make it illegal
> > to call legitimize_links() with LOOKUP_CACHED and handle the problem at
> > the two callsites.
>
> I still don't get what's weird about the semantics involved, but
> the only question I've got is the location of this VFS_BUG_ON().
> A way to ensure that we don't forget to check LOOKUP_CACHED early,
> in both (seriously similar) callers?
>

For my taste routines should document their assumptions with asserts.
Note this is dependent on CONFIG_DEBUG_VFS, so there is no code
emitted for production kernels.

As for whether the current behavior is weird, I'm from $elsewhere and
in that land this would be odd. If this is fine on linux, then i fit
even less than i thought

Re: [PATCH v3] fs: make sure to fail try_to_unlazy() and try_to_unlazy() for LOOKUP_CACHED

[Christian Brauner]

On Sat, 20 Dec 2025 06:40:22 +0100, Mateusz Guzik wrote:
> Otherwise the slowpath can be taken by the caller, defeating the flag.
> 
> This regressed after calls to legitimize_links() started being
> conditionally elided and stems from the routine always failing
> after seeing the flag, regardless if there were any links.
> 
> In order to address both the bug and the weird semantics make it illegal
> to call legitimize_links() with LOOKUP_CACHED and handle the problem at
> the two callsites.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: make sure to fail try_to_unlazy() and try_to_unlazy() for LOOKUP_CACHED
      https://git.kernel.org/vfs/vfs/c/46af9ae1305f