[PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Dai Ngo]

Use-after-free occurred when the laundromat tried to free expired
cpntf_state entry on the s2s_cp_stateids list after inter-server
copy completed. The sc_cp_list that the expired copy state was
inserted on was already freed.

When COPY completes, the Linux client normally sends LOCKU(lock_state x),
FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
from the s2s_cp_stateids list before freeing the lock state's stid.

However, sometimes the CLOSE was sent before the FREE_STATEID request.
When this happens, the nfsd4_close_open_stateid call from nfsd4_close
frees all lock states on its st_locks list without cleaning up the copy
state on the sc_cp_list list. When the time the FREE_STATEID arrives the
server returns BAD_STATEID since the lock state was freed. This causes
the use-after-free error to occur when the laundromat tries to free
the expired cpntf_state.

This patch adds a call to nfs4_free_cpntf_statelist in
nfsd4_close_open_stateid to clean up the copy state before calling
free_ol_stateid_reaplist to free the lock state's stid on the reaplist.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 9409a0dc1b76..749f51dff5c7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 	struct nfs4_client *clp = s->st_stid.sc_client;
 	bool unhashed;
 	LIST_HEAD(reaplist);
+	struct nfs4_ol_stateid *stp;
 
 	spin_lock(&clp->cl_lock);
 	unhashed = unhash_open_stateid(s, &reaplist);
@@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 		if (unhashed)
 			put_ol_stateid_locked(s, &reaplist);
 		spin_unlock(&clp->cl_lock);
+		list_for_each_entry(stp, &reaplist, st_locks)
+			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
 		free_ol_stateid_reaplist(&reaplist);
 	} else {
 		spin_unlock(&clp->cl_lock);
-- 
2.9.5

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Jeff Layton]

On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
> Use-after-free occurred when the laundromat tried to free expired
> cpntf_state entry on the s2s_cp_stateids list after inter-server
> copy completed. The sc_cp_list that the expired copy state was
> inserted on was already freed.
> 
> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
> from the s2s_cp_stateids list before freeing the lock state's stid.
> 
> However, sometimes the CLOSE was sent before the FREE_STATEID request.
> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
> frees all lock states on its st_locks list without cleaning up the copy
> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
> server returns BAD_STATEID since the lock state was freed. This causes
> the use-after-free error to occur when the laundromat tries to free
> the expired cpntf_state.
> 
> This patch adds a call to nfs4_free_cpntf_statelist in
> nfsd4_close_open_stateid to clean up the copy state before calling
> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9409a0dc1b76..749f51dff5c7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  	struct nfs4_client *clp = s->st_stid.sc_client;
>  	bool unhashed;
>  	LIST_HEAD(reaplist);
> +	struct nfs4_ol_stateid *stp;
>  
>  	spin_lock(&clp->cl_lock);
>  	unhashed = unhash_open_stateid(s, &reaplist);
> @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  		if (unhashed)
>  			put_ol_stateid_locked(s, &reaplist);
>  		spin_unlock(&clp->cl_lock);
> +		list_for_each_entry(stp, &reaplist, st_locks)
> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>  		free_ol_stateid_reaplist(&reaplist);
>  	} else {
>  		spin_unlock(&clp->cl_lock);

Nice catch.

There are a number of places that call free_ol_stateid_reaplist. Is it
really only in nfsd4_close_open_stateid that we need to do this? I
wonder if it would be better to do this inside free_ol_stateid_reaplist
instead so that all of those callers clean up the copy states as well?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[dai.ngo]

On 8/1/22 4:47 AM, Jeff Layton wrote:
> On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
>> Use-after-free occurred when the laundromat tried to free expired
>> cpntf_state entry on the s2s_cp_stateids list after inter-server
>> copy completed. The sc_cp_list that the expired copy state was
>> inserted on was already freed.
>>
>> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
>> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
>> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
>> from the s2s_cp_stateids list before freeing the lock state's stid.
>>
>> However, sometimes the CLOSE was sent before the FREE_STATEID request.
>> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
>> frees all lock states on its st_locks list without cleaning up the copy
>> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
>> server returns BAD_STATEID since the lock state was freed. This causes
>> the use-after-free error to occur when the laundromat tries to free
>> the expired cpntf_state.
>>
>> This patch adds a call to nfs4_free_cpntf_statelist in
>> nfsd4_close_open_stateid to clean up the copy state before calling
>> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>>   fs/nfsd/nfs4state.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 9409a0dc1b76..749f51dff5c7 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>   	struct nfs4_client *clp = s->st_stid.sc_client;
>>   	bool unhashed;
>>   	LIST_HEAD(reaplist);
>> +	struct nfs4_ol_stateid *stp;
>>   
>>   	spin_lock(&clp->cl_lock);
>>   	unhashed = unhash_open_stateid(s, &reaplist);
>> @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>   		if (unhashed)
>>   			put_ol_stateid_locked(s, &reaplist);
>>   		spin_unlock(&clp->cl_lock);
>> +		list_for_each_entry(stp, &reaplist, st_locks)
>> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>>   		free_ol_stateid_reaplist(&reaplist);
>>   	} else {
>>   		spin_unlock(&clp->cl_lock);
> Nice catch.
>
> There are a number of places that call free_ol_stateid_reaplist. Is it
> really only in nfsd4_close_open_stateid that we need to do this? I
> wonder if it would be better to do this inside free_ol_stateid_reaplist
> instead so that all of those callers clean up the copy states as well?

Yes, we can do this in free_ol_stateid_reaplist too, I tested it.

The linux client uses either delegation state or lock state to send with
the COPY_NOTIFY to the source server. If the server grants the delegation
in the OPEN then the client uses the delegation state, otherwise it sends
the LOCK to the source and uses the lock state for the COPY_NOTIFY. This
problem happens only when the lock state is used *and* the client sends
the CLOSE and FREE_STATEID out of order.

free_ol_stateid_reaplist is called from release_open_stateid, release_openowner,
nfsd4_close_open_stateid and nfsd4_release_lockowner. Among these functions,
only nfsd4_close_open_stateid deals with lock state that may have cpntf_state
associated with it and only for the minorversion > 1 case.

nfsd4_release_lockowner will free the lock states but if the client has
not send LOCKU yet then put_ol_stateid_locked would fail to add the lock
state on the reaplist.

I'm ok to move it to free_ol_stateid_reaplist if you still think we should
and don't mind a little overhead on the unneeded cases.

-Dai

  

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Jeff Layton]

