Bluetooth: L2CAP: Fix leaking l2cap_conn objects

Commit Message

Luiz Augusto von Dentz Sept. 13, 2023, 7:38 p.m. UTC

From: Luiz Augusto von Dentz <[e-mail]>

l2cap_conn objects must be cleanup whenever its hci_chan is deleted by
hci_chan_list_flush since it means the connection attempt is being
aborted prematurely thus no calls to connect_cfm/disconnect_cfm would
be generated causing the l2cap_conn object to leak.

Fixes: 73d80deb7bdf ("Bluetooth: prioritizing data over HCI")
Reported-by: Olivier L'Heureux <[e-mail]>
Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_conn.c         |  7 +++++++
 net/bluetooth/l2cap_core.c       | 29 ++++++++++++++++++++++++++---
 3 files changed, 35 insertions(+), 3 deletions(-)

Comments

[e-mail] Sept. 13, 2023, 8:35 p.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=783902

---Test result---

Test Summary:
CheckPatch                    FAIL      1.28 seconds
GitLint                       PASS      0.33 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      32.82 seconds
CheckAllWarning               PASS      35.82 seconds
CheckSparse                   PASS      40.91 seconds
CheckSmatch                   PASS      115.39 seconds
BuildKernel32                 PASS      31.58 seconds
TestRunnerSetup               PASS      481.60 seconds
TestRunner_l2cap-tester       PASS      27.15 seconds
TestRunner_iso-tester         PASS      47.77 seconds
TestRunner_bnep-tester        PASS      10.25 seconds
TestRunner_mgmt-tester        PASS      217.08 seconds
TestRunner_rfcomm-tester      PASS      15.95 seconds
TestRunner_sco-tester         PASS      19.14 seconds
TestRunner_ioctl-tester       PASS      17.60 seconds
TestRunner_mesh-tester        PASS      13.02 seconds
TestRunner_smp-tester         PASS      14.12 seconds
TestRunner_userchan-tester    PASS      10.90 seconds
IncrementalBuild              PASS      29.64 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: L2CAP: Fix leaking l2cap_conn objects
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#88: 
Reported-by: Olivier L'Heureux <[e-mail]>
Signed-off-by: Luiz Augusto von Dentz <[e-mail]>

total: 0 errors, 1 warnings, 0 checks, 84 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13383710.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz Sept. 13, 2023, 8:44 p.m. UTC | #2

Hi Olivier,

On Wed, Sep 13, 2023 at 1:35 PM <[e-mail]> wrote:
>
> This is automated email and please do not reply to this email!
>
> Dear submitter,
>
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=783902
>
> ---Test result---
>
> Test Summary:
> CheckPatch                    FAIL      1.28 seconds
> GitLint                       PASS      0.33 seconds
> SubjectPrefix                 PASS      0.10 seconds
> BuildKernel                   PASS      32.82 seconds
> CheckAllWarning               PASS      35.82 seconds
> CheckSparse                   PASS      40.91 seconds
> CheckSmatch                   PASS      115.39 seconds
> BuildKernel32                 PASS      31.58 seconds
> TestRunnerSetup               PASS      481.60 seconds
> TestRunner_l2cap-tester       PASS      27.15 seconds
> TestRunner_iso-tester         PASS      47.77 seconds
> TestRunner_bnep-tester        PASS      10.25 seconds
> TestRunner_mgmt-tester        PASS      217.08 seconds
> TestRunner_rfcomm-tester      PASS      15.95 seconds
> TestRunner_sco-tester         PASS      19.14 seconds
> TestRunner_ioctl-tester       PASS      17.60 seconds
> TestRunner_mesh-tester        PASS      13.02 seconds
> TestRunner_smp-tester         PASS      14.12 seconds
> TestRunner_userchan-tester    PASS      10.90 seconds
> IncrementalBuild              PASS      29.64 seconds
>
> Details
> ##############################
> Test: CheckPatch - FAIL
> Desc: Run checkpatch.pl script
> Output:
> Bluetooth: L2CAP: Fix leaking l2cap_conn objects
> WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
> #88:
> Reported-by: Olivier L'Heureux <[e-mail]>
> Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
>
> total: 0 errors, 1 warnings, 0 checks, 84 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
>
> /github/workspace/src/src/13383710.patch has style problems, please review.
>
> NOTE: Ignored message types: UNKNOWN_COMMIT_ID
>
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
>

Would you be able to check if this would fix the problem for you? I
will be working on a way to reproduce it with the timeout as well,
just want a quick validation before doing it.

Dan Carpenter Sept. 14, 2023, 12:43 p.m. UTC | #3

