All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dean Jenkins <Dean_Jenkins@mentor.com>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: Dean Jenkins <Dean_Jenkins@mentor.com>,
	"Gustavo F . Padovan" <gustavo@padovan.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	<linux-bluetooth@vger.kernel.org>
Subject: [RFC V1 00/16] hci_ldisc hci_uart_tty_close() fixes
Date: Tue, 28 Mar 2017 18:50:13 +0100	[thread overview]
Message-ID: <1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com> (raw)

This is RFC patchset V1 which reorganises hci_uart_tty_close() to overcome a
design flaw. I would like some comments on the changes.

Design Flaw
===========

An example callstack is as follows

Have Bluetooth running using a BCSP based UART Bluetooth Radio Module.

Now kill the userland hciattach program by doing
killall hciattach

When hciattach terminates, it calls ioctl TIOCSETD to run:

tty_set_ldisc() takes a tty ref lock via tty_ldisc_lock
tty_ldisc_close()
hci_uart_tty_close()
hci_unregister_dev()
hci_dev_do_close()
__hci_req_sync() which tries to send a HCI RESET command which depends on
HCI_QUIRK_RESET_ON_CLOSE being enabled and that is the default condition.

The design flaw is that in order to send a HCI RESET command the BCSP Data Link
protocol layer must be able to send and receive BCSP frames from the UART port
that is physically connected to the BCSP based UART Bluetooth Radio Module. If
this data "pipe" is broken then BCSP will attempt to retransmit every 250ms
until the BCSP layer is closed. In addition, the HCI RESET command has a 2
second timeout which will block __hci_req_sync() for 2 seconds. Enabling
BT_DBG() shows BCSP attempting to retransmit during hci_uart_tty_close().

Meanwhile, tty_set_ldisc() has a tty ref lock which prevents flush_to_ldisc()
from passing the UART port data to the BCSP layer causing a loss of RX BCSP
frames. Similarly, the hci_uart_tty_wakeup() no longer runs. These commits do
not fix this tty ref lock issue.

This patchset removes hci_ldisc.c hci_uart_tty_close() breakages that prevent
__hci_req_sync() from being successful. There are also race conditions due to
the BCSP timeout handling being asynchronous to the thread running
hci_uart_tty_close() which can cause kernel crashes in particular when BCSP is
already retransmitting due to broken communications. Therefore, disabling
the sending of the HCI RESET command does not prevent some of the races.

Solution
========

Reorganise hci_uart_tty_close() so that hci_unregister_dev() can run with no
interference to the data pipe between the Data Link protocol layer such as BCSP
and the UART driver.

The patchset cleans up the HCI_UART_REGISTERED and HCI_UART_PROTO_READY flag
handling so that:
a) hdev is only valid when HCI_UART_REGISTERED is in the set state.
b) the proto Data Link functions pointers can only be used when
   HCI_UART_PROTO_READY is in the set state.

Note that flushing can corrupt any data being sent from BCSP to the UART driver
which is undesirable.

The most important patch is
"Bluetooth: hci_ldisc: Use rwlocking to avoid closing proto races"
which adds rwlocking around the clearing of the flag HCI_UART_PROTO_READY.
This is because the atomic flag HCI_UART_PROTO_READY cannot always prevent a
thread using a Data Link protocol layer function pointer during or after closure
of the Data Link protocol layer. This can result in a kernel crash. Remember
that the BCSP retransmission handling runs in a separate thread and tries to
send a new frame. Therefore there is a small window of a race condition for the
flag HCI_UART_PROTO_READY to be seen in the set state and then calls the
proto function pointer after the Data Link protocol layer has been closed,
resulting in a kernel crash.

Next steps
==========

I am still testing these changes but I would like some feedback on the proposed
changes.

I will reply to any feedback next week.

Thanks.


