Re: [PATCH] lockd: fix NULL pointer dereference in nlmclnt_locks_release_private
Jeff Layton <[email protected]>
| Newsgroups | org.kernel.vger.linux-nfs,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
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 <[email protected]>
> ---
> 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 <[email protected]>