Hi Luiz,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luiz-Augusto-von-Dentz/Bluetooth-L2CAP-Fix-leaking-l2cap_conn-objects/20230914-034053
base:   linus/master
patch link:    https://lore.kernel.org/r/20230913193839.3029428-1-luiz.dentz%40gmail.com
patch subject: [PATCH] Bluetooth: L2CAP: Fix leaking l2cap_conn objects
config: x86_64-randconfig-161-20230914 (https://download.01.org/0day-ci/archive/20230914/[e-mail]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230914/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Reported-by: Dan Carpenter <[e-mail]>
| Closes: https://lore.kernel.org/r/[e-mail]/

smatch warnings:
net/bluetooth/hci_conn.c:2725 hci_chan_del() warn: variable dereferenced before check 'conn' (see line 2723)

vim +/conn +2725 net/bluetooth/hci_conn.c

9472007c62ecc8 Andrei Emeltchenko     2012-09-06  2720  void hci_chan_del(struct hci_chan *chan)
73d80deb7bdf01 Luiz Augusto von Dentz 2011-11-02  2721  {
73d80deb7bdf01 Luiz Augusto von Dentz 2011-11-02  2722  	struct hci_conn *conn = chan->conn;
73d80deb7bdf01 Luiz Augusto von Dentz 2011-11-02 @2723  	struct hci_dev *hdev = conn->hdev;
                                                                                       ^^^^^^^^^^
Dereference

73d80deb7bdf01 Luiz Augusto von Dentz 2011-11-02  2724  
6fc2f406e4158e Luiz Augusto von Dentz 2023-09-13 @2725  	if (!conn)
                                                                    ^^^^^
Too late

6fc2f406e4158e Luiz Augusto von Dentz 2023-09-13  2726  		return;

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index bbad301f5781..21459c38a074 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -795,6 +795,8 @@  struct hci_chan {
 	unsigned int	sent;
 	__u8		state;
 	bool		amp;
+
+	void (*cleanup)(struct hci_chan *chan);
 };
 
 struct hci_conn_params {
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e62a5f368a51..814d8f3b029e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -2737,6 +2737,9 @@  void hci_chan_del(struct hci_chan *chan)
 	struct hci_conn *conn = chan->conn;
 	struct hci_dev *hdev = conn->hdev;
 
+	if (!conn)
+		return;
+
 	BT_DBG("%s hcon %p chan %p", hdev->name, conn, chan);
 
 	list_del_rcu(&chan->list);
@@ -2746,6 +2749,10 @@  void hci_chan_del(struct hci_chan *chan)
 	/* Prevent new hci_chan's to be created for this hci_conn */
 	set_bit(HCI_CONN_DROP, &conn->flags);
 
+	if (chan->cleanup)
+		chan->cleanup(chan);
+
+	chan->conn = NULL;
 	hci_conn_put(conn);
 
 	skb_queue_purge(&chan->data_q);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 17ca13e8c044..a791f28ccd6a 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1896,6 +1896,8 @@  static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	BT_DBG("hcon %p conn %p, err %d", hcon, conn, err);
 
+	hcon->l2cap_data = NULL;
+
 	kfree_skb(conn->rx_skb);
 
 	skb_queue_purge(&conn->pending_rx);
@@ -1931,13 +1933,15 @@  static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	mutex_unlock(&conn->chan_lock);
 
-	hci_chan_del(conn->hchan);
+	if (conn->hchan) {
+		conn->hchan->cleanup = NULL;
+		hci_chan_del(conn->hchan);
+		conn->hchan = NULL;
+	}
 
 	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
 		cancel_delayed_work_sync(&conn->info_timer);
 
-	hcon->l2cap_data = NULL;
-	conn->hchan = NULL;
 	l2cap_conn_put(conn);
 }
 
@@ -7830,6 +7834,24 @@  static void process_pending_rx(struct work_struct *work)
 		l2cap_recv_frame(conn, skb);
 }
 
+static void l2cap_conn_hchan_cleanup(struct hci_chan *hchan)
+{
+	struct hci_conn *hcon = hchan->conn;
+	struct l2cap_conn *conn;
+
+	if (!hcon)
+		return;
+
+	conn = hcon->l2cap_data;
+	if (!conn)
+		return;
+
+	/* hci_chan_del has been called so we shouldn't call it gain. */
+	conn->hchan = NULL;
+
+	l2cap_conn_del(hcon, bt_to_errno(HCI_ERROR_LOCAL_HOST_TERM));
+}
+
 static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
@@ -7852,6 +7874,7 @@  static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon)
 	hcon->l2cap_data = conn;
 	conn->hcon = hci_conn_get(hcon);
 	conn->hchan = hchan;
+	hchan->cleanup = l2cap_conn_hchan_cleanup;
 
 	BT_DBG("hcon %p conn %p hchan %p", hcon, conn, hchan);