Re: [PATCH 2/2] nfs: handle failure of get_nfs_open_context

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2025-04-19 at 16:53 +0800, Li Lingfeng wrote:
> During initialization of unlockdata or lockdata structures, if acquiring
> the nfs_open_context fails, the current operation must be aborted to
> ensure the nfs_open_context remains valid after initialization completes.
> This is critical because both lock and unlock release callbacks
> dereference the nfs_open_context - an invalid context could lead to null
> pointer dereference.
> 
> Fixes: faf5f49c2d9c ("NFSv4: Make NFS clean up byte range locks asynchronously")
> Fixes: a5d16a4d090b ("NFSv4: Convert LOCK rpc call into an asynchronous RPC call")
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
>  fs/nfs/nfs4proc.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9f5689c43a50..d76cf0f79f9c 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7075,24 +7075,27 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
>  	struct nfs4_state *state = lsp->ls_state;
>  	struct inode *inode = state->inode;
>  	struct nfs_lock_context *l_ctx;
> +	struct nfs_open_context *open_ctx;
>  
>  	p = kzalloc(sizeof(*p), GFP_KERNEL);
>  	if (p == NULL)
>  		return NULL;
>  	l_ctx = nfs_get_lock_context(ctx);
> -	if (!IS_ERR(l_ctx)) {
> +	if (!IS_ERR(l_ctx))
>  		p->l_ctx = l_ctx;
> -	} else {
> -		kfree(p);
> -		return NULL;
> -	}
> +	else
> +		goto out_free;
> +	/* Ensure we don't close file until we're done freeing locks! */
> +	open_ctx = get_nfs_open_context(ctx);
> 
> 

Sorry for the confusion. Now that I look more closely, I think I was
wrong before.

This can't fail, because the caller holds a reference to ctx, so the
refcount must be non-zero. Instead of this patch, could you add a
comment in there to that effect to make this clear in the future?


> +	if (open_ctx)
> +		p->ctx = open_ctx;
> +	else
> +		goto out_free;

If we did decide to keep the error handling however, this would leak
l_ctx. That reference would also need to be put if open_ctx was NULL
here.

>  	p->arg.fh = NFS_FH(inode);
>  	p->arg.fl = &p->fl;
>  	p->arg.seqid = seqid;
>  	p->res.seqid = seqid;
>  	p->lsp = lsp;
> -	/* Ensure we don't close file until we're done freeing locks! */
> -	p->ctx = get_nfs_open_context(ctx);
>  	locks_init_lock(&p->fl);
>  	locks_copy_lock(&p->fl, fl);
>  	p->server = NFS_SERVER(inode);
> @@ -7100,6 +7103,9 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
>  	nfs4_stateid_copy(&p->arg.stateid, &lsp->ls_stateid);
>  	spin_unlock(&state->state_lock);
>  	return p;
> +out_free:
> +	kfree(p);
> +	return NULL;
>  }
>  
>  static void nfs4_locku_release_calldata(void *data)
> @@ -7327,6 +7333,8 @@ static struct nfs4_lockdata *nfs4_alloc_lockdata(struct file_lock *fl,
>  	p->lsp = lsp;
>  	p->server = server;
>  	p->ctx = get_nfs_open_context(ctx);
> +	if (!p->ctx)
> +		goto out_free_seqid;
>  	locks_init_lock(&p->fl);
>  	locks_copy_lock(&p->fl, fl);
>  	return p;

-- 
Jeff Layton <jlayton@kernel.org>