Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Sun, 03 Dec 2023 23:38:20 +0530
From:   Siddh Raman Pant <code@siddh.me>
To:     "Suman Ghosh" <sumang@marvell.com>
Cc:     "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>,
        "David S. Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>,
        "Paolo Abeni" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Samuel Ortiz" <sameo@linux.intel.com>,
        "syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com" 
        <syzbot+bbe84a4010eeea00982d@syzkaller.appspotmail.com>
Message-ID: <18c30ddee40.70f9a1a945075.1438711881490299499@siddh.me>
In-Reply-To: <SJ0PR18MB5216AB7B26C1F34A8748F2DADB87A@SJ0PR18MB5216.namprd18.prod.outlook.com>
References: <cover.1701530776.git.code@siddh.me>
 <476cccdcb57645784889fc82f0c7c10ff4c8b8c0.1701530776.git.code@siddh.me> <SJ0PR18MB5216AB7B26C1F34A8748F2DADB87A@SJ0PR18MB5216.namprd18.prod.outlook.com>
Subject: 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
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Importance: Medium
User-Agent: Zoho Mail
Precedence: bulk

On Sun, 03 Dec 2023 22:29:39 +0530, Suman Ghosh wrote:
> Hi Siddh,
>=20
> >@@ -180,6 +183,7 @@ int nfc_llcp_local_put(struct nfc_llcp_local *local)
> > =C2=A0=C2=A0=C2=A0=C2=A0if (local =3D=3D NULL)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0;
> >
> >+=C2=A0=C2=A0=C2=A0=C2=A0nfc_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=20
> "sk->sk_state =3D=3D LLCP_CONNECTED", with this change we are doing it ag=
ain.
> 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.

> > =C2=A0=C2=A0=C2=A0=C2=A0new_sock->local =3D nfc_llcp_local_get(local);
> >+=C2=A0=C2=A0=C2=A0=C2=A0if (!new_sock->local) {
> >+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0reason =3D LLCP_DM_REJ;
> >+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0release_sock(&sock->sk)=
;
> >+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sock_put(&sock->sk);
> >+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0sock_put(&new_sock->sk)=
;
> [Suman] don't we need to free new_sock? nfc_llcp_sock_free()?
>
> [...]
>
> >+=C2=A0=C2=A0=C2=A0=C2=A0local->dev =3D nfc_get_device(ndev->idx);
> >+=C2=A0=C2=A0=C2=A0=C2=A0if (!local->dev)
> >+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return -ENODEV;
> [Suman] Memory leak here. Need to call kfree(local).

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

Thanks,
Siddh