On Mon, 2022-08-01 at 11:52 -0700, dai.ngo@oracle.com wrote:
> On 8/1/22 4:47 AM, Jeff Layton wrote:
> > On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
> > > Use-after-free occurred when the laundromat tried to free expired
> > > cpntf_state entry on the s2s_cp_stateids list after inter-server
> > > copy completed. The sc_cp_list that the expired copy state was
> > > inserted on was already freed.
> > > 
> > > When COPY completes, the Linux client normally sends LOCKU(lock_state x),
> > > FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
> > > The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
> > > from the s2s_cp_stateids list before freeing the lock state's stid.
> > > 
> > > However, sometimes the CLOSE was sent before the FREE_STATEID request.
> > > When this happens, the nfsd4_close_open_stateid call from nfsd4_close
> > > frees all lock states on its st_locks list without cleaning up the copy
> > > state on the sc_cp_list list. When the time the FREE_STATEID arrives the
> > > server returns BAD_STATEID since the lock state was freed. This causes
> > > the use-after-free error to occur when the laundromat tries to free
> > > the expired cpntf_state.
> > > 
> > > This patch adds a call to nfs4_free_cpntf_statelist in
> > > nfsd4_close_open_stateid to clean up the copy state before calling
> > > free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
> > > 
> > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > ---
> > >   fs/nfsd/nfs4state.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 9409a0dc1b76..749f51dff5c7 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > >   	struct nfs4_client *clp = s->st_stid.sc_client;
> > >   	bool unhashed;
> > >   	LIST_HEAD(reaplist);
> > > +	struct nfs4_ol_stateid *stp;
> > >   
> > >   	spin_lock(&clp->cl_lock);
> > >   	unhashed = unhash_open_stateid(s, &reaplist);
> > > @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > >   		if (unhashed)
> > >   			put_ol_stateid_locked(s, &reaplist);
> > >   		spin_unlock(&clp->cl_lock);
> > > +		list_for_each_entry(stp, &reaplist, st_locks)
> > > +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
> > >   		free_ol_stateid_reaplist(&reaplist);
> > >   	} else {
> > >   		spin_unlock(&clp->cl_lock);
> > Nice catch.
> > 
> > There are a number of places that call free_ol_stateid_reaplist. Is it
> > really only in nfsd4_close_open_stateid that we need to do this? I
> > wonder if it would be better to do this inside free_ol_stateid_reaplist
> > instead so that all of those callers clean up the copy states as well?
> 
> Yes, we can do this in free_ol_stateid_reaplist too, I tested it.
> 
> The linux client uses either delegation state or lock state to send with
> the COPY_NOTIFY to the source server. If the server grants the delegation
> in the OPEN then the client uses the delegation state, otherwise it sends
> the LOCK to the source and uses the lock state for the COPY_NOTIFY. This
> problem happens only when the lock state is used *and* the client sends
> the CLOSE and FREE_STATEID out of order.
> 
> free_ol_stateid_reaplist is called from release_open_stateid, release_openowner,
> nfsd4_close_open_stateid and nfsd4_release_lockowner. Among these functions,
> only nfsd4_close_open_stateid deals with lock state that may have cpntf_state
> associated with it and only for the minorversion > 1 case.
> 
> nfsd4_release_lockowner will free the lock states but if the client has
> not send LOCKU yet then put_ol_stateid_locked would fail to add the lock
> state on the reaplist.
> 
> I'm ok to move it to free_ol_stateid_reaplist if you still think we should
> and don't mind a little overhead on the unneeded cases.
> 

If you think this is the only way this can happen, then the patch you
have is fine. In that case though, it might be good to have something
like this in free_ol_stateid_reaplist():

    WARN_ON(!list_empty(&stp->sc_cp_list));

...to try and catch cases where these objects slip through the cracks
after future changes.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[dai.ngo]

On 8/1/22 12:10 PM, Jeff Layton wrote:
> On Mon, 2022-08-01 at 11:52 -0700, dai.ngo@oracle.com wrote:
>> On 8/1/22 4:47 AM, Jeff Layton wrote:
>>> On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
>>>> Use-after-free occurred when the laundromat tried to free expired
>>>> cpntf_state entry on the s2s_cp_stateids list after inter-server
>>>> copy completed. The sc_cp_list that the expired copy state was
>>>> inserted on was already freed.
>>>>
>>>> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
>>>> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
>>>> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
>>>> from the s2s_cp_stateids list before freeing the lock state's stid.
>>>>
>>>> However, sometimes the CLOSE was sent before the FREE_STATEID request.
>>>> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
>>>> frees all lock states on its st_locks list without cleaning up the copy
>>>> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
>>>> server returns BAD_STATEID since the lock state was freed. This causes
>>>> the use-after-free error to occur when the laundromat tries to free
>>>> the expired cpntf_state.
>>>>
>>>> This patch adds a call to nfs4_free_cpntf_statelist in
>>>> nfsd4_close_open_stateid to clean up the copy state before calling
>>>> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>    fs/nfsd/nfs4state.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 9409a0dc1b76..749f51dff5c7 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>>>    	struct nfs4_client *clp = s->st_stid.sc_client;
>>>>    	bool unhashed;
>>>>    	LIST_HEAD(reaplist);
>>>> +	struct nfs4_ol_stateid *stp;
>>>>    
>>>>    	spin_lock(&clp->cl_lock);
>>>>    	unhashed = unhash_open_stateid(s, &reaplist);
>>>> @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>>>    		if (unhashed)
>>>>    			put_ol_stateid_locked(s, &reaplist);
>>>>    		spin_unlock(&clp->cl_lock);
>>>> +		list_for_each_entry(stp, &reaplist, st_locks)
>>>> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>>>>    		free_ol_stateid_reaplist(&reaplist);
>>>>    	} else {
>>>>    		spin_unlock(&clp->cl_lock);
>>> Nice catch.
>>>
>>> There are a number of places that call free_ol_stateid_reaplist. Is it
>>> really only in nfsd4_close_open_stateid that we need to do this? I
>>> wonder if it would be better to do this inside free_ol_stateid_reaplist
>>> instead so that all of those callers clean up the copy states as well?
>> Yes, we can do this in free_ol_stateid_reaplist too, I tested it.
>>
>> The linux client uses either delegation state or lock state to send with
>> the COPY_NOTIFY to the source server. If the server grants the delegation
>> in the OPEN then the client uses the delegation state, otherwise it sends
>> the LOCK to the source and uses the lock state for the COPY_NOTIFY. This
>> problem happens only when the lock state is used *and* the client sends
>> the CLOSE and FREE_STATEID out of order.
>>
>> free_ol_stateid_reaplist is called from release_open_stateid, release_openowner,
>> nfsd4_close_open_stateid and nfsd4_release_lockowner. Among these functions,
>> only nfsd4_close_open_stateid deals with lock state that may have cpntf_state
>> associated with it and only for the minorversion > 1 case.
>>
>> nfsd4_release_lockowner will free the lock states but if the client has
>> not send LOCKU yet then put_ol_stateid_locked would fail to add the lock
>> state on the reaplist.
>>
>> I'm ok to move it to free_ol_stateid_reaplist if you still think we should
>> and don't mind a little overhead on the unneeded cases.
>>
> If you think this is the only way this can happen, then the patch you
> have is fine. In that case though, it might be good to have something
> like this in free_ol_stateid_reaplist():
>
>      WARN_ON(!list_empty(&stp->sc_cp_list));
>
> ...to try and catch cases where these objects slip through the cracks
> after future changes.

Good suggestion, I'll add it to v2.

Thanks,
-Dai

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Jeff Layton]

