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

[Yunseong Kim]

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