netdev - Re: [PATCH v2] Bluetooth: hci_core: fix suspicious RCU usage in hci_conn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b2232e8d-275b-4e98-84fe-bbb33e3c6b7e@gmail.com>
Date: Sun, 28 Jul 2024 03:39:18 +0900
From: Yunseong Kim <yskelg@...il.com>
To: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
 Marcel Holtmann <marcel@...tmann.org>,
 Johan Hedberg <johan.hedberg@...il.com>,
 Luiz Augusto von Dentz <luiz.dentz@...il.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org,
 stable@...r.kernel.org, linux-kernel@...r.kernel.org,
 Yeoreum Yun <yeoreum.yun@....com>
Subject: Re: [PATCH v2] Bluetooth: hci_core: fix suspicious RCU usage in
 hci_conn_drop()

Hi Tetsuo,

> Excuse me, but I can't interpret why this patch solves the warning.
> 
> The warning says that list_for_each_entry_rcu() { } in
> ieee80211_check_combinations() is called outside of rcu_read_lock() and
> rcu_read_unlock() pair, doesn't it? How does that connected to
> guarding hci_dev_test_flag() and queue_delayed_work() with rcu_read_lock()
> and rcu_read_unlock() pair? Unless you guard list_for_each_entry_rcu() { }
> in ieee80211_check_combinations() with rcu_read_lock() and rcu_read_unlock()
> pair (or annotate that appropriate locks are already held), I can't expect
> that the warning will be solved...

Thank you for the code review.

Sorry, I apologize for attaching the wrong kernel dump.

> Also, what guarantees that drain_workqueue() won't be disturbed by
> queue_work(disc_work) which will be called after "timeo" delay, for you are
> not explicitly cancelling scheduled "disc_work" (unlike "cmd_timer" work
> and "ncmd_timer" work shown below) before calling drain_workqueue() ?
> 
> 	/* Cancel these to avoid queueing non-chained pending work */
> 	hci_dev_set_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE);
> 	/* Wait for
> 	 *
> 	 *    if (!hci_dev_test_flag(hdev, HCI_CMD_DRAIN_WORKQUEUE))
> 	 *        queue_delayed_work(&hdev->{cmd,ncmd}_timer)
> 	 *
> 	 * inside RCU section to see the flag or complete scheduling.
> 	 */
> 	synchronize_rcu();
> 	/* Explicitly cancel works in case scheduled after setting the flag. */
> 	cancel_delayed_work(&hdev->cmd_timer);
> 	cancel_delayed_work(&hdev->ncmd_timer);
> 
> 	/* Avoid potential lockdep warnings from the *_flush() calls by
> 	 * ensuring the workqueue is empty up front.
> 	 */
> 	drain_workqueue(hdev->workqueue);

Please bear with me for a moment.

I'll attach the correct kernel dump and resend the patch email.

Warm regards,

Yunseong Kim