On Mon, 2022-08-01 at 12:22 -0700, dai.ngo@oracle.com wrote:
> On 8/1/22 12:10 PM, Jeff Layton wrote:
> > On Mon, 2022-08-01 at 11:52 -0700, dai.ngo@oracle.com wrote:
> > > On 8/1/22 4:47 AM, Jeff Layton wrote:
> > > > On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
> > > > > Use-after-free occurred when the laundromat tried to free expired
> > > > > cpntf_state entry on the s2s_cp_stateids list after inter-server
> > > > > copy completed. The sc_cp_list that the expired copy state was
> > > > > inserted on was already freed.
> > > > > 
> > > > > When COPY completes, the Linux client normally sends LOCKU(lock_state x),
> > > > > FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
> > > > > The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
> > > > > from the s2s_cp_stateids list before freeing the lock state's stid.
> > > > > 
> > > > > However, sometimes the CLOSE was sent before the FREE_STATEID request.
> > > > > When this happens, the nfsd4_close_open_stateid call from nfsd4_close
> > > > > frees all lock states on its st_locks list without cleaning up the copy
> > > > > state on the sc_cp_list list. When the time the FREE_STATEID arrives the
> > > > > server returns BAD_STATEID since the lock state was freed. This causes
> > > > > the use-after-free error to occur when the laundromat tries to free
> > > > > the expired cpntf_state.
> > > > > 
> > > > > This patch adds a call to nfs4_free_cpntf_statelist in
> > > > > nfsd4_close_open_stateid to clean up the copy state before calling
> > > > > free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
> > > > > 
> > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > > > ---
> > > > >    fs/nfsd/nfs4state.c | 3 +++
> > > > >    1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > index 9409a0dc1b76..749f51dff5c7 100644
> > > > > --- a/fs/nfsd/nfs4state.c
> > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > > > >    	struct nfs4_client *clp = s->st_stid.sc_client;
> > > > >    	bool unhashed;
> > > > >    	LIST_HEAD(reaplist);
> > > > > +	struct nfs4_ol_stateid *stp;
> > > > >    
> > > > >    	spin_lock(&clp->cl_lock);
> > > > >    	unhashed = unhash_open_stateid(s, &reaplist);
> > > > > @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > > > >    		if (unhashed)
> > > > >    			put_ol_stateid_locked(s, &reaplist);
> > > > >    		spin_unlock(&clp->cl_lock);
> > > > > +		list_for_each_entry(stp, &reaplist, st_locks)
> > > > > +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
> > > > >    		free_ol_stateid_reaplist(&reaplist);
> > > > >    	} else {
> > > > >    		spin_unlock(&clp->cl_lock);
> > > > Nice catch.
> > > > 
> > > > There are a number of places that call free_ol_stateid_reaplist. Is it
> > > > really only in nfsd4_close_open_stateid that we need to do this? I
> > > > wonder if it would be better to do this inside free_ol_stateid_reaplist
> > > > instead so that all of those callers clean up the copy states as well?
> > > Yes, we can do this in free_ol_stateid_reaplist too, I tested it.
> > > 
> > > The linux client uses either delegation state or lock state to send with
> > > the COPY_NOTIFY to the source server. If the server grants the delegation
> > > in the OPEN then the client uses the delegation state, otherwise it sends
> > > the LOCK to the source and uses the lock state for the COPY_NOTIFY. This
> > > problem happens only when the lock state is used *and* the client sends
> > > the CLOSE and FREE_STATEID out of order.
> > > 
> > > free_ol_stateid_reaplist is called from release_open_stateid, release_openowner,
> > > nfsd4_close_open_stateid and nfsd4_release_lockowner. Among these functions,
> > > only nfsd4_close_open_stateid deals with lock state that may have cpntf_state
> > > associated with it and only for the minorversion > 1 case.
> > > 
> > > nfsd4_release_lockowner will free the lock states but if the client has
> > > not send LOCKU yet then put_ol_stateid_locked would fail to add the lock
> > > state on the reaplist.
> > > 
> > > I'm ok to move it to free_ol_stateid_reaplist if you still think we should
> > > and don't mind a little overhead on the unneeded cases.
> > > 
> > If you think this is the only way this can happen, then the patch you
> > have is fine. In that case though, it might be good to have something
> > like this in free_ol_stateid_reaplist():
> > 
> >      WARN_ON(!list_empty(&stp->sc_cp_list));
> > 
> > ...to try and catch cases where these objects slip through the cracks
> > after future changes.
> 
> Good suggestion, I'll add it to v2.
> 

Actually, it might be better to put it in nfs4_free_ol_stateid. If we're
freeing an open or lock stateid and this list isn't empty, then
something is wrong.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[dai.ngo]

On 8/1/22 12:26 PM, Jeff Layton wrote:
> On Mon, 2022-08-01 at 12:22 -0700, dai.ngo@oracle.com wrote:
>> On 8/1/22 12:10 PM, Jeff Layton wrote:
>>> On Mon, 2022-08-01 at 11:52 -0700, dai.ngo@oracle.com wrote:
>>>> On 8/1/22 4:47 AM, Jeff Layton wrote:
>>>>> On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
>>>>>> Use-after-free occurred when the laundromat tried to free expired
>>>>>> cpntf_state entry on the s2s_cp_stateids list after inter-server
>>>>>> copy completed. The sc_cp_list that the expired copy state was
>>>>>> inserted on was already freed.
>>>>>>
>>>>>> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
>>>>>> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
>>>>>> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
>>>>>> from the s2s_cp_stateids list before freeing the lock state's stid.
>>>>>>
>>>>>> However, sometimes the CLOSE was sent before the FREE_STATEID request.
>>>>>> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
>>>>>> frees all lock states on its st_locks list without cleaning up the copy
>>>>>> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
>>>>>> server returns BAD_STATEID since the lock state was freed. This causes
>>>>>> the use-after-free error to occur when the laundromat tries to free
>>>>>> the expired cpntf_state.
>>>>>>
>>>>>> This patch adds a call to nfs4_free_cpntf_statelist in
>>>>>> nfsd4_close_open_stateid to clean up the copy state before calling
>>>>>> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
>>>>>>
>>>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>>>> ---
>>>>>>     fs/nfsd/nfs4state.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>> index 9409a0dc1b76..749f51dff5c7 100644
>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>> @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>>>>>     	struct nfs4_client *clp = s->st_stid.sc_client;
>>>>>>     	bool unhashed;
>>>>>>     	LIST_HEAD(reaplist);
>>>>>> +	struct nfs4_ol_stateid *stp;
>>>>>>     
>>>>>>     	spin_lock(&clp->cl_lock);
>>>>>>     	unhashed = unhash_open_stateid(s, &reaplist);
>>>>>> @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>>>>>>     		if (unhashed)
>>>>>>     			put_ol_stateid_locked(s, &reaplist);
>>>>>>     		spin_unlock(&clp->cl_lock);
>>>>>> +		list_for_each_entry(stp, &reaplist, st_locks)
>>>>>> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>>>>>>     		free_ol_stateid_reaplist(&reaplist);
>>>>>>     	} else {
>>>>>>     		spin_unlock(&clp->cl_lock);
>>>>> Nice catch.
>>>>>
>>>>> There are a number of places that call free_ol_stateid_reaplist. Is it
>>>>> really only in nfsd4_close_open_stateid that we need to do this? I
>>>>> wonder if it would be better to do this inside free_ol_stateid_reaplist
>>>>> instead so that all of those callers clean up the copy states as well?
>>>> Yes, we can do this in free_ol_stateid_reaplist too, I tested it.
>>>>
>>>> The linux client uses either delegation state or lock state to send with
>>>> the COPY_NOTIFY to the source server. If the server grants the delegation
>>>> in the OPEN then the client uses the delegation state, otherwise it sends
>>>> the LOCK to the source and uses the lock state for the COPY_NOTIFY. This
>>>> problem happens only when the lock state is used *and* the client sends
>>>> the CLOSE and FREE_STATEID out of order.
>>>>
>>>> free_ol_stateid_reaplist is called from release_open_stateid, release_openowner,
>>>> nfsd4_close_open_stateid and nfsd4_release_lockowner. Among these functions,
>>>> only nfsd4_close_open_stateid deals with lock state that may have cpntf_state
>>>> associated with it and only for the minorversion > 1 case.
>>>>
>>>> nfsd4_release_lockowner will free the lock states but if the client has
>>>> not send LOCKU yet then put_ol_stateid_locked would fail to add the lock
>>>> state on the reaplist.
>>>>
>>>> I'm ok to move it to free_ol_stateid_reaplist if you still think we should
>>>> and don't mind a little overhead on the unneeded cases.
>>>>
>>> If you think this is the only way this can happen, then the patch you
>>> have is fine. In that case though, it might be good to have something
>>> like this in free_ol_stateid_reaplist():
>>>
>>>       WARN_ON(!list_empty(&stp->sc_cp_list));
>>>
>>> ...to try and catch cases where these objects slip through the cracks
>>> after future changes.
>> Good suggestion, I'll add it to v2.
>>
> Actually, it might be better to put it in nfs4_free_ol_stateid. If we're
> freeing an open or lock stateid and this list isn't empty, then
> something is wrong.

The linux client uses only delegation or lock state for COPY_NOTIFY.
However the protocol allows for using open state with COPY_NOTIFY too,
so adding the warning in nfs4_free_ol_stateid would cover behavior of
other clients. I'll add this to v2.

-Dai
  

>

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Jeff Layton]

