[PATCH] lockd: fix NULL pointer dereference in nlmclnt_locks_release_private

[PATCH] lockd: fix NULL pointer dereference in nlmclnt_locks_release_private

[Ran Hongyun]

nlmclnt_locks_init_private() unconditionally sets fl->fl_ops even when
nlmclnt_find_lockowner() returns NULL due to allocation failure. When
locks_release_private() later sees a non-NULL fl_ops, it calls
fl_release_private, which dereferences fl->fl_u.nfs_fl.owner.

nlmclnt_proc()
	nlmclnt_locks_init_private <----set fl->fl_ops unconditionally
		if (!fl->fl_u.nfs_fl.owner) <----forget to clear fl->fl_ops

locks_release_private()
	if (fl->fl_ops) <----fl->fl_ops is not NULL but owner is NULL
		fl->fl_ops->fl_release_private()
			nlmclnt_locks_release_private() <----NULL ptr dereference

Fix this by clearing fl_ops when nlmclnt_find_lockowner() fails in
nlmclnt_proc(), so that locks_release_private() skips the
fl_release_private call path.

Fixes: bf8848918d75 ("lockd: handle lockowner allocation failure in nlmclnt_proc()")
Signed-off-by: Ran Hongyun <ranhongyun1@huawei.com>
---
 fs/lockd/clntproc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index f06faf577cea..4b54481fecd5 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -176,6 +176,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *dat
 	if (!fl->fl_u.nfs_fl.owner) {
 		/* lockowner allocation has failed */
 		nlmclnt_release_call(call);
+		fl->fl_ops = NULL;
 		return -ENOMEM;
 	}
 	/* Set up the argument struct */
-- 
2.52.0

Re: [PATCH] lockd: fix NULL pointer dereference in nlmclnt_locks_release_private

[Jeff Layton]

On Mon, 2026-08-17 at 14:08 +0800, Ran Hongyun wrote:
> nlmclnt_locks_init_private() unconditionally sets fl->fl_ops even when
> nlmclnt_find_lockowner() returns NULL due to allocation failure. When
> locks_release_private() later sees a non-NULL fl_ops, it calls
> fl_release_private, which dereferences fl->fl_u.nfs_fl.owner.
> 
> nlmclnt_proc()
> 	nlmclnt_locks_init_private <----set fl->fl_ops unconditionally
> 		if (!fl->fl_u.nfs_fl.owner) <----forget to clear fl->fl_ops
> 
> locks_release_private()
> 	if (fl->fl_ops) <----fl->fl_ops is not NULL but owner is NULL
> 		fl->fl_ops->fl_release_private()
> 			nlmclnt_locks_release_private() <----NULL ptr dereference
> 
> Fix this by clearing fl_ops when nlmclnt_find_lockowner() fails in
> nlmclnt_proc(), so that locks_release_private() skips the
> fl_release_private call path.
> 
> Fixes: bf8848918d75 ("lockd: handle lockowner allocation failure in nlmclnt_proc()")
> Signed-off-by: Ran Hongyun <ranhongyun1@huawei.com>
> ---
>  fs/lockd/clntproc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
> index f06faf577cea..4b54481fecd5 100644
> --- a/fs/lockd/clntproc.c
> +++ b/fs/lockd/clntproc.c
> @@ -176,6 +176,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *dat
>  	if (!fl->fl_u.nfs_fl.owner) {
>  		/* lockowner allocation has failed */
>  		nlmclnt_release_call(call);
> +		fl->fl_ops = NULL;
>  		return -ENOMEM;
>  	}
>  	/* Set up the argument struct */

The fix looks correct, but I don't like the way that the initialization
and cleanup is spread across multiple functions. I think it would be
better to just get rid of nlmclnt_locks_init_private() altogether and
open code what it does in the single caller (nlmclnt_proc()). Then you
could just not initialize fl_ops if nlmclnt_find_lockowner() fails.

There's also another preexisting bug here. If the lockowner allocation
fails, this function also leaks the references taken in
nlmclnt_alloc_call().

Want to spin up a v2 that fixes all of this?
-- 
Jeff Layton <jlayton@kernel.org>