Re: [PATCH v2] nfc: llcp: Fix nfc_dev refcount leak in connect

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]?part=1
---
Now I have all the information needed to write the report.

commit 01a1553c1a8c5e891087997a8fe9ee0913eb3c57
Author: Shuangpeng Bai <[email protected]>

nfc: llcp: Fix nfc_dev refcount leak in connect

This patch drops the nfc_dev reference in llcp_sock_release() when the
socket is still in LLCP_CONNECTING, addressing the leak path where
llcp_sock_destruct() only puts the device on LLCP_CONNECTED. The gate
is keyed on sk_state at release time.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index feab29fc62f4..0a3d87c2c9b3 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -633,10 +633,12 @@ static int llcp_sock_release(struct socket *sock)
>
>  	if (sock->type == SOCK_RAW)
>  		nfc_llcp_sock_unlink(&local->raw_sockets, sk);
> -	else if (sk->sk_state == LLCP_CONNECTING)
> +	else if (sk->sk_state == LLCP_CONNECTING) {
>  		nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
> -	else
> +		nfc_put_device(llcp_sock->dev);
> +	} else {
>  		nfc_llcp_sock_unlink(&local->sockets, sk);
> +	}

[High]
Can the leak still occur when the peer answers the outstanding CONNECT
with a DM before userspace closes the socket?

After llcp_sock_connect() returns -EINPROGRESS, the sk sits on
local->connecting_sockets with sk_state == LLCP_CONNECTING and holds a
device reference. If a DM with LLCP_DM_NOBOUND or LLCP_DM_REJ arrives,
nfc_llcp_recv_dm() looks up the sk via nfc_llcp_connecting_sock_get()
and does:

    sk->sk_err = ENXIO;
    sk->sk_state = LLCP_CLOSED;
    sk->sk_state_change(sk);

It does not unlink the sk from connecting_sockets and does not drop the
device reference.

When llcp_sock_release() then runs, sk_state is LLCP_CLOSED, so this
new branch is skipped:

    else if (sk->sk_state == LLCP_CONNECTING) {
        nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
        nfc_put_device(llcp_sock->dev);
    } else {
        nfc_llcp_sock_unlink(&local->sockets, sk);
    }

Two things fall out of the else branch:

1. nfc_llcp_sock_unlink() is called against &local->sockets, but the sk
   is actually hashed on &local->connecting_sockets. sk_del_node_init()
   still removes it, but the wrong rwlock is taken, racing with
   read-side walkers of connecting_sockets such as
   nfc_llcp_connecting_sock_get() and nfc_llcp_recv_cc().

2. The device reference taken by llcp_sock_connect() is not dropped.
   llcp_sock_destruct() only puts it on LLCP_CONNECTED:

     if (sk->sk_state == LLCP_CONNECTED)
             nfc_put_device(llcp_sock->dev);

Is gating on sk_state at release time sufficient, or does the release
path need to track connecting_sockets membership (for example a
per-sock flag set at nfc_llcp_sock_link(&local->connecting_sockets, ...)
and cleared on transitions out of that list) so both the unlink target
and the nfc_put_device() are chosen based on where the sk actually
lives rather than its current state?

The DM-driven variant of this leak predates b4011239a08e, but the Fixes:
tag and the subject scope this patch as the fix for the connect-time
nfc_dev leak, so it seems worth handling in the same change.