netdev - Re: [PATCH - RESEND] Bluetooth: Keep master role when SCO or eSCO is active

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: <1C876A95-8077-4BA9-9A16-8AEB9C6AF117@holtmann.org>
Date:	Fri, 27 Jun 2014 11:41:15 +0200
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Kiran Kumar Raparthy <kiran.kumar@...aro.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	"hyungseoung.yoo" <hyungseoung.yoo@...sung.com>,
	"Gustavo F. Padovan" <gustavo@...ovan.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	linux-bluetooth@...r.kernel.org, netdev@...r.kernel.org,
	Android Kernel Team <kernel-team@...roid.com>,
	John Stultz <john.stultz@...aro.org>,
	Jaikumar Ganesh <jaikumarg@...roid.com>
Subject: Re: [PATCH - RESEND] Bluetooth: Keep master role when SCO or eSCO is active

Hi Kiran,

> Preserve the master role when SCO or eSCO is active
> as this improves compatability with lots of
> headset and chipset combinations.

this comment is pretty much non-sense. If you have an incoming BR/EDR connection, then by default you will be slave. The acceptor is the slave, there is no way of preserving the master role. You never had it in the first place.

So lets assume you have an existing connection with a different device, then yes it could be beneficial to not enter a scatternet situation. Assume you are already master in the existing connection, then sure, staying master here is beneficial since you don't want to maintain a second connection and you want to drive the piconet. However what happens when you are slave in the existing connection. Are you forcing now master role on the new connection or better stay slave. Or check if you might be able to become master for both devices and thus move them into a single piconet.

> This is one of the number of patches from the Android AOSP
> common.git tree, which is used on almost all Android devices.
> It looks like it would improve support for compatibility with
> lot of headset,so I wanted to submit it for review to see
> if it should go upstream.
> 
> v3: Fixed formatting issues pointed by Sergei
> 
> Cc: Marcel Holtmann <marcel@...tmann.org>
> Cc: Gustavo Padovan <gustavo@...ovan.org>
> Cc: Johan Hedberg <johan.hedberg@...il.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: linux-bluetooth@...r.kernel.org
> Cc: netdev@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: Android Kernel Team <kernel-team@...roid.com>
> Cc: John Stultz <john.stultz@...aro.org>
> Signed-off-by: hyungseoung.yoo <hyungseoung.yoo@...sung.com>
> Signed-off-by: Jaikumar Ganesh <jaikumarg@...roid.com>
> [kiran: Added context to commit message]
> Signed-off-by: Kiran Raparthy <kiran.kumar@...aro.org>

I am pretty sick of just blasting patches to everybody on the planet. Patches concerning Bluetooth subsystem should be send to linux-bluetooth@...r.kernel.org only. There is no point in spamming everybody else.

> ---
> net/bluetooth/hci_event.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 15010a2..cfb1355 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1915,6 +1915,14 @@ unlock:
> 	hci_conn_check_pending(hdev);
> }
> 
> +static inline bool is_sco_active(struct hci_dev *hdev)
> +{
> +	if (hci_conn_hash_lookup_state(hdev, SCO_LINK, BT_CONNECTED) ||
> +	    hci_conn_hash_lookup_state(hdev, ESCO_LINK, BT_CONNECTED))
> +		return true;
> +	return false;
> +}
> +
> static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	struct hci_ev_conn_request *ev = (void *) skb->data;
> @@ -1961,7 +1969,9 @@ static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 			bacpy(&cp.bdaddr, &ev->bdaddr);
> 
> -			if (lmp_rswitch_capable(hdev) && (mask & HCI_LM_MASTER))
> +			if (lmp_rswitch_capable(hdev) &&
> +			    ((mask & HCI_LM_MASTER) ||
> +			    is_sco_active(hdev)))
> 				cp.role = 0x00; /* Become master */
> 			else
> 				cp.role = 0x01; /* Remain slave */

So this is a policy that takes global affect and might not be the what all devices want to do. The problem now is that acceptors force slave role on the initiator. It might be the case that the initiator can not facilitate the slave role here. And that results in connection failures with certain devices. Not a good idea.

Where I am standing this should be a policy that the SCO sockets that are active can enforce. So on a SCO socket you might set that you want to stay master. And that policy, if set, then gets enforced here. That allows other profiles to pick whatever policy they want.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html