[PATCH] nfsd: Fix id-to-name cache entry leak in idtoname_parse()

[PATCH] nfsd: Fix id-to-name cache entry leak in idtoname_parse()

[Wentao Liang]

idtoname_lookup() returns the id-to-name cache entry with a new
reference. The name is parsed between the lookup and the update call,
and when the parsed name is malformed (negative length or longer than
IDMAP_NAMESZ), idtoname_parse() jumps to the error label without
dropping the reference returned by idtoname_lookup(), leaking the cache
entry.

Release the reference with cache_put() before taking the error exit.

Fixes: f9ecc921b5b5 ("[PATCH] knfsd: Use new cache code for name/id lookup caches")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 fs/nfsd/nfs4idmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index ba06d3d3e6dd..e0f1bebdf660 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -252,8 +252,10 @@ idtoname_parse(struct cache_detail *cd, char *buf, int buflen)
 	/* Name */
 	error = -EINVAL;
 	len = qword_get(&buf, buf1, PAGE_SIZE);
-	if (len < 0 || len >= IDMAP_NAMESZ)
+	if (len < 0 || len >= IDMAP_NAMESZ) {
+		cache_put(&res->h, cd);
 		goto out;
+	}
 	if (len == 0)
 		set_bit(CACHE_NEGATIVE, &ent.h.flags);
 	else
-- 
2.34.1

Re: [PATCH] nfsd: Fix id-to-name cache entry leak in idtoname_parse()

[Jeff Layton]

On Thu, 2026-09-17 at 16:31 +0000, Wentao Liang wrote:
> idtoname_lookup() returns the id-to-name cache entry with a new
> reference. The name is parsed between the lookup and the update call,
> and when the parsed name is malformed (negative length or longer than
> IDMAP_NAMESZ), idtoname_parse() jumps to the error label without
> dropping the reference returned by idtoname_lookup(), leaking the cache
> entry.
> 
> Release the reference with cache_put() before taking the error exit.
> 
> Fixes: f9ecc921b5b5 ("[PATCH] knfsd: Use new cache code for name/id lookup caches")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  fs/nfsd/nfs4idmap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
> index ba06d3d3e6dd..e0f1bebdf660 100644
> --- a/fs/nfsd/nfs4idmap.c
> +++ b/fs/nfsd/nfs4idmap.c
> @@ -252,8 +252,10 @@ idtoname_parse(struct cache_detail *cd, char *buf, int buflen)
>  	/* Name */
>  	error = -EINVAL;
>  	len = qword_get(&buf, buf1, PAGE_SIZE);
> -	if (len < 0 || len >= IDMAP_NAMESZ)
> +	if (len < 0 || len >= IDMAP_NAMESZ) {
> +		cache_put(&res->h, cd);
>  		goto out;
> +	}
>  	if (len == 0)
>  		set_bit(CACHE_NEGATIVE, &ent.h.flags);
>  	else

Looks like we have the same bug later if idtoname_update() fails too? I
think it would be better to restructure the code near the end of the
function for better error handling. Something like this maybe?

diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index 4e5297593963..9fba65bb6ba4 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -255,7 +255,7 @@ idtoname_parse(struct cache_detail *cd, char *buf,
int buflen)
        error = -EINVAL;
        len = qword_get(&buf, buf1, PAGE_SIZE);
        if (len < 0 || len >= IDMAP_NAMESZ)
-               goto out;
+               goto out_put;
        if (len == 0)
                set_bit(CACHE_NEGATIVE, &ent.h.flags);
        else
@@ -263,10 +263,11 @@ idtoname_parse(struct cache_detail *cd, char
*buf, int buflen)
        error = -ENOMEM;
        res = idtoname_update(cd, &ent, res);
        if (res == NULL)
-               goto out;
+               goto out_put;
 
-       cache_put(&res->h, cd);
        error = 0;
+out_put:
+       cache_put(&res->h, cd);
 out:
        kfree(buf1);
        return error;

Re: [PATCH] nfsd: Fix id-to-name cache entry leak in idtoname_parse()

[Chuck Lever]

On Thu, 2026-09-17 at 14:10 -0400, Jeff Layton wrote:
> Looks like we have the same bug later if idtoname_update() fails too?

I don't think so: when sunrpc_cache_update() fails to allocate the
replacement entry, it drops the reference on the old entry itself:

	tmp = detail->alloc();
	if (!tmp) {
		cache_put(old, detail);
		return NULL;
	}

So that exit is already balanced.

> +out_put:
> +       cache_put(&res->h, cd);
>  out:

On the update failure path, res is NULL here, and cache_put()
dereferences it. A shared label would need the "if (res)" guard that
expkey_parse() uses.

A shared put label might be a reasonable clean-up, but for a fix
headed to stable, the two-line version Wentao posted looks OK
to me.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

Re: [PATCH] nfsd: Fix id-to-name cache entry leak in idtoname_parse()

[Jeff Layton]

On Thu, 2026-09-17 at 14:53 -0400, Chuck Lever wrote:
> On Thu, 2026-09-17 at 14:10 -0400, Jeff Layton wrote:
> > Looks like we have the same bug later if idtoname_update() fails too?
> 
> I don't think so: when sunrpc_cache_update() fails to allocate the
> replacement entry, it drops the reference on the old entry itself:
> 
> 	tmp = detail->alloc();
> 	if (!tmp) {
> 		cache_put(old, detail);
> 		return NULL;
> 	}
> 
> So that exit is already balanced.
> 
> > +out_put:
> > +       cache_put(&res->h, cd);
> >  out:
> 
> On the update failure path, res is NULL here, and cache_put()
> dereferences it. A shared label would need the "if (res)" guard that
> expkey_parse() uses.
> 
> A shared put label might be a reasonable clean-up, but for a fix
> headed to stable, the two-line version Wentao posted looks OK
> to me.
> 

I stand corrected then re: the other bug. It might be nice to clean
that up and make the refcounting more evident, but I agree that should
wait for a later patch.

You can add this:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: Fix id-to-name cache entry leak in idtoname_parse()

[Chuck Lever]

On Thu, 17 Sep 2026 16:31:54 +0000, Wentao Liang wrote:
> idtoname_lookup() returns the id-to-name cache entry with a new
> reference. The name is parsed between the lookup and the update call,
> and when the parsed name is malformed (negative length or longer than
> IDMAP_NAMESZ), idtoname_parse() jumps to the error label without
> dropping the reference returned by idtoname_lookup(), leaking the cache
> entry.
> 
> [...]

Applied to nfsd-testing, thanks!

[1/1] nfsd: Fix id-to-name cache entry leak in idtoname_parse()
      commit: 775ce3d60e25d646447e7ca6c0864b3abf658817

--
Chuck Lever