* Re: Kernel panic in rfcomm_run - unbalanced refcount on rfcomm_session
From: Ville Tervo @ 2010-02-18 7:15 UTC (permalink / raw)
To: ext Nick Pelly; +Cc: Bluettooth Linux
In-Reply-To: <35c90d961002172104q3af1ca8p850004f8b93e8af7@mail.gmail.com>
Hi nick,
ext Nick Pelly wrote:
> Since 2.6.32 we are seeing kernel panics like:
>
> [10651.110229] Unable to handle kernel paging request at virtual
> address 6b6b6b6b
> [10651.111968] Internal error: Oops: 5 [#1] PREEMPT
> [10651.113952] CPU: 0 Tainted: G W (2.6.32-59979-gd0c97db #1)
> [10651.114624] PC is at rfcomm_run+0xa04/0xdbc
> <...>
> [10651.406188] [<c031ad24>] (rfcomm_run+0xa04/0xdbc) from [<c006ce30>]
> (kthread+0x78/0x80)
> [10651.406585] [<c006ce30>] (kthread+0x78/0x80) from [<c002793c>]
> (kernel_thread_exit+0x0/0x8)
>
> (rfcomm_run() is all inlined so theres not much of a stack trace))
>
> This is a use-after-free on struct rfcomm_session s in the call chain
> rfcomm_run() -> rfcomm_process_sessions() -> rfcomm_process_dlcs() ->
> list_for_each_safe(p, n, &s->dlcs). The only way this can happen is if
> there is an unbalanced refcount on the rfcomm session.
>
I have seen same traces.
> We found that reverting the patch
> 9e726b17422bade75fba94e625cd35fd1353e682 "Bluetooth: Fix rejected
> connection not disconnecting ACL link" fixes the issue for us. The
> patch itself looks ok, I added some logging to check the new refcounts
> in the patch are balanced and they are. However if I remove the new
> calls to rfcomm_session_put() and rfcomm_session_hold() the crash is
> resolved. I also found that we can crash without hitting
> rfcomm_session_timeout(), so its not related to Marcel's recent patch
> to remove the scheduling-while-atomic warning.
>
> 9e726b17422bade75fba94e625cd35fd1353e682 does lead to a delay in
> calling rfcomm_session_del() due to the extra refcount while waiting
> for the new timeout. I believe that this delay has revealed some more
> subtle problem elsewhere that causes an unbalanced refcount and then
> the kernel panic.
>
> I have debug kernel logs and hci logs - they are too large to send to
> the list but I can send them directly to anyone interested in
> debugging.
>
> We see this crash frequently with a number of headsets since 2.6.32,
> but not reliably. I do have a 100% repro case with the Nuvi Garmin,
> with these exact steps:
> 1) Make sure Nuvi is unpaired, Bluez stack is unpaired, and kernel has
> been rebooted since unpairing.
> 2) Initiate device discovery, pairing, and handsfree connection from Nuvi
> 3) Observe HFP rfcomm connect briefly, then disconnect, and kernel panic
>
Some OBEX cases also trigger this same problem. I don't have exact steps
right now. I'll try to dig some more information.
--
Ville
^ permalink raw reply
* [bluetooth-next 1/1] bluetooth: handle device reset event
From: Tomas Winkler @ 2010-02-18 9:40 UTC (permalink / raw)
To: marcel, linux-bluetooth; +Cc: guy.cohen, ron.rindjunsky, Gregory Paskar
From: Gregory Paskar <greogry.paskar@intel.com>
A bluetooth device experiencing hardware failure may issue
a HARDWARE_ERROR hci event. The reaction to this event is device
reset flow implemented in following sequence.
1. Notify: HCI_DEV_DOWN
2. Reinitialize internal structures.
3. Call driver flush function
4. Send HCI reset request to the device.
5. Send HCI init sequence reset to the device.
6. Notify HCI_DEV_UP.
Signed-off-by: Gregory Paskar <greogry.paskar@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
include/net/bluetooth/hci.h | 5 ++++
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_core.c | 40 +++++++++++++++++++++++++++++++++++++-
net/bluetooth/hci_event.c | 3 ++
4 files changed, 49 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ed3aea1..cd23eb4 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -691,6 +691,11 @@ struct hci_ev_cmd_status {
__le16 opcode;
} __attribute__ ((packed));
+#define HCI_EV_HARDWARE_ERROR 0x10
+struct hci_ev_hw_error {
+ __u8 hw_code;
+} __attribute__ ((packed));
+
#define HCI_EV_ROLE_CHANGE 0x12
struct hci_ev_role_change {
__u8 status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 7b86094..d2b1cba 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -110,6 +110,8 @@ struct hci_dev {
struct tasklet_struct rx_task;
struct tasklet_struct tx_task;
+ struct work_struct hw_err_work;
+
struct sk_buff_head rx_q;
struct sk_buff_head raw_q;
struct sk_buff_head cmd_q;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 94ba349..6002439 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -834,6 +834,44 @@ static int hci_rfkill_set_block(void *data, bool blocked)
return 0;
}
+static void hci_device_hw_error(struct hci_dev *hdev)
+{
+ hci_req_lock(hdev);
+ clear_bit(HCI_UP, &hdev->flags);
+ hci_notify(hdev, HCI_DEV_DOWN);
+ tasklet_disable(&hdev->tx_task);
+ skb_queue_purge(&hdev->rx_q);
+ skb_queue_purge(&hdev->cmd_q);
+ hci_dev_lock_bh(hdev);
+ inquiry_cache_flush(hdev);
+ hci_conn_hash_flush(hdev);
+ hci_dev_unlock_bh(hdev);
+ if (hdev->flush)
+ hdev->flush(hdev);
+ atomic_set(&hdev->cmd_cnt, 1);
+ hdev->acl_cnt = 0;
+ hdev->sco_cnt = 0;
+ hci_reset_req(hdev, 0);
+ atomic_set(&hdev->cmd_cnt, 1);
+ hci_init_req(hdev, 0);
+ tasklet_enable(&hdev->tx_task);
+ set_bit(HCI_UP, &hdev->flags);
+ hci_notify(hdev, HCI_DEV_UP);
+ hci_req_unlock(hdev);
+}
+
+
+static void hci_hw_err_work(struct work_struct *ws)
+{
+ struct hci_dev *hdev;
+ hdev = container_of(ws, struct hci_dev, hw_err_work);
+ if (!hdev) {
+ BT_DBG("%s: hci_hw_error_worker: hdev = NULL ", hdev->name);
+ return;
+ }
+ hci_device_hw_error(hdev);
+}
+
static const struct rfkill_ops hci_rfkill_ops = {
.set_block = hci_rfkill_set_block,
};
@@ -903,7 +941,7 @@ int hci_register_dev(struct hci_dev *hdev)
tasklet_init(&hdev->cmd_task, hci_cmd_task,(unsigned long) hdev);
tasklet_init(&hdev->rx_task, hci_rx_task, (unsigned long) hdev);
tasklet_init(&hdev->tx_task, hci_tx_task, (unsigned long) hdev);
-
+ INIT_WORK(&hdev->hw_err_work, hci_hw_err_work);
skb_queue_head_init(&hdev->rx_q);
skb_queue_head_init(&hdev->cmd_q);
skb_queue_head_init(&hdev->raw_q);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 28517ba..e8f48cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1953,6 +1953,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
case HCI_EV_REMOTE_HOST_FEATURES:
hci_remote_host_features_evt(hdev, skb);
break;
+ case HCI_EV_HARDWARE_ERROR:
+ schedule_work(&hdev->hw_err_work);
+ break;
default:
BT_DBG("%s event 0x%x", hdev->name, event);
--
1.6.0.6
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
^ permalink raw reply related
* Re: [Patch 0/2] Resend: Fix hcidump compilation with bluez-4.61+
From: Stefan Seyfried @ 2010-02-18 10:12 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Marcel Holtmann
In-Reply-To: <1266410982-26964-1-git-send-email-stefan.seyfried@googlemail.com>
Hi Marcel,
On Wed, 17 Feb 2010 13:49:40 +0100
Stefan Seyfried <stefan.seyfried@googlemail.com> wrote:
> Bluez 4.61 broke compilation of hcidump.
> The first patch fixes this for bluez 4.61, the second patch implements an
> (admittedly ugly) hack to also allow to compile hcidump with older bluez
> versions.
Is there anything grossly wrong with these patches (apart from the
admitted ugliness ;) so they get ignored or do I have an email problem
with git-send-email on my side?
Thanks,
seife
--
Stefan Seyfried
"Any ideas, John?"
"Well, surrounding them's out."
^ permalink raw reply
* Re: [PATCH] Always use "nice abort" in osso-gwobex
From: Bastien Nocera @ 2010-02-18 10:56 UTC (permalink / raw)
To: Bastien Nocera; +Cc: BlueZ development
In-Reply-To: <1266455391.678.1742.camel@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 285 bytes --]
On Thu, 2010-02-18 at 01:09 +0000, Bastien Nocera wrote:
> Heya,
>
> As discussed on IRC, we should always use the "nice abort" of openobex,
> rather than disconnecting from device when cancelling transfers.
Updated patch for that, forgot to remove some bits in configure.ac
Cheers
[-- Attachment #2: 0002-Always-use-nice-abort.patch --]
[-- Type: text/x-patch, Size: 2157 bytes --]
>From 4812cec279691aa057540abad443d2ccf9aff6ea Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Thu, 18 Feb 2010 01:05:37 +0000
Subject: [PATCH 2/2] Always use "nice abort"
So we don't disconnect when cancelling a request.
---
configure.ac | 5 -----
src/obex-xfer.c | 8 --------
2 files changed, 0 insertions(+), 13 deletions(-)
diff --git a/configure.ac b/configure.ac
index 9d2c19f..a915ec7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -38,7 +38,6 @@ AC_SUBST(LDFLAGS)
AC_ARG_ENABLE(docs, [ --enable-docs build DOXYGEN documentation (requires Doxygen)],enable_docs=$enableval,enable_docs=auto)
AC_ARG_ENABLE(tracing, [ --enable-tracing Enable debuging information],enable_tracing=$enableval,enable_tracing=no)
-AC_ARG_ENABLE(niceabort, [ --enable-niceabort Enable proper abort],enable_niceabort=$enableval,enable_niceabort=no)
AC_ARG_ENABLE(localtime, [ --enable-localtime Send time as local instead of UTC],enable_localtime=$enableval,enable_localtime=no)
AC_ARG_ENABLE(coverage, [ --enable-coverage Enable coverage], enable_coverage=$enableval, enable_coverage=no)
@@ -98,10 +97,6 @@ else
AC_MSG_RESULT(no)
fi
-if (test x$enable_niceabort = xyes); then
- AC_DEFINE(USE_NICE_ABORT, 1, [Enable nice abort support.])
-fi
-
if (test x$enable_localtime = xyes); then
AC_DEFINE(USE_LOCALTIME, 1, [Send time as local instead of UTC.])
fi
diff --git a/src/obex-xfer.c b/src/obex-xfer.c
index 38a2f4f..81ac5dc 100644
--- a/src/obex-xfer.c
+++ b/src/obex-xfer.c
@@ -98,18 +98,10 @@ gboolean gw_obex_xfer_do_abort(struct gw_obex_xfer *xfer) {
xfer->abort = TRUE;
-#ifdef USE_NICE_ABORT
debug("Performing nice abort\n");
if (OBEX_CancelRequest(xfer->ctx->handle, TRUE) != 0)
return FALSE;
return TRUE;
-#else
- debug("Performing abort through disconnection (without ABORT command)\n");
- xfer->ctx->done = TRUE;
- OBEX_CancelRequest(xfer->ctx->handle, FALSE);
- obex_link_error(xfer->ctx);
- return FALSE;
-#endif
}
GwObexXfer *gw_obex_put_async(GwObex *ctx, const char *name, const char *type,
--
1.6.6.1
^ permalink raw reply related
* [PATCH] Mark inuse flag whenever a stream is configured
From: Luiz Augusto von Dentz @ 2010-02-18 14:04 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
Currently it was only marked when acting as the initiator.
---
audio/avdtp.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/audio/avdtp.c b/audio/avdtp.c
index efed9b6..2591845 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1426,6 +1426,7 @@ static gboolean avdtp_setconf_cmd(struct avdtp
*session, uint8_t transaction,
}
sep->stream = stream;
+ sep->info.inuse = 1;
session->streams = g_slist_append(session->streams, stream);
avdtp_sep_set_state(session, sep, AVDTP_STATE_CONFIGURED);
--
1.6.3.3
--
Luiz Augusto von Dentz
Computer Engineer
[-- Attachment #2: 0001-Mark-inuse-flag-whenever-a-stream-is-configured.patch --]
[-- Type: text/x-patch, Size: 774 bytes --]
From 22ea50f266692225dfc3836b90ced4514e2de16c Mon Sep 17 00:00:00 2001
From: Luiz Augusto Von Dentz <luiz.dentz-von@nokia.com>
Date: Thu, 18 Feb 2010 13:21:21 +0200
Subject: [PATCH] Mark inuse flag whenever a stream is configured
Currently it was only marked when acting as the initiator.
---
audio/avdtp.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/audio/avdtp.c b/audio/avdtp.c
index efed9b6..2591845 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -1426,6 +1426,7 @@ static gboolean avdtp_setconf_cmd(struct avdtp *session, uint8_t transaction,
}
sep->stream = stream;
+ sep->info.inuse = 1;
session->streams = g_slist_append(session->streams, stream);
avdtp_sep_set_state(session, sep, AVDTP_STATE_CONFIGURED);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Fix kernel crash on BT stress tests.
From: Andrei Emeltchenko @ 2010-02-18 16:18 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20100217235755.GA6011@vigoh>
Hi Gustavo
On Thu, Feb 18, 2010 at 1:57 AM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 13:58:34 +0200]:
>
>> On Tue, Feb 16, 2010 at 1:34 PM, Gustavo F. Padovan
>> <padovan@profusion.mobi> wrote:
>> > Hi Andrei,
>> >
>> > * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-02-16 12:36:47 +0200]:
>> >
>> >> From 0135f732cb45e5e91062aca84a61a40b172200a4 Mon Sep 17 00:00:00 2001
>> >> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> >> Date: Tue, 16 Feb 2010 10:52:33 +0200
>> >> Subject: [PATCH] Bluetooth: Fix kernel crash on BT stress tests.
>> >>
>> >> Added very simple check that req buffer has enough space to
>> >> fit configuration parameters. Shall be enough to reject packets
>> >> with configuration size more than req buffer.
>> >>
>> >> Crash trace below
>> >>
>> >> [ 6069.659393] Unable to handle kernel paging request at virtual
>> >> address 02000205
>> >> [ 6069.673034] Internal error: Oops: 805 [#1] PREEMPT
>> >> ...
>> >> [ 6069.727172] PC is at l2cap_add_conf_opt+0x70/0xf0 [l2cap]
>> >> [ 6069.732604] LR is at l2cap_recv_frame+0x1350/0x2e78 [l2cap]
>> >> ...
>> >> [ 6070.030303] Backtrace:
>> >> [ 6070.032806] [<bf1c2880>] (l2cap_add_conf_opt+0x0/0xf0 [l2cap]) from
>> >> [<bf1c6624>] (l2cap_recv_frame+0x1350/0x2e78 [l2cap])
>> >> [ 6070.043823] r8:dc5d3100 r7:df2a91d6 r6:00000001 r5:df2a8000 r4:00000200
>> >> [ 6070.050659] [<bf1c52d4>] (l2cap_recv_frame+0x0/0x2e78 [l2cap]) from
>> >> [<bf1c8408>] (l2cap_recv_acldata+0x2bc/0x350 [l2cap])
>> >> [ 6070.061798] [<bf1c814c>] (l2cap_recv_acldata+0x0/0x350 [l2cap]) from
>> >> [<bf0037a4>] (hci_rx_task+0x244/0x478 [bluetooth])
>> >> [ 6070.072631] r6:dc647700 r5:00000001 r4:df2ab740
>> >> [ 6070.077362] [<bf003560>] (hci_rx_task+0x0/0x478 [bluetooth]) from
>> >> [<c006b9fc>] (tasklet_action+0x78/0xd8)
>> >> [ 6070.087005] [<c006b984>] (tasklet_action+0x0/0xd8) from [<c006c160>]
>> >
>> > Are you using ERTM or Streaming mode? If yes, I have a guess about the
>> > source of the problem. On l2cap_parse_conf_rsp we check for:
>> >
>> > while (len >= L2CAP_CONF_OPT_SIZE) {
>> > len -= l2cap_get_conf_opt(&rsp, &type, &olen, &val);
>> >
>> > But on case L2CAP_CONF_RFC olen is greater than L2CAP_CONF_OPT_SIZE we
>> > can exceed the buffer size. So the right fix will be check if len >=
>> > olen in that case.
Is len always bigger then olen?
in l2cap_get_conf_opt :
len = L2CAP_CONF_OPT_SIZE + opt->len;
opt->len is olen
-- Andrei
>>
>> We use test tool which sends "Configure Response" packet with size 262
>> bytes. So "req" buffer gets overwritten. But in the code nobody checks
>> that "req" might be overwritten.
>
> So I have found another issue. ;)
> Have to fix it.
>
>>
>> -- Andrei
>>
>> >
>> >
>> >>
>> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> >> ---
>> >> net/bluetooth/l2cap.c | 6 ++++++
>> >> 1 files changed, 6 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> >> index 400efa2..69b7280 100644
>> >> --- a/net/bluetooth/l2cap.c
>> >> +++ b/net/bluetooth/l2cap.c
>> >> @@ -2830,6 +2830,12 @@ static inline int l2cap_config_rsp(struct
>> >> l2cap_conn *conn, struct l2cap_cmd_hdr
>> >> int len = cmd->len - sizeof(*rsp);
>> >> char req[64];
>> >>
>> >> + if (len > sizeof(req) - sizeof(struct l2cap_conf_req)) {
>> >> + BT_ERR("Config response is too big");
>> >> + l2cap_send_disconn_req(conn, sk);
>> >> + goto done;
>> >> + }
>> >> +
>> >> /* throw out any old stored conf requests */
>> >> result = L2CAP_CONF_SUCCESS;
>> >> len = l2cap_parse_conf_rsp(sk, rsp->data,
>> >> --
>> >> 1.6.0.4
>> >
>> >
>> > --
>> > Gustavo F. Padovan
>> > http://padovan.org
>> >
>> > ProFUSION embedded systems - http://profusion.mobi
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Gustavo F. Padovan
> http://padovan.org
>
> ProFUSION embedded systems - http://profusion.mobi
>
^ permalink raw reply
* Re: [PATCH] Always use "nice abort" in osso-gwobex
From: Johan Hedberg @ 2010-02-18 17:06 UTC (permalink / raw)
To: Bastien Nocera; +Cc: BlueZ development
In-Reply-To: <1266490576.678.2975.camel@localhost.localdomain>
Hi Bastien,
On Thu, Feb 18, 2010, Bastien Nocera wrote:
> On Thu, 2010-02-18 at 01:09 +0000, Bastien Nocera wrote:
> > Heya,
> >
> > As discussed on IRC, we should always use the "nice abort" of openobex,
> > rather than disconnecting from device when cancelling transfers.
>
> Updated patch for that, forgot to remove some bits in configure.ac
Both patches have been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Mark inuse flag whenever a stream is configured
From: Johan Hedberg @ 2010-02-18 17:09 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <2d5a2c101002180604u7cecd3ecsa3a047c3c28bcf61@mail.gmail.com>
Hi Luiz,
On Thu, Feb 18, 2010, Luiz Augusto von Dentz wrote:
> Currently it was only marked when acting as the initiator.
> ---
> audio/avdtp.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/audio/avdtp.c b/audio/avdtp.c
> index efed9b6..2591845 100644
> --- a/audio/avdtp.c
> +++ b/audio/avdtp.c
> @@ -1426,6 +1426,7 @@ static gboolean avdtp_setconf_cmd(struct avdtp
> *session, uint8_t transaction,
> }
>
> sep->stream = stream;
> + sep->info.inuse = 1;
> session->streams = g_slist_append(session->streams, stream);
>
> avdtp_sep_set_state(session, sep, AVDTP_STATE_CONFIGURED);
The patch has been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Punch a hole for HandsfreeAgent in dbus policy
From: Johan Hedberg @ 2010-02-18 17:12 UTC (permalink / raw)
To: Denis Kenzior; +Cc: linux-bluetooth, Denis Kenzior
In-Reply-To: <1266026813-29676-2-git-send-email-denis.kenzior@intel.com>
Hi Denis,
On Fri, Feb 12, 2010, Denis Kenzior wrote:
> ---
> src/bluetooth.conf | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/src/bluetooth.conf b/src/bluetooth.conf
> index 315009c..56e7a83 100644
> --- a/src/bluetooth.conf
> +++ b/src/bluetooth.conf
> @@ -11,6 +11,7 @@
> <allow own="org.bluez"/>
> <allow send_destination="org.bluez"/>
> <allow send_interface="org.bluez.Agent"/>
> + <allow send_interface="org.bluez.HandsfreeAgent"/>
> </policy>
The patch is now upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Bluetooth: Allow SCO/eSCO packet type selection for outgoing SCO connections.
From: Nick Pelly @ 2010-02-18 23:47 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Ville Tervo, linux-bluetooth
In-Reply-To: <1266427894.8849.66.camel@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 3332 bytes --]
On Wed, Feb 17, 2010 at 9:31 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Nick,
>
>> >> As a first step, can we get consensus on the userspace API:
>> >>
>> >> --- a/include/net/bluetooth/hci.h
>> >> +++ b/include/net/bluetooth/hci.h
>> >> @@ -139,8 +139,11 @@ enum {
>> >> +#define ALL_SCO_PKTS (SCO_ESCO_MASK | EDR_ESCO_MASK)
>> >> +#define ALL_ESCO_PKTS (SCO_ESCO_MASK | ESCO_EV3 | ESCO_EV4 | ESCO_EV5)
>> >>
>> >> --- a/include/net/bluetooth/sco.h
>> >> +++ b/include/net/bluetooth/sco.h
>> >> @@ -37,6 +37,7 @@
>> >> struct sockaddr_sco {
>> >> sa_family_t sco_family;
>> >> bdaddr_t sco_bdaddr;
>> >> + __u16 sco_pkt_type;
>> >> };
>> >>
>> >> This will at least unblock my work.
>> >
>> > Would it be better to add new sockopt for sco socket?
>>
>> What advantage does that have?
>>
>> Putting it in struct sockaddr_sco seems to make more sense since
>> packet types can only be selected during SCO connection establishment.
>> They can't be changed once the socket is connected.
>
> in theory you can change the allowed packet types for ACL, SCO and eSCO
> after the connection is active. However the usefulness here is fairly
> limited. In case of ACL it is purely academical and most link manager
> will just ignore you. Mainly because the host stack can't really make a
> good decision here anyway.
>
> Personally I think it is a total brain dead concept to give this into
> the control of the host stack anyway. For eSCO packet types this make a
> bit more sense since you might wanna control the bandwidth. However
> changing them later is just pointless. And I don't recall of any profile
> actually mentioning about it. I think they had a great idea behind eSCO
> support, but since it is impossible to get the negotiation parts right,
> everybody sticks with simple eSCO channels and doesn't bother to change
> them.
>
> And even if we would be going for a setsockopt(), that would be blocking
> and then again pretty much pointless API. The sockaddr is most logical
> thing that fits into what we wanna achieve. Disallow/allow certain
> packet types and essentially force SCO over eSCO.
Ok seems like there is agreement on using struct sockaddr_sco for sco_pkt_type.
A more controversial question is whether to follow the SIG convention
of reversing the logic on the EDR bits in the packet type mask (1
means do *not* use this packet for the EDR bits), or to use consistent
logic for each bit (1 always means *allow* this packet type) for
packet selection in scokaddr_sco.sco_pkt_type.
The original patch I posted followed the SIG convention. However after
more thinking I am leaning towards using consistent logic for each
bit. 1 will always mean 'allow this packet'. My rationale is that the
most common use for sco_pkt_type will be to request only SCO packet
types allowed. If however the SIG adds more reverse logic packet
types, and we follow the SIG convention, then old userspace code that
just requested the SCO packet types will now end up with the new
packet types as well. So I think it is best to avoid this situation
and for 1 to always mean 'allow this packet' in
sockaddr_sco.sco_pkt_type.
Attached is a new patch with the consistent bit logic.
Comments?
Cheers,
Nick
[-- Attachment #2: 0001-Bluetooth-Allow-SCO-eSCO-packet-type-selection-for-o.patch --]
[-- Type: application/octet-stream, Size: 12308 bytes --]
From 6efa97b316cf3f8897a64a739ea221879dc134dd Mon Sep 17 00:00:00 2001
From: Nick Pelly <npelly@google.com>
Date: Thu, 11 Feb 2010 11:54:28 -0800
Subject: [PATCH] Bluetooth: Allow SCO/eSCO packet type selection for outgoing SCO connections.
__u16 sco_pkt_type is introduced to struct sockaddr_sco. It allows bitwise
selection of SCO/eSCO packet types. Currently those bits are:
0x0001 HV1 may be used.
0x0002 HV2 may be used.
0x0004 HV3 may be used.
0x0008 EV3 may be used.
0x0010 EV4 may be used.
0x0020 EV5 may be used.
0x0040 2-EV3 may be used.
0x0080 3-EV3 may be used.
0x0100 2-EV5 may be used.
0x0200 3-EV5 may be used.
This is similar to the Packet Type parameter in the HCI Setup Synchronous
Connection Command, except that we are not reversing the logic on the EDR bits.
This makes the use of sco_pkt_tpye forward portable for the use case of
white-listing packet types, which we expect will be the primary use case.
If sco_pkt_type is zero, or userspace uses the old struct sockaddr_sco,
then the default behavior is to allow all packet types.
Packet type selection is just a request made to the Bluetooth chipset, and
it is up to the link manager on the chipset to negiotiate and decide on the
actual packet types used. Furthermore, when a SCO/eSCO connection is eventually
made there is no way for the host stack to determine which packet type was used
(however it is possible to get the link type of SCO or eSCO).
sco_pkt_type is ignored for incoming SCO connections. It is possible
to add this in the future as a parameter to the Accept Synchronous Connection
Command, however its a little trickier because the kernel does not
currently preserve sockaddr_sco data between userspace calls to accept().
The most common use for sco_pkt_type will be to white-list only SCO packets,
which can be done with the hci.h constant SCO_ESCO_MASK.
This patch is motivated by broken Bluetooth carkits such as the Motorolo
HF850 (it claims to support eSCO, but will actually reject eSCO connections
after 5 seconds) and the 2007/2008 Infiniti G35/37 (fails to route audio
if a 2-EV5 packet type is negiotiated). With this patch userspace can maintain
a list of compatible packet types to workaround remote devices such as these.
Based on a patch by Marcel Holtmann.
Signed-off-by: Nick Pelly <npelly@google.com>
---
include/net/bluetooth/hci.h | 6 ++-
include/net/bluetooth/hci_core.h | 7 +++-
include/net/bluetooth/sco.h | 4 ++-
net/bluetooth/hci_conn.c | 33 ++++++++++++++-------
net/bluetooth/hci_event.c | 6 ++-
net/bluetooth/l2cap.c | 2 +-
net/bluetooth/sco.c | 60 +++++++++++++++++++++++++------------
7 files changed, 79 insertions(+), 39 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 4f7795f..b32dcff 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -139,8 +139,10 @@ enum {
#define ESCO_2EV5 0x0100
#define ESCO_3EV5 0x0200
-#define SCO_ESCO_MASK (ESCO_HV1 | ESCO_HV2 | ESCO_HV3)
-#define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
+#define SCO_ESCO_MASK (ESCO_HV1 | ESCO_HV2 | ESCO_HV3)
+#define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
+#define ALL_ESCO_MASK (SCO_ESCO_MASK | ESCO_EV3 | ESCO_EV4 | ESCO_EV5 | \
+ EDR_ESCO_MASK)
/* ACL flags */
#define ACL_START 0x00
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b6d36cb..cbcc5b1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -326,12 +326,15 @@ void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
void hci_add_sco(struct hci_conn *conn, __u16 handle);
void hci_setup_sync(struct hci_conn *conn, __u16 handle);
-struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst);
+struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type,
+ __u16 pkt_type, bdaddr_t *dst);
int hci_conn_del(struct hci_conn *conn);
void hci_conn_hash_flush(struct hci_dev *hdev);
void hci_conn_check_pending(struct hci_dev *hdev);
-struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 sec_level, __u8 auth_type);
+struct hci_conn *hci_connect(struct hci_dev *hdev, int type,
+ __u16 pkt_type, bdaddr_t *dst,
+ __u8 sec_level, __u8 auth_type);
int hci_conn_check_link_mode(struct hci_conn *conn);
int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
int hci_conn_change_link_key(struct hci_conn *conn);
diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
index e28a2a7..924338a 100644
--- a/include/net/bluetooth/sco.h
+++ b/include/net/bluetooth/sco.h
@@ -37,6 +37,7 @@
struct sockaddr_sco {
sa_family_t sco_family;
bdaddr_t sco_bdaddr;
+ __u16 sco_pkt_type;
};
/* SCO socket options */
@@ -72,7 +73,8 @@ struct sco_conn {
struct sco_pinfo {
struct bt_sock bt;
- __u32 flags;
+ __u16 pkt_type;
+
struct sco_conn *conn;
};
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fa8b412..2f4d30f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -196,7 +196,8 @@ static void hci_conn_idle(unsigned long arg)
hci_conn_enter_sniff_mode(conn);
}
-struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
+struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type,
+ __u16 pkt_type, bdaddr_t *dst)
{
struct hci_conn *conn;
@@ -221,14 +222,22 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
conn->pkt_type = hdev->pkt_type & ACL_PTYPE_MASK;
break;
case SCO_LINK:
- if (lmp_esco_capable(hdev))
- conn->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
- (hdev->esco_type & EDR_ESCO_MASK);
- else
- conn->pkt_type = hdev->pkt_type & SCO_PTYPE_MASK;
- break;
+ if (!pkt_type)
+ pkt_type = SCO_ESCO_MASK;
case ESCO_LINK:
- conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK;
+ if (!pkt_type)
+ pkt_type = ALL_ESCO_MASK;
+ if (lmp_esco_capable(hdev)) {
+ /* HCI Setup Synchronous Connection Command uses
+ reverse logic on the EDR_ESCO_MASK bits */
+ conn->pkt_type = (pkt_type ^ EDR_ESCO_MASK) &
+ hdev->esco_type;
+ } else {
+ /* Legacy HCI Add Sco Connection Command uses a
+ shifted bitmask */
+ conn->pkt_type = (pkt_type << 5) & hdev->pkt_type &
+ SCO_PTYPE_MASK;
+ }
break;
}
@@ -340,7 +349,9 @@ EXPORT_SYMBOL(hci_get_route);
/* Create SCO or ACL connection.
* Device _must_ be locked */
-struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8 sec_level, __u8 auth_type)
+struct hci_conn *hci_connect(struct hci_dev *hdev, int type,
+ __u16 pkt_type, bdaddr_t *dst,
+ __u8 sec_level, __u8 auth_type)
{
struct hci_conn *acl;
struct hci_conn *sco;
@@ -348,7 +359,7 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
BT_DBG("%s dst %s", hdev->name, batostr(dst));
if (!(acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst))) {
- if (!(acl = hci_conn_add(hdev, ACL_LINK, dst)))
+ if (!(acl = hci_conn_add(hdev, ACL_LINK, 0, dst)))
return NULL;
}
@@ -364,7 +375,7 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
return acl;
if (!(sco = hci_conn_hash_lookup_ba(hdev, type, dst))) {
- if (!(sco = hci_conn_add(hdev, type, dst))) {
+ if (!(sco = hci_conn_add(hdev, type, pkt_type, dst))) {
hci_conn_put(acl);
return NULL;
}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 10edd1a..5343e0f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -579,7 +579,7 @@ static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
}
} else {
if (!conn) {
- conn = hci_conn_add(hdev, ACL_LINK, &cp->bdaddr);
+ conn = hci_conn_add(hdev, ACL_LINK, 0, &cp->bdaddr);
if (conn) {
conn->out = 1;
conn->link_mode |= HCI_LM_MASTER;
@@ -964,7 +964,9 @@ static inline void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *sk
conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
if (!conn) {
- if (!(conn = hci_conn_add(hdev, ev->link_type, &ev->bdaddr))) {
+ /* pkt_type not yet used for incoming connections */
+ if (!(conn = hci_conn_add(hdev, ev->link_type, 0,
+ &ev->bdaddr))) {
BT_ERR("No memmory for new connection");
hci_dev_unlock(hdev);
return;
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index d056886..b342f06 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -955,7 +955,7 @@ static int l2cap_do_connect(struct sock *sk)
}
}
- hcon = hci_connect(hdev, ACL_LINK, dst,
+ hcon = hci_connect(hdev, ACL_LINK, 0, dst,
l2cap_pi(sk)->sec_level, auth_type);
if (!hcon)
goto done;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 77f4153..754db7a 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -176,6 +176,7 @@ static int sco_connect(struct sock *sk)
{
bdaddr_t *src = &bt_sk(sk)->src;
bdaddr_t *dst = &bt_sk(sk)->dst;
+ __u16 pkt_type = sco_pi(sk)->pkt_type;
struct sco_conn *conn;
struct hci_conn *hcon;
struct hci_dev *hdev;
@@ -192,10 +193,13 @@ static int sco_connect(struct sock *sk)
if (lmp_esco_capable(hdev) && !disable_esco)
type = ESCO_LINK;
- else
+ else {
type = SCO_LINK;
+ pkt_type &= SCO_ESCO_MASK;
+ }
- hcon = hci_connect(hdev, type, dst, BT_SECURITY_LOW, HCI_AT_NO_BONDING);
+ hcon = hci_connect(hdev, type, pkt_type, dst,
+ BT_SECURITY_LOW, HCI_AT_NO_BONDING);
if (!hcon)
goto done;
@@ -451,18 +455,22 @@ static int sco_sock_create(struct net *net, struct socket *sock, int protocol)
return 0;
}
-static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
+static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
{
- struct sockaddr_sco *sa = (struct sockaddr_sco *) addr;
+ struct sockaddr_sco sa;
struct sock *sk = sock->sk;
- bdaddr_t *src = &sa->sco_bdaddr;
- int err = 0;
+ bdaddr_t *src = &sa.sco_bdaddr;
+ int len, err = 0;
- BT_DBG("sk %p %s", sk, batostr(&sa->sco_bdaddr));
+ BT_DBG("sk %p %s", sk, batostr(&sa.sco_bdaddr));
if (!addr || addr->sa_family != AF_BLUETOOTH)
return -EINVAL;
+ memset(&sa, 0, sizeof(sa));
+ len = min_t(unsigned int, sizeof(sa), alen);
+ memcpy(&sa, addr, len);
+
lock_sock(sk);
if (sk->sk_state != BT_OPEN) {
@@ -476,7 +484,8 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
err = -EADDRINUSE;
} else {
/* Save source address */
- bacpy(&bt_sk(sk)->src, &sa->sco_bdaddr);
+ bacpy(&bt_sk(sk)->src, &sa.sco_bdaddr);
+ sco_pi(sk)->pkt_type = sa.sco_pkt_type;
sk->sk_state = BT_BOUND;
}
@@ -489,26 +498,34 @@ done:
static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen, int flags)
{
- struct sockaddr_sco *sa = (struct sockaddr_sco *) addr;
struct sock *sk = sock->sk;
- int err = 0;
-
+ struct sockaddr_sco sa;
+ int len, err = 0;
BT_DBG("sk %p", sk);
- if (addr->sa_family != AF_BLUETOOTH || alen < sizeof(struct sockaddr_sco))
+ if (!addr || addr->sa_family != AF_BLUETOOTH)
return -EINVAL;
- if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
- return -EBADFD;
-
- if (sk->sk_type != SOCK_SEQPACKET)
- return -EINVAL;
+ memset(&sa, 0, sizeof(sa));
+ len = min_t(unsigned int, sizeof(sa), alen);
+ memcpy(&sa, addr, len);
lock_sock(sk);
+ if (sk->sk_type != SOCK_SEQPACKET) {
+ err = -EINVAL;
+ goto done;
+ }
+
+ if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
+ err = -EBADFD;
+ goto done;
+ }
+
/* Set destination address and psm */
- bacpy(&bt_sk(sk)->dst, &sa->sco_bdaddr);
+ bacpy(&bt_sk(sk)->dst, &sa.sco_bdaddr);
+ sco_pi(sk)->pkt_type = sa.sco_pkt_type;
if ((err = sco_connect(sk)))
goto done;
@@ -610,10 +627,13 @@ static int sco_sock_getname(struct socket *sock, struct sockaddr *addr, int *len
addr->sa_family = AF_BLUETOOTH;
*len = sizeof(struct sockaddr_sco);
- if (peer)
+ if (peer) {
bacpy(&sa->sco_bdaddr, &bt_sk(sk)->dst);
- else
+ sa->sco_pkt_type = sco_pi(sk)->pkt_type;
+ } else {
bacpy(&sa->sco_bdaddr, &bt_sk(sk)->src);
+ sa->sco_pkt_type = sco_pi(sk)->pkt_type;
+ }
return 0;
}
--
1.6.5.3
^ permalink raw reply related
* Userspace patches for eSCO/SCO packet selection
From: Nick Pelly @ 2010-02-19 0:10 UTC (permalink / raw)
To: Linux Bluetooth
[-- Attachment #1: Type: text/plain, Size: 293 bytes --]
For hcidump:
0001-Print-pkt_type-when-decoding-Setup-Synchronous-Conne.patch
0002-Print-pkt_type-on-HCI-Accept-Synchrnous-Connection-R.patch
For bluez userspace:
0001-Update-userspace-headers-for-SCO-eSCO-packet-selecti.patch
0002-Add-option-to-change-sco-packet-type-in-scotest.patch
Nick
[-- Attachment #2: 0001-Print-pkt_type-when-decoding-Setup-Synchronous-Conne.patch --]
[-- Type: application/octet-stream, Size: 896 bytes --]
From 0e2012c382c085cb4df31b47681bd2ddb3e4b1ef Mon Sep 17 00:00:00 2001
From: Nick Pelly <npelly@google.com>
Date: Tue, 9 Feb 2010 17:05:02 -0800
Subject: [PATCH 1/2] Print pkt_type when decoding Setup Synchronous Connection command.
---
parser/hci.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/parser/hci.c b/parser/hci.c
index c0ca27e..958cd38 100644
--- a/parser/hci.c
+++ b/parser/hci.c
@@ -917,8 +917,9 @@ static inline void setup_sync_conn_dump(int level, struct frame *frm)
setup_sync_conn_cp *cp = frm->ptr;
p_indent(level, frm);
- printf("handle %d voice setting 0x%4.4x\n", btohs(cp->handle),
- btohs(cp->voice_setting));
+ printf("handle %d voice setting 0x%4.4x pkt_type 0x%4.4x\n",
+ btohs(cp->handle), btohs(cp->voice_setting),
+ btohs(cp->pkt_type));
}
static inline void hold_mode_dump(int level, struct frame *frm)
--
1.6.5.3
[-- Attachment #3: 0002-Print-pkt_type-on-HCI-Accept-Synchrnous-Connection-R.patch --]
[-- Type: application/octet-stream, Size: 1419 bytes --]
From 55f43e604797356468733a7a333bc835e9ab2332 Mon Sep 17 00:00:00 2001
From: Nick Pelly <npelly@google.com>
Date: Thu, 11 Feb 2010 15:26:27 -0800
Subject: [PATCH 2/2] Print pkt_type on HCI Accept Synchrnous Connection Request Command.
---
parser/hci.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/parser/hci.c b/parser/hci.c
index 958cd38..ee4fdb0 100644
--- a/parser/hci.c
+++ b/parser/hci.c
@@ -922,6 +922,17 @@ static inline void setup_sync_conn_dump(int level, struct frame *frm)
btohs(cp->pkt_type));
}
+static inline void accept_sync_conn_req_dump(int level, struct frame *frm)
+{
+ accept_sync_conn_req_cp *cp = frm->ptr;
+ char addr[18];
+
+ p_indent(level, frm);
+ p_ba2str(&cp->bdaddr, addr);
+ printf("bdaddr %s voice_setting 0x%4.4x pkt_type 0x%4.4x\n",
+ addr, btohs(cp->voice_setting), btohs(cp->pkt_type));
+}
+
static inline void hold_mode_dump(int level, struct frame *frm)
{
hold_mode_cp *cp = frm->ptr;
@@ -1385,9 +1396,11 @@ static inline void command_dump(int level, struct frame *frm)
return;
case OCF_CREATE_CONN_CANCEL:
case OCF_REMOTE_NAME_REQ_CANCEL:
- case OCF_ACCEPT_SYNC_CONN_REQ:
bdaddr_command_dump(level + 1, frm);
return;
+ case OCF_ACCEPT_SYNC_CONN_REQ:
+ accept_sync_conn_req_dump(level + 1, frm);
+ return;
case OCF_ADD_SCO:
case OCF_SET_CONN_PTYPE:
add_sco_dump(level + 1, frm);
--
1.6.5.3
[-- Attachment #4: 0001-Update-userspace-headers-for-SCO-eSCO-packet-selecti.patch --]
[-- Type: application/octet-stream, Size: 3689 bytes --]
From 4c246b19b8e6227f897e892faa1abd8af7c619eb Mon Sep 17 00:00:00 2001
From: Nick Pelly <npelly@google.com>
Date: Thu, 18 Feb 2010 15:59:16 -0800
Subject: [PATCH 1/2] Update userspace headers for SCO/eSCO packet selection in struct sockaddr_sco.
uint16_t sco_pkt_type is introduced to struct sockaddr_sco. It allows bitwise
selection of SCO/eSCO packet types. Currently those bits are:
0x0001 HV1 may be used.
0x0002 HV2 may be used.
0x0004 HV3 may be used.
0x0008 EV3 may be used.
0x0010 EV4 may be used.
0x0020 EV5 may be used.
0x0040 2-EV3 may be used.
0x0080 3-EV3 may be used.
0x0100 2-EV5 may be used.
0x0200 3-EV5 may be used.
This is similar to the Packet Type parameter in the HCI Setup Synchronous
Connection Command, except that we are not reversing the logic on the EDR bits.
This makes the use of sco_pkt_tpye forward portable for the use case of
white-listing packet types, which we expect will be the primary use case.
If sco_pkt_type is zero, or userspace uses the old struct sockaddr_sco,
then the default behavior is to allow all packet types.
Packet type selection is just a request made to the Bluetooth chipset, and
it is up to the link manager on the chipset to negiotiate and decide on the
actual packet types used. Furthermore, when a SCO/eSCO connection is eventually
made there is no way for the host stack to determine which packet type was used
(however it is possible to get the link type of SCO or eSCO).
sco_pkt_type is ignored for incoming SCO connections. It is possible
to add this in the future as a parameter to the Accept Synchronous Connection
Command, however its a little trickier because the kernel does not
currently preserve sockaddr_sco data between userspace calls to accept().
The most common use for sco_pkt_type will be to white-list only SCO packets,
which can be done with the hci.h constant SCO_ESCO_MASK.
This patch is motivated by broken Bluetooth carkits such as the Motorola
HF850 (it claims to support eSCO, but will actually reject eSCO connections
after 5 seconds) and the 2007/2008 Infiniti G35/37 (fails to route audio
if a 2-EV5 packet type is negiotiated). With this patch userspace can maintain
a list of compatible packet types to workaround remote devices such as these.
---
include/bluetooth/hci.h | 17 +++++++++++++++++
include/bluetooth/sco.h | 2 ++
2 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/include/bluetooth/hci.h b/include/bluetooth/hci.h
index dcd32e7..aaee296 100644
--- a/include/bluetooth/hci.h
+++ b/include/bluetooth/hci.h
@@ -133,6 +133,23 @@ enum {
#define SCO_PTYPE_MASK (HCI_HV1 | HCI_HV2 | HCI_HV3)
#define ACL_PTYPE_MASK (HCI_DM1 | HCI_DH1 | HCI_DM3 | HCI_DH3 | HCI_DM5 | HCI_DH5)
+/* eSCO packet types */
+#define ESCO_HV1 0x0001
+#define ESCO_HV2 0x0002
+#define ESCO_HV3 0x0004
+#define ESCO_EV3 0x0008
+#define ESCO_EV4 0x0010
+#define ESCO_EV5 0x0020
+#define ESCO_2EV3 0x0040
+#define ESCO_3EV3 0x0080
+#define ESCO_2EV5 0x0100
+#define ESCO_3EV5 0x0200
+
+#define SCO_ESCO_MASK (ESCO_HV1 | ESCO_HV2 | ESCO_HV3)
+#define EDR_ESCO_MASK (ESCO_2EV3 | ESCO_3EV3 | ESCO_2EV5 | ESCO_3EV5)
+#define ALL_ESCO_MASK (SCO_ESCO_MASK | ESCO_EV3 | ESCO_EV4 | ESCO_EV5 | \
+ EDR_ESCO_MASK)
+
/* HCI Error codes */
#define HCI_UNKNOWN_COMMAND 0x01
#define HCI_NO_CONNECTION 0x02
diff --git a/include/bluetooth/sco.h b/include/bluetooth/sco.h
index 43b6069..ff71829 100644
--- a/include/bluetooth/sco.h
+++ b/include/bluetooth/sco.h
@@ -41,6 +41,8 @@ extern "C" {
struct sockaddr_sco {
sa_family_t sco_family;
bdaddr_t sco_bdaddr;
+ /* for use with eSCO masks such as SCO_ESCO_MASK */
+ uint16_t sco_pkt_type;
};
/* set/get sockopt defines */
--
1.6.5.3
[-- Attachment #5: 0002-Add-option-to-change-sco-packet-type-in-scotest.patch --]
[-- Type: application/octet-stream, Size: 2059 bytes --]
From 833b10b88c0e8aa7acd98133f8f7ccac64cafd3a Mon Sep 17 00:00:00 2001
From: Nick Pelly <npelly@google.com>
Date: Thu, 18 Feb 2010 16:04:42 -0800
Subject: [PATCH 2/2] Add option to change sco packet type in scotest.
---
test/scotest.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/test/scotest.c b/test/scotest.c
index 80081c6..d72c610 100644
--- a/test/scotest.c
+++ b/test/scotest.c
@@ -55,6 +55,9 @@ static unsigned char *buf;
/* Default data size */
static long data_size = 672;
+/* Default packet type */
+static uint16_t pkt_type = 0;
+
static bdaddr_t bdaddr;
static float tv2fl(struct timeval tv)
@@ -91,6 +94,7 @@ static int do_connect(char *svr)
/* Connect to remote device */
memset(&addr, 0, sizeof(addr));
addr.sco_family = AF_BLUETOOTH;
+ addr.sco_pkt_type = pkt_type;
str2ba(svr, &addr.sco_bdaddr);
if (connect(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
@@ -139,6 +143,7 @@ static void do_listen(void (*handler)(int sk))
/* Bind to local address */
memset(&addr, 0, sizeof(addr));
addr.sco_family = AF_BLUETOOTH;
+ addr.sco_pkt_type = pkt_type;
bacpy(&addr.sco_bdaddr, &bdaddr);
if (bind(sk, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
@@ -329,7 +334,7 @@ static void usage(void)
{
printf("scotest - SCO testing\n"
"Usage:\n");
- printf("\tscotest <mode> [-b bytes] [bd_addr]\n");
+ printf("\tscotest <mode> [-b bytes] [-p pkt_type] [bd_addr]\n");
printf("Modes:\n"
"\t-d dump (server)\n"
"\t-c reconnect (client)\n"
@@ -344,7 +349,7 @@ int main(int argc ,char *argv[])
struct sigaction sa;
int opt, sk, mode = RECV;
- while ((opt=getopt(argc,argv,"rdscmnb:")) != EOF) {
+ while ((opt=getopt(argc,argv,"rdscmnb:p:")) != EOF) {
switch(opt) {
case 'r':
mode = RECV;
@@ -374,6 +379,13 @@ int main(int argc ,char *argv[])
data_size = atoi(optarg);
break;
+ case 'p':
+ if (sscanf(optarg, "0x%4hx", &pkt_type) != 1) {
+ usage();
+ exit(1);
+ }
+ break;
+
default:
usage();
exit(1);
--
1.6.5.3
^ permalink raw reply related
* Re: [PATCH] Bluetooth: Allow SCO/eSCO packet type selection for outgoing SCO connections.
From: Ville Tervo @ 2010-02-19 8:08 UTC (permalink / raw)
To: ext Nick Pelly; +Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org
In-Reply-To: <35c90d961002181547x1ea26ba7je4eee29ef29e6fd2@mail.gmail.com>
Hi Nick
>> And even if we would be going for a setsockopt(), that would be blocking
>> and then again pretty much pointless API. The sockaddr is most logical
>> thing that fits into what we wanna achieve. Disallow/allow certain
>> packet types and essentially force SCO over eSCO.
>
> Ok seems like there is agreement on using struct sockaddr_sco for sco_pkt_type.
>
> A more controversial question is whether to follow the SIG convention
> of reversing the logic on the EDR bits in the packet type mask (1
> means do *not* use this packet for the EDR bits), or to use consistent
> logic for each bit (1 always means *allow* this packet type) for
> packet selection in scokaddr_sco.sco_pkt_type.
>
> The original patch I posted followed the SIG convention. However after
> more thinking I am leaning towards using consistent logic for each
> bit. 1 will always mean 'allow this packet'. My rationale is that the
> most common use for sco_pkt_type will be to request only SCO packet
> types allowed. If however the SIG adds more reverse logic packet
> types, and we follow the SIG convention, then old userspace code that
> just requested the SCO packet types will now end up with the new
> packet types as well. So I think it is best to avoid this situation
> and for 1 to always mean 'allow this packet' in
> sockaddr_sco.sco_pkt_type.
>
> Attached is a new patch with the consistent bit logic.
>
> Comments?
In order to keep backwards compatibility 1 should mean "don't allow this
packet type" for all packets. Other wise old application with new kernel
would not allow any packet types.
--
Ville
^ permalink raw reply
* obexd-0.21: configure doesn't work as expected
From: Pacho Ramos @ 2010-02-19 11:13 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]
Hello all!
The problem we are suffering downstream is the following:
http://bugs.gentoo.org/show_bug.cgi?id=305063#c5
In summary, the same files are being *installed* with
--with-phonebook=ebook and --with-phonebook=dummy (in both cases,
phonebook files are not being installed at all). A
$PREFIX/lib/obex/plugins directory is created but nothing is installed
on it.
Another problem is that, when passing --without-phonebook to configure,
build fails with:
$ make
make --no-print-directory all-am
CC gdbus/mainloop.o
CC gdbus/object.o
CC gdbus/watch.o
CC plugins/filesystem.o
CC plugins/opp.o
CC plugins/ftp.o
CC plugins/pbap.o
CC plugins/syncevolution.o
CC src/main.o
GEN src/builtin.h
CC src/plugin.o
CC src/logging.o
CC src/btio.o
CC src/manager.o
CC src/obex.o
CC src/bluetooth.o
CC src/mimetype.o
CC src/service.o
make[1]: *** No rule to make target `plugins/phonebook-no.c', needed by
`plugins/phonebook.c'. Stop.
make: *** [all] Error 2
Thanks a lot for your help :-)
[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCHes] Patches for OpenObex
From: Bastien Nocera @ 2010-02-19 12:10 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 512 bytes --]
Heya,
2 small patches for openobex.
The first patch fixes libusb1 compilation on my machine.
For the second patch, when libusb1 is used, we can set self->fd to be a
monitoring file descriptor for incoming data.
With that patch, I could make osso-gwobex work with USB connections.
There's still some bugs to take care of, but I believe this patch to be
correct. Are there any places in openobex where the self->fd will be
directly when connected via USB? If so, those would need to be fixed as
well.
Cheers
[-- Attachment #2: 0001-Fix-libusb1-detection.patch --]
[-- Type: text/x-patch, Size: 904 bytes --]
>From 80ac78ca813d057b2c506f936e1b8b8d3c939357 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Fri, 19 Feb 2010 12:03:01 +0000
Subject: [PATCH 1/2] Fix libusb1 detection
There's no need to check for the presence of a .pc file in
/usr/lib/, pkg-config will already do that for you, and it would
fail on machines where /usr/lib64 is used, or when libusb1 is
installed in another prefix.
---
acinclude.m4 | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 81fa9da..d813b71 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -191,7 +191,6 @@ AC_DEFUN([AC_PATH_USB], [
AC_DEFUN([AC_PATH_USB1], [
usb1_lib_found=no
PKG_CHECK_MODULES(USB1, libusb-1.0, usb1_lib_found=yes, usb1_lib_found=no)
- AC_CHECK_FILE(${prefix}/lib/pkgconfig/libusb-1.0.pc, REQUIRES="libusb1")
AC_SUBST(USB1_CFLAGS)
AC_SUBST(USB1_LIBS)
--
1.6.6.1
[-- Attachment #3: 0002-Export-the-libusb1-read-file-descriptor.patch --]
[-- Type: text/x-patch, Size: 1877 bytes --]
>From 55979f925051f794337a59a8e76727d03a215c62 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Fri, 19 Feb 2010 12:04:43 +0000
Subject: [PATCH 2/2] Export the libusb1 read file descriptor
When using libusb1, we can export the file descriptor that
corresponds to reading from the device, so it can be used to
setup polling sources, and timeout based operations.
This would be used by osso-gwobex to monitor incoming data, for
USB support.
---
lib/usb1obex.c | 31 +++++++++++++++++++++++++++++++
1 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/lib/usb1obex.c b/lib/usb1obex.c
index dfa1685..6a80fbb 100644
--- a/lib/usb1obex.c
+++ b/lib/usb1obex.c
@@ -30,6 +30,7 @@
#include <stdio.h> /* perror */
#include <errno.h> /* errno and EADDRNOTAVAIL */
#include <stdlib.h>
+#include <poll.h> /* POLLOUT */
#include <libusb.h>
@@ -339,6 +340,35 @@ void usbobex_free_interfaces(int num, obex_interface_t *intf)
}
/*
+ * Function usbobex_get_fd ()
+ *
+ * Get the "poll out" file descriptor for the USB device,
+ * used to check for events in an async way
+ *
+ */
+static int usbobex_get_fd(void)
+{
+ const struct libusb_pollfd **usbfds;
+ const struct libusb_pollfd *usbfd;
+ int i = 0;
+
+ DEBUG(4, "Getting the USB file descriptor");
+
+ usbfds = libusb_get_pollfds(libusb_ctx);
+ if (usbfds == NULL) {
+ DEBUG(4, "Could not get USB file descriptors");
+ return INVALID_SOCKET;
+ }
+
+ while ((usbfd = usbfds[i++]) != NULL) {
+ if (usbfd->events & POLLOUT)
+ return usbfd->fd;
+ }
+
+ return INVALID_SOCKET;
+}
+
+/*
* Function usbobex_connect_request (self)
*
* Open the USB connection
@@ -379,6 +409,7 @@ int usbobex_connect_request(obex_t *self)
}
self->trans.mtu = OBEX_MAXIMUM_MTU;
+ self->fd = usbobex_get_fd();
DEBUG(2, "transport mtu=%d\n", self->trans.mtu);
return 1;
--
1.6.6.1
^ permalink raw reply related
* Re: [PATCHes] Patches for OpenObex
From: Bastien Nocera @ 2010-02-19 13:49 UTC (permalink / raw)
To: BlueZ development
In-Reply-To: <1266581433.678.6028.camel@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 693 bytes --]
On Fri, 2010-02-19 at 12:10 +0000, Bastien Nocera wrote:
> Heya,
>
> 2 small patches for openobex.
>
> The first patch fixes libusb1 compilation on my machine.
>
> For the second patch, when libusb1 is used, we can set self->fd to be a
> monitoring file descriptor for incoming data.
>
> With that patch, I could make osso-gwobex work with USB connections.
>
> There's still some bugs to take care of, but I believe this patch to be
> correct. Are there any places in openobex where the self->fd will be
> directly when connected via USB? If so, those would need to be fixed as
> well.
Another patch to fix an invalid memory access when obex_transport_read()
has to resize its buffer.
[-- Attachment #2: 0001-Fix-invalid-memory-access.patch --]
[-- Type: text/x-patch, Size: 3130 bytes --]
>From a3e0a7c2ed10ffab279ad3cab0c3139e651e35b7 Mon Sep 17 00:00:00 2001
From: Bastien Nocera <hadess@hadess.net>
Date: Fri, 19 Feb 2010 13:40:27 +0000
Subject: [PATCH] Fix invalid memory access
The hdr pointer will not be valid any more if the transport
read() does a realloc, so cache the opcode that we'll use
later in the code.
Fixes the following valgrind error:
==31644== Thread 2:
==31644== Invalid read of size 1
==31644== at 0x4E3D787: obex_data_indication (obex_main.c:307)
==31644== by 0x4E3F403: obex_transport_handle_input (obex_transport.c:72)
==31644== by 0x4E3C67E: OBEX_HandleInput (obex.c:449)
==31644== by 0x4C335BD: gw_obex_request_sync (obex-priv.c:108)
==31644== by 0x4C34114: gw_obex_get (obex-priv.c:939)
==31644== by 0x4C319EE: gw_obex_read_dir (gw-obex.c:198)
==31644== by 0x408222: _retrieve_folder_listing (gvfsbackendobexftp.c:552)
==31644== by 0x408DD1: do_enumerate (gvfsbackendobexftp.c:1549)
==31644== by 0x411491: g_vfs_job_run (gvfsjob.c:198)
==31644== by 0x3C758658CA: ??? (in /lib64/libglib-2.0.so.0.2303.0)
==31644== by 0x3C75863A03: ??? (in /lib64/libglib-2.0.so.0.2303.0)
==31644== by 0x3C74806CA9: start_thread (in /lib64/libpthread-2.11.90.so)
==31644== Address 0x5313f50 is 0 bytes inside a block of size 71,679 free'd
==31644== at 0x4A05255: realloc (vg_replace_malloc.c:476)
==31644== by 0x4E41D12: buf_resize (databuffer.c:147)
==31644== by 0x4E42063: buf_reserve_end (databuffer.c:217)
==31644== by 0x4E3FE90: obex_transport_read (obex_transport.c:519)
==31644== by 0x4E3D711: obex_data_indication (obex_main.c:268)
==31644== by 0x4E3F403: obex_transport_handle_input (obex_transport.c:72)
==31644== by 0x4E3C67E: OBEX_HandleInput (obex.c:449)
==31644== by 0x4C335BD: gw_obex_request_sync (obex-priv.c:108)
==31644== by 0x4C34114: gw_obex_get (obex-priv.c:939)
==31644== by 0x4C319EE: gw_obex_read_dir (gw-obex.c:198)
==31644== by 0x408222: _retrieve_folder_listing (gvfsbackendobexftp.c:552)
==31644== by 0x408DD1: do_enumerate (gvfsbackendobexftp.c:1549)
==31644==
---
lib/obex_main.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/lib/obex_main.c b/lib/obex_main.c
index 9fb65d7..435f1a9 100644
--- a/lib/obex_main.c
+++ b/lib/obex_main.c
@@ -234,6 +234,7 @@ int obex_data_indication(obex_t *self, uint8_t *buf, int buflen)
int actual = 0;
unsigned int size;
int ret;
+ int opcode;
DEBUG(4, "\n");
@@ -262,6 +263,9 @@ int obex_data_indication(obex_t *self, uint8_t *buf, int buflen)
hdr = (obex_common_hdr_t *) msg->data;
size = ntohs(hdr->len);
+ /* As hdr might not be valid anymore if the _read() does a realloc */
+ opcode = hdr->opcode;
+
actual = 0;
if(msg->data_size < (int) ntohs(hdr->len)) {
@@ -304,7 +308,7 @@ int obex_data_indication(obex_t *self, uint8_t *buf, int buflen)
DUMPBUFFER(2, "Rx", msg);
actual = msg->data_size;
- final = hdr->opcode & OBEX_FINAL; /* Extract final bit */
+ final = opcode & OBEX_FINAL; /* Extract final bit */
/* Dispatch to the mode we are in */
if(self->state & MODE_SRV) {
--
1.6.6.1
^ permalink raw reply related
* Re: obexd-0.21: configure doesn't work as expected
From: Vinicius Gomes @ 2010-02-19 15:01 UTC (permalink / raw)
To: pacho; +Cc: linux-bluetooth
In-Reply-To: <1266578028.5249.1.camel@localhost.localdomain>
Hi Pacho,
2010/2/19 Pacho Ramos <pacho@condmat1.ciencias.uniovi.es>:
> Hello all!
>
> The problem we are suffering downstream is the following:
> http://bugs.gentoo.org/show_bug.cgi?id=305063#c5
>
> In summary, the same files are being *installed* with
> --with-phonebook=ebook and --with-phonebook=dummy (in both cases,
> phonebook files are not being installed at all). A
> $PREFIX/lib/obex/plugins directory is created but nothing is installed
> on it.
>
This is not a bug, it is by design, the phonebook driver is a built-in
module. In fact, all the plugins
we have currently are built-in. That's why nothing gets installed in
the plugins folder.
> Another problem is that, when passing --without-phonebook to configure,
> build fails with:
> $ make
> make --no-print-directory all-am
> CC gdbus/mainloop.o
> CC gdbus/object.o
> CC gdbus/watch.o
> CC plugins/filesystem.o
> CC plugins/opp.o
> CC plugins/ftp.o
> CC plugins/pbap.o
> CC plugins/syncevolution.o
> CC src/main.o
> GEN src/builtin.h
> CC src/plugin.o
> CC src/logging.o
> CC src/btio.o
> CC src/manager.o
> CC src/obex.o
> CC src/bluetooth.o
> CC src/mimetype.o
> CC src/service.o
> make[1]: *** No rule to make target `plugins/phonebook-no.c', needed by
> `plugins/phonebook.c'. Stop.
> make: *** [all] Error 2
>
Thanks for the report, this is a bug. There's a patch for it in my
development tree. Waiting for review.
But before the next release happens, disabling a plugin is equivalent
to setting its value to dummy, i.e.
--without-phonebook (if it worked ;-) would be the same as
--with-phonebook=dummy.
>
> Thanks a lot for your help :-)
>
Cheers,
--
Vinicius Gomes
INdT - Instituto Nokia de Tecnologia
^ permalink raw reply
* Re: obexd-0.21: configure doesn't work as expected
From: Pacho Ramos @ 2010-02-19 15:07 UTC (permalink / raw)
To: Vinicius Gomes; +Cc: linux-bluetooth
In-Reply-To: <2a9506371002190701o2d46102bpd50f9a95b37a816e@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
El vie, 19-02-2010 a las 12:01 -0300, Vinicius Gomes escribió:
> > In summary, the same files are being *installed* with
> > --with-phonebook=ebook and --with-phonebook=dummy (in both cases,
> > phonebook files are not being installed at all). A
> > $PREFIX/lib/obex/plugins directory is created but nothing is installed
> > on it.
> >
>
> This is not a bug, it is by design, the phonebook driver is a built-in
> module. In fact, all the plugins
> we have currently are built-in. That's why nothing gets installed in
> the plugins folder.
Ah, then, why is plugins folder being created even if nothing will be
installed in it?
> Thanks for the report, this is a bug. There's a patch for it in my
> development tree. Waiting for review.
>
> But before the next release happens, disabling a plugin is equivalent
> to setting its value to dummy, i.e.
> --without-phonebook (if it worked ;-) would be the same as
> --with-phonebook=dummy.
>
Great, where could I see your development tree?
Thanks a lot :-)
[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] Fix double free on AVDTP Abort response
From: Daniel Örstadius @ 2010-02-19 16:20 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1: Type: text/plain, Size: 732 bytes --]
With the patch I submitted some time ago
http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=e9b1a8f7266d0674b1ea068a5bb5698e9ee424c9
there is a code path leading to a double free:
session_cb -> avdtp_parse_resp -> avdtp_abort_resp ->
avdtp_sep_set_state(..., AVDTP_STATE_IDLE) -> handle_unanswered_req
A response to AVDTP Abort could lead to the pending request being
freed both in session_cb and handle_unanswered_req.
This patch avoids doing it in the latter function. The primary purpose
of adding handle_unanswered_req was to trigger responses on the Audio
API (it was based on avdtp.c:request_timeout). AFAIU, AVDTP Abort
doesn't lead to an API response and will be freed elsewhere
(session_cb or avdtp_unref).
/Daniel
[-- Attachment #2: 0001-Fix-double-free-on-AVDTP-Abort-response.patch --]
[-- Type: text/x-patch, Size: 963 bytes --]
From 803637bc0e452392498714cd8245a06f5aea2edc Mon Sep 17 00:00:00 2001
From: Daniel Orstadius <daniel.orstadius@gmail.com>
Date: Fri, 19 Feb 2010 17:51:48 +0200
Subject: [PATCH] Fix double free on AVDTP Abort response
The pending request might be freed twice when receiving an Abort
response, in handle_unanswered_req and session_cb. Avoid freeing
it in handle_unanswered_req.
---
audio/avdtp.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/audio/avdtp.c b/audio/avdtp.c
index 2591845..ae7c88e 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -905,6 +905,13 @@ static void handle_unanswered_req(struct avdtp *session,
struct avdtp_local_sep *lsep;
struct avdtp_error err;
+ if (session->req == AVDTP_ABORT) {
+ /* Avoid freeing the Abort request here */
+ debug("handle_unanswered_req: Abort req, returning");
+ session->req->stream = NULL;
+ return;
+ }
+
req = session->req;
session->req = NULL;
--
1.6.0.4
^ permalink raw reply related
* Re: obexd-0.21: configure doesn't work as expected
From: Vinicius Gomes @ 2010-02-19 17:24 UTC (permalink / raw)
To: pacho; +Cc: linux-bluetooth
In-Reply-To: <1266592077.5249.21.camel@localhost.localdomain>
Hi Pacho,
2010/2/19 Pacho Ramos <pacho@condmat1.ciencias.uniovi.es>:
> Ah, then, why is plugins folder being created even if nothing will be
> installed in it?
>
Nothing is installed right now. But it doesn't mean that nothing will
ever be ;-)
And if you want to have a custom plugin for some application, you just write
it and install it there, no need to mess with core obexd.
>> Thanks for the report, this is a bug. There's a patch for it in my
>> development tree. Waiting for review.
>>
>> But before the next release happens, disabling a plugin is equivalent
>> to setting its value to dummy, i.e.
>> --without-phonebook (if it worked ;-) would be the same as
>> --with-phonebook=dummy.
>>
>
> Great, where could I see your development tree?
>
Sure. Forgot the link, here it is:
http://git.infradead.org/users/vcgomes/obexd.git (devel branch)
> Thanks a lot :-)
>
Cheers,
--
Vinicius Gomes
INdT - Instituto Nokia de Tecnologia
^ permalink raw reply
* Re: [PATCH] Fix double free on AVDTP Abort response
From: Johan Hedberg @ 2010-02-19 17:32 UTC (permalink / raw)
To: Daniel Örstadius; +Cc: linux-bluetooth
In-Reply-To: <eb7933e21002190820x7b2b0ea3v3f48ce96a5d11a1e@mail.gmail.com>
Hi Daniel,
On Fri, Feb 19, 2010, Daniel Örstadius wrote:
> + if (session->req == AVDTP_ABORT) {
> + /* Avoid freeing the Abort request here */
> + debug("handle_unanswered_req: Abort req, returning");
> + session->req->stream = NULL;
> + return;
> + }
Please try to at least do a compile check before you submit patches.
This one gives the following error:
audio/avdtp.c: In function ‘handle_unanswered_req’:
audio/avdtp.c:908: error: comparison between pointer and integer
What you probably want is session->req->signal_id == AVDTP_ABORT.
Johan
^ permalink raw reply
* Re: obexd-0.21: configure doesn't work as expected
From: Pacho Ramos @ 2010-02-19 17:40 UTC (permalink / raw)
To: Vinicius Gomes; +Cc: linux-bluetooth
In-Reply-To: <2a9506371002190924p2ff4ff6eq243dc3031ae3b3a7@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]
El vie, 19-02-2010 a las 14:24 -0300, Vinicius Gomes escribió:
>
> 2010/2/19 Pacho Ramos <pacho@condmat1.ciencias.uniovi.es>:
> > Ah, then, why is plugins folder being created even if nothing will be
> > installed in it?
> >
>
> Nothing is installed right now. But it doesn't mean that nothing will
> ever be ;-)
>
> And if you want to have a custom plugin for some application, you just write
> it and install it there, no need to mess with core obexd.
>
True, silly me!
> >> Thanks for the report, this is a bug. There's a patch for it in my
> >> development tree. Waiting for review.
> >>
> >> But before the next release happens, disabling a plugin is equivalent
> >> to setting its value to dummy, i.e.
> >> --without-phonebook (if it worked ;-) would be the same as
> >> --with-phonebook=dummy.
> >>
> >
> > Great, where could I see your development tree?
> >
>
> Sure. Forgot the link, here it is:
> http://git.infradead.org/users/vcgomes/obexd.git (devel branch)
>
Great :-D
Thanks and best regards
[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* obex-client: how to select a dongle from muliple dongles to send-files
From: Ed Tsang @ 2010-02-19 19:47 UTC (permalink / raw)
To: Bluettooth Linux
Hi,
Need some hint from the expert.
I am using test/send-files to send files. But when I have multiple dongles, I would like in run time to select which dongle to send out the file either using hci0/1 or the actual dongle address or other means. I tried trace it down but got lost. Anyone could provide some hint.
Ed
__________________________________________________________________
Looking for the perfect gift? Give the gift of Flickr!
http://www.flickr.com/gift/
^ permalink raw reply
* Re: [PATCH] Bluetooth: Allow SCO/eSCO packet type selection for outgoing SCO connections.
From: Nick Pelly @ 2010-02-19 21:23 UTC (permalink / raw)
To: Ville Tervo; +Cc: Marcel Holtmann, linux-bluetooth@vger.kernel.org
In-Reply-To: <4B7E46FC.5030408@nokia.com>
On Fri, Feb 19, 2010 at 12:08 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> Hi Nick
>
>>> And even if we would be going for a setsockopt(), that would be blocking
>>> and then again pretty much pointless API. The sockaddr is most logical
>>> thing that fits into what we wanna achieve. Disallow/allow certain
>>> packet types and essentially force SCO over eSCO.
>>
>> Ok seems like there is agreement on using struct sockaddr_sco for
>> sco_pkt_type.
>>
>> A more controversial question is whether to follow the SIG convention
>> of reversing the logic on the EDR bits in the packet type mask (1
>> means do *not* use this packet for the EDR bits), or to use consistent
>> logic for each bit (1 always means *allow* this packet type) for
>> packet selection in scokaddr_sco.sco_pkt_type.
>>
>> The original patch I posted followed the SIG convention. However after
>> more thinking I am leaning towards using consistent logic for each
>> bit. 1 will always mean 'allow this packet'. My rationale is that the
>> most common use for sco_pkt_type will be to request only SCO packet
>> types allowed. If however the SIG adds more reverse logic packet
>> types, and we follow the SIG convention, then old userspace code that
>> just requested the SCO packet types will now end up with the new
>> packet types as well. So I think it is best to avoid this situation
>> and for 1 to always mean 'allow this packet' in
>> sockaddr_sco.sco_pkt_type.
>>
>> Attached is a new patch with the consistent bit logic.
>>
>> Comments?
>
> In order to keep backwards compatibility 1 should mean "don't allow this
> packet type" for all packets. Other wise old application with new kernel
> would not allow any packet types.
The current patch achieves this backwards compatibility by assuming
that all packet types are allowed if the old sockaddr_sco struct is
used (it checks the size), or if sco_pkt_type is 0.
Nick
^ permalink raw reply
* Re: obex-client: how to select a dongle from muliple dongles to send-files
From: lorenzo.brito @ 2010-02-19 22:14 UTC (permalink / raw)
To: Ed Tsang; +Cc: Bluettooth Linux
In-Reply-To: <588634.45024.qm@web52606.mail.re2.yahoo.com>
Im implementing something similar. Obexftp library gives you a free
conecxion so what im doing is to call the method by diferent threats.
Sent from my iPod
On 19/02/2010, at 13:47, Ed Tsang <netdesign_98@yahoo.com> wrote:
> Hi,
> Need some hint from the expert.
> I am using test/send-files to send files. But when I have
> multiple dongles, I would like in run time to select which dongle to
> send out the file either using hci0/1 or the actual dongle address
> or other means. I tried trace it down but got lost. Anyone could
> provide some hint.
>
> Ed
>
>
>
>
> __________________________________________________________________
> Looking for the perfect gift? Give the gift of Flickr!
>
> http://www.flickr.com/gift/
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Kernel panic in rfcomm_run - unbalanced refcount on rfcomm_session
From: Dave Young @ 2010-02-20 8:17 UTC (permalink / raw)
To: Nick Pelly; +Cc: Bluettooth Linux
In-Reply-To: <35c90d961002172104q3af1ca8p850004f8b93e8af7@mail.gmail.com>
On Thu, Feb 18, 2010 at 1:04 PM, Nick Pelly <npelly@google.com> wrote:
> Since 2.6.32 we are seeing kernel panics like:
>
> [10651.110229] Unable to handle kernel paging request at virtual
> address 6b6b6b6b
> [10651.111968] Internal error: Oops: 5 [#1] PREEMPT
> [10651.113952] CPU: 0 Tainted: G W (2.6.32-59979-gd0c97db #1)
> [10651.114624] PC is at rfcomm_run+0xa04/0xdbc
> <...>
> [10651.406188] [<c031ad24>] (rfcomm_run+0xa04/0xdbc) from [<c006ce30>]
> (kthread+0x78/0x80)
> [10651.406585] [<c006ce30>] (kthread+0x78/0x80) from [<c002793c>]
> (kernel_thread_exit+0x0/0x8)
>
> (rfcomm_run() is all inlined so theres not much of a stack trace))
Could you make rfcomm_process_sessions to be not inlined, and get new
kernel logs?
>
> This is a use-after-free on struct rfcomm_session s in the call chain
> rfcomm_run() -> rfcomm_process_sessions() -> rfcomm_process_dlcs() ->
> list_for_each_safe(p, n, &s->dlcs). The only way this can happen is if
> there is an unbalanced refcount on the rfcomm session.
>
> We found that reverting the patch
> 9e726b17422bade75fba94e625cd35fd1353e682 "Bluetooth: Fix rejected
> connection not disconnecting ACL link" fixes the issue for us. The
> patch itself looks ok, I added some logging to check the new refcounts
> in the patch are balanced and they are. However if I remove the new
> calls to rfcomm_session_put() and rfcomm_session_hold() the crash is
> resolved. I also found that we can crash without hitting
> rfcomm_session_timeout(), so its not related to Marcel's recent patch
> to remove the scheduling-while-atomic warning.
>
> 9e726b17422bade75fba94e625cd35fd1353e682 does lead to a delay in
> calling rfcomm_session_del() due to the extra refcount while waiting
> for the new timeout. I believe that this delay has revealed some more
> subtle problem elsewhere that causes an unbalanced refcount and then
> the kernel panic.
>
> I have debug kernel logs and hci logs - they are too large to send to
> the list but I can send them directly to anyone interested in
> debugging.
>
> We see this crash frequently with a number of headsets since 2.6.32,
> but not reliably. I do have a 100% repro case with the Nuvi Garmin,
> with these exact steps:
> 1) Make sure Nuvi is unpaired, Bluez stack is unpaired, and kernel has
> been rebooted since unpairing.
> 2) Initiate device discovery, pairing, and handsfree connection from Nuvi
> 3) Observe HFP rfcomm connect briefly, then disconnect, and kernel panic
>
> Our short-term solution is unfortunately to revert
> 9e726b17422bade75fba94e625cd35fd1353e682.
>
> Nick
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Regards
dave
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox