| Message ID | 20200414115512.1.I9dd050ead919f2cc3ef83d4e866de537c7799cf3@changeid |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | Bluetooth: Terminate the link if pairing is cancelled | expand |
Hi Manish, > If user decides to cancel ongoing pairing process (e.g. by clicking > the cancel button on the pairing/passkey window), abort any ongoing > pairing and then terminate the link. > > Signed-off-by: Manish Mandlik <mmandlik@google.com> > --- > Hello Linux-Bluetooth, > > This patch aborts any ongoing pairing and then terminates the link > by calling hci_abort_conn() in cancel_pair_device() function. > > However, I'm not very sure if hci_abort_conn() should be called here > in cancel_pair_device() or in smp for example to terminate the link > after it had sent the pairing failed PDU. > > Please share your thoughts on this. I am look into this. Just bare with me for a bit to verify the call chain. Regards Marcel
Hi Manish, Marcel, On Tue, Apr 28, 2020 at 2:38 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Manish, > > > If user decides to cancel ongoing pairing process (e.g. by clicking > > the cancel button on the pairing/passkey window), abort any ongoing > > pairing and then terminate the link. > > > > Signed-off-by: Manish Mandlik <mmandlik@google.com> > > --- > > Hello Linux-Bluetooth, > > > > This patch aborts any ongoing pairing and then terminates the link > > by calling hci_abort_conn() in cancel_pair_device() function. > > > > However, I'm not very sure if hci_abort_conn() should be called here > > in cancel_pair_device() or in smp for example to terminate the link > > after it had sent the pairing failed PDU. > > Id recommend leaving the hci_abort_conn out since that is a policy decision the userspace should be in charge to decide if the link should be disconnected or not. > > Please share your thoughts on this. > > I am look into this. Just bare with me for a bit to verify the call chain. > > Regards > > Marcel >
Hi Luiz, >>> If user decides to cancel ongoing pairing process (e.g. by clicking >>> the cancel button on the pairing/passkey window), abort any ongoing >>> pairing and then terminate the link. >>> >>> Signed-off-by: Manish Mandlik <mmandlik@google.com> >>> --- >>> Hello Linux-Bluetooth, >>> >>> This patch aborts any ongoing pairing and then terminates the link >>> by calling hci_abort_conn() in cancel_pair_device() function. >>> >>> However, I'm not very sure if hci_abort_conn() should be called here >>> in cancel_pair_device() or in smp for example to terminate the link >>> after it had sent the pairing failed PDU. >>> > > Id recommend leaving the hci_abort_conn out since that is a policy > decision the userspace should be in charge to decide if the link > should be disconnected or not. eventually the link will disconnect anyway if we have no users. However maybe we should try to track if we created the link because Pair Device action. If created the link, then aborting the pairing should disconnect the link right away. Otherwise we can leave it around. Regards Marcel
Hi Manish, > Based on your feedback, in the BlueZ kernel, if we plan to track whether the link was created because of Pair Device action or not, we'll need to add a flag in struch hci_conn and update related functions/APIs. I was wondering if this would look like a clean fix or not. > > Another option could be disconnecting from BlueZ daemon while handling 'cancel pairing' user request. But the problem with this approach is that there is no way to request the kernel to send SMP failure PDU with the existing implementation. > > Third option could be handling this in the chromium and requesting a disconnect when the user hits the cancel button. I believe Ubuntu/Android are taking a similar approach. However, on Android, if the 'cancel' button is selected on the pairing window, it shows 'pairing failed because of invalid passkey' message. > > Bluetooth specification doesn't have any mention about how to handle the pairing cancel case. Based on the statistics we have for ChromeOS, over 60% pairing attempts are cancelled by users. Since the link is not terminated, the bluetooth keyboard keeps on requesting to enter a new passkey even if the user selects to cancel the pairing and there is no way to cancel the pairing process. > > Can you please help me select the better approach to handle the pairing cancel case? Should we need to propose this to be addressed in the Bluetooth Specification as well? are we sending Cancel Pairing correctly? If so and we only care about the cancel case, then I would just track if the connection was triggered by a pairing and then only cancel pairing terminate the connection. Regards Marcel
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 6552003a170eb..1aaa44282af4f 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -3030,6 +3030,18 @@ static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data, err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_CANCEL_PAIR_DEVICE, 0, addr, sizeof(*addr)); + + /* Since user doesn't want to proceed with the connection, + * abort any ongoing pairing and then terminate the link. + */ + if (addr->type == BDADDR_BREDR) + hci_remove_link_key(hdev, &addr->bdaddr); + else + smp_cancel_and_remove_pairing(hdev, &addr->bdaddr, + le_addr_type(addr->type)); + + hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM); + unlock: hci_dev_unlock(hdev); return err;
If user decides to cancel ongoing pairing process (e.g. by clicking the cancel button on the pairing/passkey window), abort any ongoing pairing and then terminate the link. Signed-off-by: Manish Mandlik <mmandlik@google.com> --- Hello Linux-Bluetooth, This patch aborts any ongoing pairing and then terminates the link by calling hci_abort_conn() in cancel_pair_device() function. However, I'm not very sure if hci_abort_conn() should be called here in cancel_pair_device() or in smp for example to terminate the link after it had sent the pairing failed PDU. Please share your thoughts on this. Thanks and regards, Manish. net/bluetooth/mgmt.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)