[v3] nfc: llcp: fix double put/unlock on LLCP_CLOSED in recv handlers

[PATCH v3 1/2] nfc: llcp: avoid double release/put on LLCP_CLOSED in nfc_llcp_recv_disc()

Posted by Qianchang Zhao 1 month, 3 weeks ago

nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().

In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state,
the code used to perform release_sock() and nfc_llcp_sock_put() in the
CLOSED branch but then continued execution and later performed the same
cleanup again on the common exit path. This results in refcount imbalance
(double put) and unbalanced lock release.

Remove the redundant CLOSED-branch cleanup so that release_sock() and
nfc_llcp_sock_put() are performed exactly once via the common exit path, 
while keeping the existing DM_DISC reply behavior.

Fixes: d646960f7986 ("NFC: Initial LLCP support")
Cc: stable@vger.kernel.org
Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
---
 net/nfc/llcp_core.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index beeb3b4d2..ed37604ed 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
 
 	nfc_llcp_socket_purge(llcp_sock);
 
-	if (sk->sk_state == LLCP_CLOSED) {
-		release_sock(sk);
-		nfc_llcp_sock_put(llcp_sock);
-	}
-
 	if (sk->sk_state == LLCP_CONNECTED) {
 		nfc_put_device(local->dev);
 		sk->sk_state = LLCP_CLOSED;
-- 
2.34.1

Re: [PATCH v3 1/2] nfc: llcp: avoid double release/put on LLCP_CLOSED in nfc_llcp_recv_disc()

Posted by Paolo Abeni 1 month, 1 week ago

On 12/18/25 3:59 AM, Qianchang Zhao wrote:
> nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().
> 
> In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state,
> the code used to perform release_sock() and nfc_llcp_sock_put() in the
> CLOSED branch but then continued execution and later performed the same
> cleanup again on the common exit path. This results in refcount imbalance
> (double put) and unbalanced lock release.
> 
> Remove the redundant CLOSED-branch cleanup so that release_sock() and
> nfc_llcp_sock_put() are performed exactly once via the common exit path, 
> while keeping the existing DM_DISC reply behavior.
> 
> Fixes: d646960f7986 ("NFC: Initial LLCP support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
> ---
>  net/nfc/llcp_core.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index beeb3b4d2..ed37604ed 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
>  
>  	nfc_llcp_socket_purge(llcp_sock);
>  
> -	if (sk->sk_state == LLCP_CLOSED) {
> -		release_sock(sk);
> -		nfc_llcp_sock_put(llcp_sock);

To rephrase Krzysztof concernt, this does not looks like the correct
fix: later on nfc_llcp_recv_disc() will try a send over a closed socket,
which looks wrong. Instead you could just return after
nfc_llcp_sock_put(), or do something alike:

	if (sk->sk_state == LLCP_CLOSED)
		goto cleanup;

	// ...


cleanup:
	release_sock(sk);
	nfc_llcp_sock_put(llcp_sock);
}

/P

Re: [PATCH v3 1/2] nfc: llcp: avoid double release/put on LLCP_CLOSED in nfc_llcp_recv_disc()

Posted by Paolo Abeni 1 month, 1 week ago

On 12/28/25 10:02 AM, Paolo Abeni wrote:
> On 12/18/25 3:59 AM, Qianchang Zhao wrote:
>> nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().
>>
>> In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state,
>> the code used to perform release_sock() and nfc_llcp_sock_put() in the
>> CLOSED branch but then continued execution and later performed the same
>> cleanup again on the common exit path. This results in refcount imbalance
>> (double put) and unbalanced lock release.
>>
>> Remove the redundant CLOSED-branch cleanup so that release_sock() and
>> nfc_llcp_sock_put() are performed exactly once via the common exit path, 
>> while keeping the existing DM_DISC reply behavior.
>>
>> Fixes: d646960f7986 ("NFC: Initial LLCP support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
>> ---
>>  net/nfc/llcp_core.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
>> index beeb3b4d2..ed37604ed 100644
>> --- a/net/nfc/llcp_core.c
>> +++ b/net/nfc/llcp_core.c
>> @@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
>>  
>>  	nfc_llcp_socket_purge(llcp_sock);
>>  
>> -	if (sk->sk_state == LLCP_CLOSED) {
>> -		release_sock(sk);
>> -		nfc_llcp_sock_put(llcp_sock);
> 
> To rephrase Krzysztof concernt, this does not looks like the correct
> fix: later on nfc_llcp_recv_disc() will try a send over a closed socket,
> which looks wrong. Instead you could just return after
> nfc_llcp_sock_put(), or do something alike:
> 
> 	if (sk->sk_state == LLCP_CLOSED)
> 		goto cleanup;
> 
> 	// ...
> 
> 
> cleanup:
> 	release_sock(sk);
> 	nfc_llcp_sock_put(llcp_sock);
> }

I'm sorry for the confusing feedback above.

I read the comments on patch 2/2 only after processing this one.

Indeed following the half-interrupted discussion on old revision, with
bad patch splitting is quite difficult.

@Qianchang Zhao: my _guess_ is that on LLCP_CLOSED the code has to
release the final sk reference... In any case discussion an a patch
series revision is not concluded until the reviewer agrees on that.

@Krzysztof: ... but still it looks like in the current code there is a
double release on the sk socket lock, which looks wrong, what am I
missing here?

/P

Re: [PATCH v3 1/2] nfc: llcp: avoid double release/put on LLCP_CLOSED in nfc_llcp_recv_disc()

Posted by Krzysztof Kozlowski 4 weeks, 1 day ago

On 28/12/2025 10:16, Paolo Abeni wrote:
> On 12/28/25 10:02 AM, Paolo Abeni wrote:
>> On 12/18/25 3:59 AM, Qianchang Zhao wrote:
>>> nfc_llcp_sock_get() takes a reference on the LLCP socket via sock_hold().
>>>
>>> In nfc_llcp_recv_disc(), when the socket is already in LLCP_CLOSED state,
>>> the code used to perform release_sock() and nfc_llcp_sock_put() in the
>>> CLOSED branch but then continued execution and later performed the same
>>> cleanup again on the common exit path. This results in refcount imbalance
>>> (double put) and unbalanced lock release.
>>>
>>> Remove the redundant CLOSED-branch cleanup so that release_sock() and
>>> nfc_llcp_sock_put() are performed exactly once via the common exit path, 
>>> while keeping the existing DM_DISC reply behavior.
>>>
>>> Fixes: d646960f7986 ("NFC: Initial LLCP support")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
>>> ---
>>>  net/nfc/llcp_core.c | 5 -----
>>>  1 file changed, 5 deletions(-)
>>>
>>> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
>>> index beeb3b4d2..ed37604ed 100644
>>> --- a/net/nfc/llcp_core.c
>>> +++ b/net/nfc/llcp_core.c
>>> @@ -1177,11 +1177,6 @@ static void nfc_llcp_recv_disc(struct nfc_llcp_local *local,
>>>  
>>>  	nfc_llcp_socket_purge(llcp_sock);
>>>  
>>> -	if (sk->sk_state == LLCP_CLOSED) {
>>> -		release_sock(sk);
>>> -		nfc_llcp_sock_put(llcp_sock);
>>
>> To rephrase Krzysztof concernt, this does not looks like the correct
>> fix: later on nfc_llcp_recv_disc() will try a send over a closed socket,
>> which looks wrong. Instead you could just return after
>> nfc_llcp_sock_put(), or do something alike:
>>
>> 	if (sk->sk_state == LLCP_CLOSED)
>> 		goto cleanup;
>>
>> 	// ...
>>
>>
>> cleanup:
>> 	release_sock(sk);
>> 	nfc_llcp_sock_put(llcp_sock);
>> }
> 
> I'm sorry for the confusing feedback above.
> 
> I read the comments on patch 2/2 only after processing this one.
> 
> Indeed following the half-interrupted discussion on old revision, with
> bad patch splitting is quite difficult.
> 
> @Qianchang Zhao: my _guess_ is that on LLCP_CLOSED the code has to
> release the final sk reference... In any case discussion an a patch
> series revision is not concluded until the reviewer agrees on that.

I would expect the code to return on LLCP_CLOSED, instead of proceeding
to sending nfc_llcp_send_dm() disconnect, because nfc_llcp_send_dm()
should happen earlier (before marking LLCP socket as closed), but that's
more of my assumption than actual knowledge.

> 
> @Krzysztof: ... but still it looks like in the current code there is a
> double release on the sk socket lock, which looks wrong, what am I
> missing here?

Author focused only on get/put and of course from that point of view
there is imbalance. But I asked at v2, for which there was still no
answer, what about releasing the initial reference from
nfc_llcp_sock_from_sn(). Maybe that was the intention here?

Best regards,
Krzysztof