[PATCH 0/2] lockd: fix two bugs on nlmclnt_find_lockowner() failure path

[PATCH 0/2] lockd: fix two bugs on nlmclnt_find_lockowner() failure path

[Ran Hongyun]

Both bugs are triggered when nlmclnt_find_lockowner() returns NULL
due to allocation failure in nlmclnt_proc():

Patch 1 fixes a NULL pointer dereference: nlmclnt_locks_init_private()
unconditionally sets fl->fl_ops before checking whether owner is NULL,
so locks_release_private() later calls fl_release_private which
dereferences the NULL owner. Fix by inlining the function so that
fl_ops is only set after the owner is valid.

Patch 2 fixes a reference leak: call->a_callback_data has not been
assigned when nlmclnt_release_call() is invoked on the error path,
so nlmclnt_ops->nlmclnt_release_call(NULL) skips cleanup and the
references taken by nlmclnt_alloc_call() are never freed. Fix by
moving the assignment before the lockowner check.

Ran Hongyun (2):
  lockd: fix NULL pointer dereference in nlmclnt_locks_release_private
  lockd: fix reference leak on lockowner allocation failure in
    nlmclnt_proc

 fs/lockd/clntproc.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

-- 
2.52.0

[PATCH 1/2] 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 it by inlining nlmclnt_locks_init_private() into its single
caller nlmclnt_proc(), so that fl_ops is only set after
nlmclnt_find_lockowner() succeeds

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

diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index f06faf577cea..dc0519ac0172 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -31,11 +31,11 @@ static int	nlmclnt_test(struct nlm_rqst *, struct file_lock *);
 static int	nlmclnt_lock(struct nlm_rqst *, struct file_lock *);
 static int	nlmclnt_unlock(struct nlm_rqst *, struct file_lock *);
 static int	nlm_stat_to_errno(__be32 stat);
-static void	nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *host);
 static int	nlmclnt_cancel(struct nlm_host *, int , struct file_lock *);
 
 static const struct rpc_call_ops nlmclnt_unlock_ops;
 static const struct rpc_call_ops nlmclnt_cancel_ops;
+static const struct file_lock_operations nlmclnt_lock_ops;
 
 /*
  * Cookie counter for NLM requests
@@ -172,12 +172,16 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *dat
 	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
 		nlmclnt_ops->nlmclnt_alloc_call(data);
 
-	nlmclnt_locks_init_private(fl, host);
+	fl->fl_u.nfs_fl.state = 0;
+	fl->fl_u.nfs_fl.owner = nlmclnt_find_lockowner(host, fl->c.flc_owner);
 	if (!fl->fl_u.nfs_fl.owner) {
 		/* lockowner allocation has failed */
 		nlmclnt_release_call(call);
 		return -ENOMEM;
 	}
+	INIT_LIST_HEAD(&fl->fl_u.nfs_fl.list);
+	fl->fl_ops = &nlmclnt_lock_ops;
+
 	/* Set up the argument struct */
 	nlmclnt_setlockargs(call, fl);
 	call->a_callback_data = data;
@@ -484,15 +488,6 @@ static const struct file_lock_operations nlmclnt_lock_ops = {
 	.fl_release_private = nlmclnt_locks_release_private,
 };
 