Dean Jenkins (16):
  Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work()
  Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev
  Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY
  Bluetooth: hci_ldisc: Add HCI RESET comment to hci_unregister_dev()
    call
  Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame()
  Bluetooth: hci_ldisc: Add protocol check to hci_uart_dequeue()
  Bluetooth: hci_ldisc: Add protocol check to hci_uart_tx_wakeup()
  Bluetooth: hci_ldisc: Separate flag handling in hci_uart_tty_close()
  Bluetooth: hci_ldisc: Tidy-up HCI_UART_REGISTERED in
    hci_uart_tty_close()
  Bluetooth: hci_ldisc: hci_uart_tty_close() detach tty after last msg
  Bluetooth: hci_ldisc: hci_uart_tty_close() move hci_uart_close()
  Bluetooth: hci_ldisc: hci_uart_tty_close() move cancel_work_sync()
  Bluetooth: hci_ldisc: hci_uart_tty_close() free hu->tx_skb
  Bluetooth: hci_ldisc: Simplify flushing
  Bluetooth: hci_ldisc: Use rwlocking to avoid closing proto races
  Bluetooth: hci_ldisc: Add ioctl_mutex avoiding concurrency

 drivers/bluetooth/hci_ldisc.c | 105 ++++++++++++++++++++++++++++++++----------
 drivers/bluetooth/hci_uart.h  |   3 ++
 2 files changed, 84 insertions(+), 24 deletions(-)

-- 
2.7.4

             reply	other threads:[~2017-03-28 17:50 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-28 17:50 Dean Jenkins [this message]
2017-03-28 17:50 ` [RFC V1 01/16] Bluetooth: hci_ldisc: Add missing return in hci_uart_init_work() Dean Jenkins
2017-03-28 17:50 ` [RFC V1 02/16] Bluetooth: hci_ldisc: Ensure hu->hdev set to NULL before freeing hdev Dean Jenkins
2017-03-28 17:50 ` [RFC V1 03/16] Bluetooth: hci_ldisc: Add missing clear HCI_UART_PROTO_READY Dean Jenkins
2017-03-28 17:50 ` [RFC V1 04/16] Bluetooth: hci_ldisc: Add HCI RESET comment to hci_unregister_dev() call Dean Jenkins
2017-03-30 10:11   ` Marcel Holtmann
2017-03-28 17:50 ` [RFC V1 05/16] Bluetooth: hci_ldisc: Add protocol check to hci_uart_send_frame() Dean Jenkins
2017-03-28 17:50 ` [RFC V1 06/16] Bluetooth: hci_ldisc: Add protocol check to hci_uart_dequeue() Dean Jenkins
2017-03-28 17:50 ` [RFC V1 07/16] Bluetooth: hci_ldisc: Add protocol check to hci_uart_tx_wakeup() Dean Jenkins
2017-03-30 10:11   ` Marcel Holtmann
2017-03-28 17:50 ` [RFC V1 08/16] Bluetooth: hci_ldisc: Separate flag handling in hci_uart_tty_close() Dean Jenkins
2017-03-28 17:50 ` [RFC V1 09/16] Bluetooth: hci_ldisc: Tidy-up HCI_UART_REGISTERED " Dean Jenkins
2017-03-28 17:50 ` [RFC V1 10/16] Bluetooth: hci_ldisc: hci_uart_tty_close() detach tty after last msg Dean Jenkins
2017-03-28 17:50 ` [RFC V1 11/16] Bluetooth: hci_ldisc: hci_uart_tty_close() move hci_uart_close() Dean Jenkins
2017-03-28 17:50 ` [RFC V1 12/16] Bluetooth: hci_ldisc: hci_uart_tty_close() move cancel_work_sync() Dean Jenkins
2017-03-28 17:50 ` [RFC V1 13/16] Bluetooth: hci_ldisc: hci_uart_tty_close() free hu->tx_skb Dean Jenkins
2017-03-28 17:50 ` [RFC V1 14/16] Bluetooth: hci_ldisc: Simplify flushing Dean Jenkins
2017-03-28 17:50 ` [RFC V1 15/16] Bluetooth: hci_ldisc: Use rwlocking to avoid closing proto races Dean Jenkins
2017-03-28 17:50 ` [RFC V1 16/16] Bluetooth: hci_ldisc: Add ioctl_mutex avoiding concurrency Dean Jenkins
2017-03-30 10:11 ` [RFC V1 00/16] hci_ldisc hci_uart_tty_close() fixes Marcel Holtmann
2017-04-03 15:09   ` Dean Jenkins
2017-04-03 15:51     ` Marcel Holtmann
2017-04-04 20:36       ` Dean Jenkins
2017-04-05 15:28         ` Dean Jenkins
2017-04-06  7:23           ` Marcel Holtmann

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=1490723429-28870-1-git-send-email-Dean_Jenkins@mentor.com \
    --to=dean_jenkins@mentor.com \
    --cc=gustavo@padovan.org \
    --cc=johan.hedberg@gmail.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --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.