All of lore.kernel.org
 help / color / mirror / Atom feed
From: Olivier L'Heureux <olivier.lheureux@mind.be>
To: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org,
	Olivier L'Heureux <olivier.lheureux@mind.be>
Subject: [PATCH 3/7] Bluetooth: L2CAP: use refcount on struct l2cap_chan->conn
Date: Tue,  5 Sep 2023 00:11:54 +0200	[thread overview]
Message-ID: <20230904221158.35425-4-olivier.lheureux@mind.be> (raw)
In-Reply-To: <20230904221158.35425-1-olivier.lheureux@mind.be>

We have detected a memory leak of "struct l2cap_conn" objects, and of
"struct hci_conn" in the bluetooth subsystem [12]. The second is a
consequence of the first. This memory leak was also detected by syzbot
[13].

"struct l2cap_conn" is allocated in "l2cap_conn_add()" [2]. "struct
l2cap_conn" is reference-counted [1] by its "ref" member [3]: every
holder of a reference to it should increment the reference counter by
calling "l2cap_conn_get()" [4], and every reference holder that stops
keeping the reference should call "l2cap_conn_put()" [5]. The "struct
l2cap_conn" is freed when the counter reaches 0, meaning there is no
reference holder any more.

This mechanism is already used by the "hidp_session_new()" [6] and
"session_free()" [7] functions that create and delete the "struct
hidp_session" [8] objects, which contains a "conn" reference to a
"struct l2cap_conn".

The same reference counting mechanism is not used for the "conn"
reference kept in the "struct l2cap_chan" [9] object. There were two
places where the reference counting was not applied:

 1. In "__l2cap_chan_add()" [11], the "struct l2cap_conn" reference is
    stored in the "conn" member of the "struct l2cap_chan" [9].
 2. In "l2cap_chan_del()" [10], the "conn" member of the "struct
    l2cap_chan" is set to NULL.

To apply the reference counting to the "conn" member of the "struct
l2cap_chan", we use "l2cap_conn_get()" in "__l2cap_chan_add()", where
we store the reference, and "l2cap_conn_put()" in "l2cap_chan_del()",
where we drop the reference.

Handling the "conn" member of the "struct l2cap_chan" with the
existing reference counter will help to fix the kernel memory leak in
a following commit.

References:
- [1]  "Adding reference counters (krefs) to kernel objects"
       <https://www.kernel.org/doc/html/v6.5/core-api/kref.html>
- [2]  "l2cap_conn_add()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L7833>
- [3]  "struct l2cap_conn"
       <https://elixir.bootlin.com/linux/v6.5/source/include/net/bluetooth/l2cap.h#L674>
- [4]  "l2cap_conn_get()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L1952>
- [5]  "l2cap_conn_put()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L1959>
- [6]  "hidp_session_new()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/hidp/core.c#L910>
- [7]  "session_free()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/hidp/core.c#L979>
- [8]  "struct hidp_session"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/hidp/hidp.h#L137>
- [9]  "struct l2cap_chan"
       <https://elixir.bootlin.com/linux/v6.5/source/include/net/bluetooth/l2cap.h#L540>
- [10] "l2cap_chan_del()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L642>
- [11] "__l2cap_chan_add()"
       <https://elixir.bootlin.com/linux/v6.5/source/net/bluetooth/l2cap_core.c#L583>
- [12] "ble-memleak-repro"
       <https://gitlab.com/essensium-mind/ble-memleak-repro.git>
- [13] "[syzbot] [bluetooth?] memory leak in hci_conn_add (2)"
       <https://lore.kernel.org/linux-bluetooth/0000000000000fd01206046e7410@google.com/T/#u>

Signed-off-by: Olivier L'Heureux <olivier.lheureux@fortrobotics.com>
Signed-off-by: Olivier L'Heureux <olivier.lheureux@mind.be>
---
 net/bluetooth/l2cap_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c749b434df97..768632fba6f8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -587,7 +587,7 @@ void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
 
-	chan->conn = conn;
+	chan->conn = l2cap_conn_get(conn);
 
 	switch (chan->chan_type) {
 	case L2CAP_CHAN_CONN_ORIENTED:
@@ -669,6 +669,8 @@ void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 		if (mgr && mgr->bredr_chan == chan)
 			mgr->bredr_chan = NULL;
+
+		l2cap_conn_put(conn);
 	}
 
 	if (chan->hs_hchan) {
-- 
2.39.2


  parent reply	other threads:[~2023-09-04 22:12 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-04 22:11 [PATCH RFC 0/7] Fix a memory leak in Bluetooth L2CAP Olivier L'Heureux
2023-09-04 22:11 ` [PATCH 1/7] ARM: dts: stm32: Add Bluetooth (usart2) feature on stm32mp157x Olivier L'Heureux
2023-09-04 22:38   ` Fix a memory leak in Bluetooth L2CAP bluez.test.bot
2023-09-04 22:11 ` [PATCH 2/7] Bluetooth: add many traces for allocation/free/refcounting Olivier L'Heureux
2023-09-04 22:11 ` Olivier L'Heureux [this message]
2023-09-04 22:11 ` [PATCH 4/7] Bluetooth: L2CAP: free the leaking struct l2cap_conn Olivier L'Heureux
2023-09-04 22:11 ` [PATCH 5/7] Bluetooth: introduce hci_conn_free() for better structure Olivier L'Heureux
2023-09-04 22:11 ` [PATCH 6/7] Bluetooth: L2CAP: inc refcount if reuse struct l2cap_conn Olivier L'Heureux
2023-09-04 22:11 ` [PATCH 7/7] Bluetooth: unlink objects to avoid use-after-free Olivier L'Heureux
2023-09-05 20:42 ` [PATCH RFC 0/7] Fix a memory leak in Bluetooth L2CAP Luiz Augusto von Dentz
2023-09-12 19:19   ` Luiz Augusto von Dentz
2023-09-13 22:25   ` Olivier L'Heureux
2023-09-14 21:17     ` Luiz Augusto von Dentz
2023-11-07  8:46       ` Marleen Vos
2023-11-07 15:10         ` Luiz Augusto von Dentz
     [not found]           ` <CAJav_R4Dp1rddk67vNDPvwS4208pk7-aORXM0KiwXPaOkXmGkw@mail.gmail.com>
2023-11-13 14:58             ` Luiz Augusto von Dentz
2023-11-13 15:01               ` Marleen Vos

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230904221158.35425-4-olivier.lheureux@mind.be \
    --to=olivier.lheureux@mind.be \
    --cc=johan.hedberg@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=marcel@holtmann.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.