[PATCH 0/2] nfs: handle failure during allocing lock/unlock data

[PATCH 0/2] nfs: handle failure during allocing lock/unlock data

[Li Lingfeng]

Lack of memory can cause nfs_lock_context allocation failures in unlock
paths, triggering NULL pointer dereference upon unlock completion.
Additionally, failed nfs_open_context acquisition may lead to similar
vulnerabilities. Proper error handling during lock/unlock data
initialization prevents critical faults.

Li Lingfeng (2):
  nfs: handle failure of nfs_get_lock_context in unlock path
  nfs: handle failure of get_nfs_open_context

 fs/nfs/nfs4proc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH 1/2] nfs: handle failure of nfs_get_lock_context in unlock path

[Li Lingfeng]

When memory is insufficient, the allocation of nfs_lock_context in
nfs_get_lock_context() fails and returns -ENOMEM. If we mistakenly treat
an nfs4_unlockdata structure (whose l_ctx member has been set to -ENOMEM)
as valid and proceed to execute rpc_run_task(), this will trigger a NULL
pointer dereference in nfs4_locku_prepare. For example:

BUG: kernel NULL pointer dereference, address: 000000000000000c
PGD 0 P4D 0
Oops: Oops: 0000 [#1] SMP PTI
CPU: 15 UID: 0 PID: 12 Comm: kworker/u64:0 Not tainted 6.15.0-rc2-dirty #60
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40
Workqueue: rpciod rpc_async_schedule
RIP: 0010:nfs4_locku_prepare+0x35/0xc2
Code: 89 f2 48 89 fd 48 c7 c7 68 69 ef b5 53 48 8b 8e 90 00 00 00 48 89 f3
RSP: 0018:ffffbbafc006bdb8 EFLAGS: 00010246
RAX: 000000000000004b RBX: ffff9b964fc1fa00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: fffffffffffffff4 RDI: ffff9ba53fddbf40
RBP: ffff9ba539934000 R08: 0000000000000000 R09: ffffbbafc006bc38
R10: ffffffffb6b689c8 R11: 0000000000000003 R12: ffff9ba539934030
R13: 0000000000000001 R14: 0000000004248060 R15: ffffffffb56d1c30
FS: 0000000000000000(0000) GS:ffff9ba5881f0000(0000) knlGS:00000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000000c CR3: 000000093f244000 CR4: 00000000000006f0
Call Trace:
 <TASK>
 __rpc_execute+0xbc/0x480
 rpc_async_schedule+0x2f/0x40
 process_one_work+0x232/0x5d0
 worker_thread+0x1da/0x3d0
 ? __pfx_worker_thread+0x10/0x10
 kthread+0x10d/0x240
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x34/0x50
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30
 </TASK>
Modules linked in:
CR2: 000000000000000c
---[ end trace 0000000000000000 ]---

Free the allocated nfs4_unlockdata when nfs_get_lock_context() fails and
return NULL to terminate subsequent rpc_run_task, preventing NULL pointer
dereference.

Fixes: f30cb757f680 ("NFS: Always wait for I/O completion before unlock")
Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfs/nfs4proc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 970f28dbf253..9f5689c43a50 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7074,10 +7074,18 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
 	struct nfs4_unlockdata *p;
 	struct nfs4_state *state = lsp->ls_state;
 	struct inode *inode = state->inode;
+	struct nfs_lock_context *l_ctx;
 
 	p = kzalloc(sizeof(*p), GFP_KERNEL);
 	if (p == NULL)
 		return NULL;
+	l_ctx = nfs_get_lock_context(ctx);
+	if (!IS_ERR(l_ctx)) {
+		p->l_ctx = l_ctx;
+	} else {
+		kfree(p);
+		return NULL;
+	}
 	p->arg.fh = NFS_FH(inode);
 	p->arg.fl = &p->fl;
 	p->arg.seqid = seqid;
@@ -7085,7 +7093,6 @@ static struct nfs4_unlockdata *nfs4_alloc_unlockdata(struct file_lock *fl,
 	p->lsp = lsp;
 	/* Ensure we don't close file until we're done freeing locks! */
 	p->ctx = get_nfs_open_context(ctx);
-	p->l_ctx = nfs_get_lock_context(ctx);
 	locks_init_lock(&p->fl);
 	locks_copy_lock(&p->fl, fl);
 	p->server = NFS_SERVER(inode);
-- 
2.31.1

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

[Li Lingfeng]

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);
+	if (open_ctx)
+		p->ctx = open_ctx;
+	else
+		goto out_free;
 	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;
-- 
2.31.1

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

[Jeff Layton]

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>

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

[Li Lingfeng]

在 2025/4/19 20:34, Jeff Layton 写道:
> 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?
Hi Jeff,

Thank you for the feedback.
Adding a comment instead of this patch may be better.

However, I’d like to seek your guidance on a broader question: For
scenarios where an error condition ​currently cannot occur but would lead
to severe consequences (e.g., NULL pointer dereference, data corruption)
if it ever did happen (e.g., due to future code changes or bugs), do you
recommend proactively adding error handling as a defensive measure?

My rationale:
​Current code: No code path triggers this condition today --> Handling
code would be "dead" for now.
​Future risks: If a bug introduced later allows the condition to occur,
silent failure or crashes could result.
Is there a kernel/dev policy on such preemptive safeguards? Or should we
address these only when the triggering scenarios materialize?

Your insight would help me align with the project’s practices.
Thanks in advance!

Best regards,
Lingfeng
>
>
>> +	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;

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

[Jeff Layton]

On Mon, 2025-04-21 at 09:56 +0800, Li Lingfeng wrote:
> 在 2025/4/19 20:34, Jeff Layton 写道:
> > 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?
> Hi Jeff,
> 
> Thank you for the feedback.
> Adding a comment instead of this patch may be better.
> 
> However, I’d like to seek your guidance on a broader question: For
> scenarios where an error condition ​currently cannot occur but would lead
> to severe consequences (e.g., NULL pointer dereference, data corruption)
> if it ever did happen (e.g., due to future code changes or bugs), do you
> recommend proactively adding error handling as a defensive measure?
> 
> My rationale:
> ​Current code: No code path triggers this condition today --> Handling
> code would be "dead" for now.
> ​Future risks: If a bug introduced later allows the condition to occur,
> silent failure or crashes could result.
> Is there a kernel/dev policy on such preemptive safeguards? Or should we
> address these only when the triggering scenarios materialize?
> 
> Your insight would help me align with the project’s practices.
> Thanks in advance!
> 

There is no firm policy here. We just have to make a judgment call in
these situations.

In general, we don't want to litter the code with a lot of conditionals
or BUG_ONs/WARN_ONs for cases that can really never happen, as that
might slow things down for little benefit, and it makes the code less
readable. OTOH, being proactive about catching errors is a good thing,
so if there is any chance that things could change in the future, it's
good to have a warning about it.

In this particular case, given that we have to hold a reference in
order to pass a pointer to the ctx in the first place, there is little
value in doing (e.g.) WARN_ON(!open_ctx), as that should really never
happen.



> Best regards,
> Lingfeng
> > 
> > 
> > > +	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>