-static void nlmclnt_locks_init_private(struct file_lock *fl, struct nlm_host *host)
-{
-	fl->fl_u.nfs_fl.state = 0;
-	fl->fl_u.nfs_fl.owner = nlmclnt_find_lockowner(host,
-						       fl->c.flc_owner);
-	INIT_LIST_HEAD(&fl->fl_u.nfs_fl.list);
-	fl->fl_ops = &nlmclnt_lock_ops;
-}
-
 static int do_vfs_lock(struct file_lock *fl)
 {
 	return locks_lock_file_wait(fl->c.flc_file, fl);
-- 
2.52.0

[PATCH 2/2] lockd: fix reference leak on lockowner allocation failure in nlmclnt_proc

[Ran Hongyun]

If nlmclnt_find_lockowner() fails, nlmclnt_release_call() is invoked
before call->a_callback_data is assigned, so it passes NULL to
nlmclnt_ops->nlmclnt_release_call() and the references taken by
nlmclnt_alloc_call() are leaked.

Fix it by moving the a_callback_data assignment before the lockowner
check, so nlmclnt_release_call() can free the references  properly.

Fixes: b1ece737f44f ("lockd: Introduce nlmclnt_operations")
Signed-off-by: Ran Hongyun <ranhongyun1@huawei.com>
---
 fs/lockd/clntproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/lockd/clntproc.c b/fs/lockd/clntproc.c
index dc0519ac0172..6b69a52788b7 100644
--- a/fs/lockd/clntproc.c
+++ b/fs/lockd/clntproc.c
@@ -171,6 +171,7 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *dat
 
 	if (nlmclnt_ops && nlmclnt_ops->nlmclnt_alloc_call)
 		nlmclnt_ops->nlmclnt_alloc_call(data);
+	call->a_callback_data = data;
 
 	fl->fl_u.nfs_fl.state = 0;
 	fl->fl_u.nfs_fl.owner = nlmclnt_find_lockowner(host, fl->c.flc_owner);
@@ -184,7 +185,6 @@ int nlmclnt_proc(struct nlm_host *host, int cmd, struct file_lock *fl, void *dat
 
 	/* Set up the argument struct */
 	nlmclnt_setlockargs(call, fl);
-	call->a_callback_data = data;
 
 	if (IS_SETLK(cmd) || IS_SETLKW(cmd)) {
 		if (fl->c.flc_type != F_UNLCK) {
-- 
2.52.0

Re: [PATCH 0/2] lockd: fix two bugs on nlmclnt_find_lockowner() failure path

[Jeff Layton]

nit: usually when you send a new version of a patchset you should
declare it as "v2". So "PATCH v2 0/2", etc...

Using the b4 command makes this easy to track.

On Wed, 2026-08-19 at 15:46 +0800, Ran Hongyun wrote:
> Both bugs are triggered when nlmclnt_find_lockowner() returns NULL
> due to allocation failure in nlmclnt_proc():
> 
> Patch 1 fixes a NULL pointer dereference: nlmclnt_locks_init_private()
> unconditionally sets fl->fl_ops before checking whether owner is NULL,
> so locks_release_private() later calls fl_release_private which
> dereferences the NULL owner. Fix by inlining the function so that
> fl_ops is only set after the owner is valid.
> 
> Patch 2 fixes a reference leak: call->a_callback_data has not been
> assigned when nlmclnt_release_call() is invoked on the error path,
> so nlmclnt_ops->nlmclnt_release_call(NULL) skips cleanup and the
> references taken by nlmclnt_alloc_call() are never freed. Fix by
> moving the assignment before the lockowner check.
> 
> Ran Hongyun (2):
>   lockd: fix NULL pointer dereference in nlmclnt_locks_release_private
>   lockd: fix reference leak on lockowner allocation failure in
>     nlmclnt_proc
> 
>  fs/lockd/clntproc.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)

New version looks good. Note it looks like Shuangpeng Bai sent a less
complete patch for this problem back in July that I just noticed. Your
version seems more correct though.

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

Re: [PATCH 0/2] lockd: fix two bugs on nlmclnt_find_lockowner() failure path

[Jeff Layton]

On Wed, 2026-08-19 at 15:46 +0800, Ran Hongyun wrote:
> Both bugs are triggered when nlmclnt_find_lockowner() returns NULL
> due to allocation failure in nlmclnt_proc():
> 
> Patch 1 fixes a NULL pointer dereference: nlmclnt_locks_init_private()
> unconditionally sets fl->fl_ops before checking whether owner is NULL,
> so locks_release_private() later calls fl_release_private which
> dereferences the NULL owner. Fix by inlining the function so that
> fl_ops is only set after the owner is valid.
> 
> Patch 2 fixes a reference leak: call->a_callback_data has not been
> assigned when nlmclnt_release_call() is invoked on the error path,
> so nlmclnt_ops->nlmclnt_release_call(NULL) skips cleanup and the
> references taken by nlmclnt_alloc_call() are never freed. Fix by
> moving the assignment before the lockowner check.
> 
> Ran Hongyun (2):
>   lockd: fix NULL pointer dereference in nlmclnt_locks_release_private
>   lockd: fix reference leak on lockowner allocation failure in
>     nlmclnt_proc
> 
>  fs/lockd/clntproc.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)

To be clear, these are client-side patches. Anna/Trond, can you pick
these up?

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>