On Mon, 2022-08-01 at 13:25 -0700, dai.ngo@oracle.com wrote:
> On 8/1/22 12:26 PM, Jeff Layton wrote:
> > On Mon, 2022-08-01 at 12:22 -0700, dai.ngo@oracle.com wrote:
> > > On 8/1/22 12:10 PM, Jeff Layton wrote:
> > > > On Mon, 2022-08-01 at 11:52 -0700, dai.ngo@oracle.com wrote:
> > > > > On 8/1/22 4:47 AM, Jeff Layton wrote:
> > > > > > On Sun, 2022-07-31 at 13:19 -0700, Dai Ngo wrote:
> > > > > > > Use-after-free occurred when the laundromat tried to free expired
> > > > > > > cpntf_state entry on the s2s_cp_stateids list after inter-server
> > > > > > > copy completed. The sc_cp_list that the expired copy state was
> > > > > > > inserted on was already freed.
> > > > > > > 
> > > > > > > When COPY completes, the Linux client normally sends LOCKU(lock_state x),
> > > > > > > FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
> > > > > > > The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
> > > > > > > from the s2s_cp_stateids list before freeing the lock state's stid.
> > > > > > > 
> > > > > > > However, sometimes the CLOSE was sent before the FREE_STATEID request.
> > > > > > > When this happens, the nfsd4_close_open_stateid call from nfsd4_close
> > > > > > > frees all lock states on its st_locks list without cleaning up the copy
> > > > > > > state on the sc_cp_list list. When the time the FREE_STATEID arrives the
> > > > > > > server returns BAD_STATEID since the lock state was freed. This causes
> > > > > > > the use-after-free error to occur when the laundromat tries to free
> > > > > > > the expired cpntf_state.
> > > > > > > 
> > > > > > > This patch adds a call to nfs4_free_cpntf_statelist in
> > > > > > > nfsd4_close_open_stateid to clean up the copy state before calling
> > > > > > > free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
> > > > > > > 
> > > > > > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > > > > > ---
> > > > > > >     fs/nfsd/nfs4state.c | 3 +++
> > > > > > >     1 file changed, 3 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > > > index 9409a0dc1b76..749f51dff5c7 100644
> > > > > > > --- a/fs/nfsd/nfs4state.c
> > > > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > > > @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > > > > > >     	struct nfs4_client *clp = s->st_stid.sc_client;
> > > > > > >     	bool unhashed;
> > > > > > >     	LIST_HEAD(reaplist);
> > > > > > > +	struct nfs4_ol_stateid *stp;
> > > > > > >     
> > > > > > >     	spin_lock(&clp->cl_lock);
> > > > > > >     	unhashed = unhash_open_stateid(s, &reaplist);
> > > > > > > @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> > > > > > >     		if (unhashed)
> > > > > > >     			put_ol_stateid_locked(s, &reaplist);
> > > > > > >     		spin_unlock(&clp->cl_lock);
> > > > > > > +		list_for_each_entry(stp, &reaplist, st_locks)
> > > > > > > +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
> > > > > > >     		free_ol_stateid_reaplist(&reaplist);
> > > > > > >     	} else {
> > > > > > >     		spin_unlock(&clp->cl_lock);
> > > > > > Nice catch.
> > > > > > 
> > > > > > There are a number of places that call free_ol_stateid_reaplist. Is it
> > > > > > really only in nfsd4_close_open_stateid that we need to do this? I
> > > > > > wonder if it would be better to do this inside free_ol_stateid_reaplist
> > > > > > instead so that all of those callers clean up the copy states as well?
> > > > > Yes, we can do this in free_ol_stateid_reaplist too, I tested it.
> > > > > 
> > > > > The linux client uses either delegation state or lock state to send with
> > > > > the COPY_NOTIFY to the source server. If the server grants the delegation
> > > > > in the OPEN then the client uses the delegation state, otherwise it sends
> > > > > the LOCK to the source and uses the lock state for the COPY_NOTIFY. This
> > > > > problem happens only when the lock state is used *and* the client sends
> > > > > the CLOSE and FREE_STATEID out of order.
> > > > > 
> > > > > free_ol_stateid_reaplist is called from release_open_stateid, release_openowner,
> > > > > nfsd4_close_open_stateid and nfsd4_release_lockowner. Among these functions,
> > > > > only nfsd4_close_open_stateid deals with lock state that may have cpntf_state
> > > > > associated with it and only for the minorversion > 1 case.
> > > > > 
> > > > > nfsd4_release_lockowner will free the lock states but if the client has
> > > > > not send LOCKU yet then put_ol_stateid_locked would fail to add the lock
> > > > > state on the reaplist.
> > > > > 
> > > > > I'm ok to move it to free_ol_stateid_reaplist if you still think we should
> > > > > and don't mind a little overhead on the unneeded cases.
> > > > > 
> > > > If you think this is the only way this can happen, then the patch you
> > > > have is fine. In that case though, it might be good to have something
> > > > like this in free_ol_stateid_reaplist():
> > > > 
> > > >       WARN_ON(!list_empty(&stp->sc_cp_list));
> > > > 
> > > > ...to try and catch cases where these objects slip through the cracks
> > > > after future changes.
> > > Good suggestion, I'll add it to v2.
> > > 
> > Actually, it might be better to put it in nfs4_free_ol_stateid. If we're
> > freeing an open or lock stateid and this list isn't empty, then
> > something is wrong.
> 
> The linux client uses only delegation or lock state for COPY_NOTIFY.
> However the protocol allows for using open state with COPY_NOTIFY too,
> so adding the warning in nfs4_free_ol_stateid would cover behavior of
> other clients. I'll add this to v2.
> 

Might also want to check this when freeing a deleg stid too.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Chuck Lever III]

