Re: [PATCH v2] Bluetooth: hci_core: fix suspicious RCU usage in hci_conn_drop()

From: Yunseong Kim
Date: Sat Jul 27 2024 - 14:39:37 EST


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