RE: [EXT] [PATCH net-next v2 1/2] nfc: llcp_core: Hold a ref to llcp_local->dev when holding a ref to llcp_local

[Siddh Raman Pant]

On Sun, 03 Dec 2023 22:29:39 +0530, Suman Ghosh wrote:
> Hi Siddh,
> 
> >@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
> >     if (local == NULL)
> >         return 0;
> >
> >+    nfc_put_device(local->dev);
> [Suman] One question here, if we consider the path, nfc_llcp_mac_is_down() ->
> nfc_llcp_socket_release() -> nfc_llcp_local_put(), then inside
> nfc_llcp_socket_release() we are already doing nfc_put_device() if 
> "sk->sk_state == LLCP_CONNECTED", with this change we are doing it again.
> I guess you need to add some check to avoid that. Let me know if I am
> missing something.

The socket state is set to LLCP_CONNECTED in just two places:
nfc_llcp_recv_connect() and nfc_llcp_recv_cc().

nfc_get_device() is used prior to setting the socket state to
LLCP_CONNECTED in nfc_llcp_recv_connect(). After that, it calls
nfc_llcp_send_cc(), which I suppose is a connection PDU by some
Google-fu (NFC specs is paywalled).

In nfc_llcp_recv_cc(), we do not use nfc_get_device(), but since
one must send cc (which is done in nfc_llcp_recv_connect()), I
think we are good here.

This patch change doesn't touch any other refcounting. We increment
the refcount whenever we get the local, and decrement when we put it.
nfc_llcp_find_local() involves getting it, so all users of that
function increment the refcount, which is also the case with
nfc_llcp_mac_is_down(). The last nfc_llcp_local_put() then correctly
decrements the refcount.

If there is indeed a refcount error due to LLCP_CONNECTED, it probably
exists without this patch too.

> >     new_sock->local = nfc_llcp_local_get(local);
> >+    if (!new_sock->local) {
> >+        reason = LLCP_DM_REJ;
> >+        release_sock(&sock->sk);
> >+        sock_put(&sock->sk);
> >+        sock_put(&new_sock->sk);
> [Suman] don't we need to free new_sock? nfc_llcp_sock_free()?
>
> [...]
>
> >+    local->dev = nfc_get_device(ndev->idx);
> >+    if (!local->dev)
> >+        return -ENODEV;
> [Suman] Memory leak here. Need to call kfree(local).

Yes, you are correct. Very stupid of me. Will send a v3.

Thanks,
Siddh