> On Jul 31, 2022, at 4:19 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> Use-after-free occurred when the laundromat tried to free expired
> cpntf_state entry on the s2s_cp_stateids list after inter-server
> copy completed. The sc_cp_list that the expired copy state was
> inserted on was already freed.
> 
> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
> from the s2s_cp_stateids list before freeing the lock state's stid.
> 
> However, sometimes the CLOSE was sent before the FREE_STATEID request.
> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
> frees all lock states on its st_locks list without cleaning up the copy
> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
> server returns BAD_STATEID since the lock state was freed. This causes
> the use-after-free error to occur when the laundromat tries to free
> the expired cpntf_state.
> 
> This patch adds a call to nfs4_free_cpntf_statelist in
> nfsd4_close_open_stateid to clean up the copy state before calling
> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>

I'm interested in Olga's comments as well, so I'm going to
wait a bit before applying this one.

Also, did you figure out where this crash started to occur?
I'd like to have a precise sense of whether this should be
backported.


> ---
> fs/nfsd/nfs4state.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9409a0dc1b76..749f51dff5c7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> 	struct nfs4_client *clp = s->st_stid.sc_client;
> 	bool unhashed;
> 	LIST_HEAD(reaplist);
> +	struct nfs4_ol_stateid *stp;
> 
> 	spin_lock(&clp->cl_lock);
> 	unhashed = unhash_open_stateid(s, &reaplist);
> @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> 		if (unhashed)
> 			put_ol_stateid_locked(s, &reaplist);
> 		spin_unlock(&clp->cl_lock);
> +		list_for_each_entry(stp, &reaplist, st_locks)
> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
> 		free_ol_stateid_reaplist(&reaplist);
> 	} else {
> 		spin_unlock(&clp->cl_lock);
> -- 
> 2.9.5
> 

--
Chuck Lever

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[dai.ngo]

On 8/1/22 9:29 AM, Chuck Lever III wrote:
>
>> On Jul 31, 2022, at 4:19 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>> Use-after-free occurred when the laundromat tried to free expired
>> cpntf_state entry on the s2s_cp_stateids list after inter-server
>> copy completed. The sc_cp_list that the expired copy state was
>> inserted on was already freed.
>>
>> When COPY completes, the Linux client normally sends LOCKU(lock_state x),
>> FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
>> The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
>> from the s2s_cp_stateids list before freeing the lock state's stid.
>>
>> However, sometimes the CLOSE was sent before the FREE_STATEID request.
>> When this happens, the nfsd4_close_open_stateid call from nfsd4_close
>> frees all lock states on its st_locks list without cleaning up the copy
>> state on the sc_cp_list list. When the time the FREE_STATEID arrives the
>> server returns BAD_STATEID since the lock state was freed. This causes
>> the use-after-free error to occur when the laundromat tries to free
>> the expired cpntf_state.
>>
>> This patch adds a call to nfs4_free_cpntf_statelist in
>> nfsd4_close_open_stateid to clean up the copy state before calling
>> free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> I'm interested in Olga's comments as well, so I'm going to
> wait a bit before applying this one.
>
> Also, did you figure out where this crash started to occur?
> I'd like to have a precise sense of whether this should be
> backported.

I think this started with the commit 624322f1adc58 to introduce
COPY_NOTIFY operation.

-Dai

>
>
>> ---
>> fs/nfsd/nfs4state.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 9409a0dc1b76..749f51dff5c7 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>> 	struct nfs4_client *clp = s->st_stid.sc_client;
>> 	bool unhashed;
>> 	LIST_HEAD(reaplist);
>> +	struct nfs4_ol_stateid *stp;
>>
>> 	spin_lock(&clp->cl_lock);
>> 	unhashed = unhash_open_stateid(s, &reaplist);
>> @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>> 		if (unhashed)
>> 			put_ol_stateid_locked(s, &reaplist);
>> 		spin_unlock(&clp->cl_lock);
>> +		list_for_each_entry(stp, &reaplist, st_locks)
>> +			nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
>> 		free_ol_stateid_reaplist(&reaplist);
>> 	} else {
>> 		spin_unlock(&clp->cl_lock);
>> -- 
>> 2.9.5
>>
> --
> Chuck Lever
>
>
>

Re: [PATCH] NFSD: fix use-after-free on source server when doing inter-server copy

[Olga Kornievskaia]

On Mon, Aug 1, 2022 at 12:29 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jul 31, 2022, at 4:19 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> >
> > Use-after-free occurred when the laundromat tried to free expired
> > cpntf_state entry on the s2s_cp_stateids list after inter-server
> > copy completed. The sc_cp_list that the expired copy state was
> > inserted on was already freed.
> >
> > When COPY completes, the Linux client normally sends LOCKU(lock_state x),
> > FREE_STATEID(lock_state x) and CLOSE(open_state y) to the source server.
> > The nfs4_put_stid call from nfsd4_free_stateid cleans up the copy state
> > from the s2s_cp_stateids list before freeing the lock state's stid.
> >
> > However, sometimes the CLOSE was sent before the FREE_STATEID request.
> > When this happens, the nfsd4_close_open_stateid call from nfsd4_close
> > frees all lock states on its st_locks list without cleaning up the copy
> > state on the sc_cp_list list. When the time the FREE_STATEID arrives the
> > server returns BAD_STATEID since the lock state was freed. This causes
> > the use-after-free error to occur when the laundromat tries to free
> > the expired cpntf_state.
> >
> > This patch adds a call to nfs4_free_cpntf_statelist in
> > nfsd4_close_open_stateid to clean up the copy state before calling
> > free_ol_stateid_reaplist to free the lock state's stid on the reaplist.
> >
> > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>
> I'm interested in Olga's comments as well, so I'm going to
> wait a bit before applying this one.

Sorry folks, I totally missed this thread.... I was on vacation, came
back and started working on this after running into the oops with
Chuck's new patch set..

Well as you saw from my other post that my solution is different and
suggests putting cleanup of the copy_notify states together with
idr_remove() of the stateid it was associated with.

> Also, did you figure out where this crash started to occur?
> I'd like to have a precise sense of whether this should be
> backported.

I'm not going to claim this is the first occurrence but Jorge first
ran into this while testing ssc over iwarp on the 5.15-rc4 kernel.

>
>
> > ---
> > fs/nfsd/nfs4state.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 9409a0dc1b76..749f51dff5c7 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -6608,6 +6608,7 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> >       struct nfs4_client *clp = s->st_stid.sc_client;
> >       bool unhashed;
> >       LIST_HEAD(reaplist);
> > +     struct nfs4_ol_stateid *stp;
> >
> >       spin_lock(&clp->cl_lock);
> >       unhashed = unhash_open_stateid(s, &reaplist);
> > @@ -6616,6 +6617,8 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> >               if (unhashed)
> >                       put_ol_stateid_locked(s, &reaplist);
> >               spin_unlock(&clp->cl_lock);
> > +             list_for_each_entry(stp, &reaplist, st_locks)
> > +                     nfs4_free_cpntf_statelist(clp->net, &stp->st_stid);
> >               free_ol_stateid_reaplist(&reaplist);
> >       } else {
> >               spin_unlock(&clp->cl_lock);
> > --
> > 2.9.5
> >
>
> --
> Chuck Lever
>
>
>