* Re: [PATCH 2/6] Bluetooth: fix receiving L2CAP packets over LE
From: Vinicius Gomes @ 2010-10-29 13:41 UTC (permalink / raw)
To: Ville Tervo; +Cc: ext Anderson Briglia, linux-bluetooth@vger.kernel.org
In-Reply-To: <20101029104435.GQ15050@null>
Hi Ville,
On Fri, Oct 29, 2010 at 6:44 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> Hi Anderson,
>
> On Sat, Oct 23, 2010 at 01:56:56AM +0200, ext Anderson Briglia wrote:
>> From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
>>
>> As L2CAP packets coming over LE don't have any more encapsulation,
>> other than L2CAP, we are able to process them as soon as they arrive.
>
> Why is this change needed? Was something broken without this patch?
>
This change is needed because without it the receiving side would
always think that it was receiving continuation frames.
As the flags parameter is zero, it would fall into the "} else {"
condition, and because no frame was received before, conn->rx_len
would be zero and the frame would be discarded. Without this patch I
was seeing those "Unexpected continuation frame ..." messages on the
receiving side.
>
>
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
>> ---
>> net/bluetooth/l2cap.c | 17 +++++++++++++++--
>> 1 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index 2bf083e..1ac44f4 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -4768,17 +4768,30 @@ static int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
>> static int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>> {
>> struct l2cap_conn *conn = hcon->l2cap_data;
>> + struct l2cap_hdr *hdr;
>> + int len;
>>
>> if (!conn && !(conn = l2cap_conn_add(hcon, 0)))
>> goto drop;
>>
>> BT_DBG("conn %p len %d flags 0x%x", conn, skb->len, flags);
>>
>> + if (hcon->type == LE_LINK) {
>> + hdr = (struct l2cap_hdr *) skb->data;
>> + len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
>> +
>> + if (len == skb->len) {
>> + /* Complete frame received */
>> + l2cap_recv_frame(conn, skb);
>> + return 0;
>> + }
>> +
>> + goto drop;
>> + }
>> +
>> if (flags & ACL_START) {
>> - struct l2cap_hdr *hdr;
>> struct sock *sk;
>> u16 cid;
>> - int len;
>>
>> if (conn->rx_len) {
>> BT_ERR("Unexpected start frame (len %d)", skb->len);
>> --
>> 1.7.0.4
>>
>> --
>> 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
>
> --
> Ville
>
Cheers,
--
Vinicius
^ permalink raw reply
* [PATCHv3 0/2] Fix kernel crash in rfcomm/l2cap
From: Emeltchenko Andrei @ 2010-10-29 13:42 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
Do not delete l2cap channel and socket sk when sk is owned by user.
To delete l2cap channel standard timer is used.
lock_sock and release_sock do not hold a normal spinlock directly but
instead hold the owner field. This means bh_lock_sock can still execute
even if the socket is "locked". More info can be found here:
http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
When sending following sequence:
...
No. Time Source Destination Protocol Info
89 1.951202 RFCOMM Rcvd DISC DLCI=20
90 1.951324 RFCOMM Sent UA DLCI=20
91 1.959381 HCI_EVT Number of Completed Packets
92 1.966461 RFCOMM Rcvd DISC DLCI=0
93 1.966492 L2CAP Rcvd Disconnect Request
94 1.972595 L2CAP Sent Disconnect Response
...
krfcommd kernel thread is preempted with l2cap tasklet which remove l2cap_conn
(L2CAP connection handler structure). Then rfcomm thread tries to send RFCOMM
UA which is reply to RFCOMM DISC and when de-referencing l2cap_conn crash
happens.
Andrei Emeltchenko (2):
Bluetooth: Check sk is not owned before freeing l2cap_conn
Bluetooth: timer check sk is not owned before freeing
net/bluetooth/l2cap.c | 58 ++++++++++++++++++++++++++++++++++++++----------
1 files changed, 46 insertions(+), 12 deletions(-)
^ permalink raw reply
* [PATCHv3 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
From: Emeltchenko Andrei @ 2010-10-29 13:43 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288359781-4059-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Check that socket sk is not locked in user process before removing
l2cap connection handler.
lock_sock and release_sock do not hold a normal spinlock directly but
instead hold the owner field. This means bh_lock_sock can still execute
even if the socket is "locked". More info can be found here:
http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
krfcommd kernel thread may be preempted with l2cap tasklet which remove
l2cap_conn structure. If krfcommd is in process of sending of RFCOMM reply
(like "RFCOMM UA" reply to "RFCOMM DISC") then kernel crash happens.
...
[ 694.175933] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[ 694.184936] pgd = c0004000
[ 694.187683] [00000000] *pgd=00000000
[ 694.191711] Internal error: Oops: 5 [#1] PREEMPT
[ 694.196350] last sysfs file: /sys/devices/platform/hci_h4p/firmware/hci_h4p/loading
[ 694.260375] CPU: 0 Not tainted (2.6.32.10 #1)
[ 694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
[ 694.270721] LR is at 0xd7017303
...
[ 694.525085] Backtrace:
[ 694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap]) from [<c02f2cc8>] (sock_sendmsg+0xb8/0xd8)
[ 694.537292] [<c02f2c10>] (sock_sendmsg+0x0/0xd8) from [<c02f3044>] (kernel_sendmsg+0x48/0x80)
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
net/bluetooth/l2cap.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 6f931cc..b1344d8 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3078,6 +3078,14 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
break;
default:
+ /* don't delete l2cap channel if sk is owned by user */
+ if (sock_owned_by_user(sk)) {
+ sk->sk_state = BT_DISCONN;
+ l2cap_sock_clear_timer(sk);
+ l2cap_sock_set_timer(sk, HZ);
+ break;
+ }
+
l2cap_chan_del(sk, ECONNREFUSED);
break;
}
@@ -3283,6 +3291,15 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
sk->sk_shutdown = SHUTDOWN_MASK;
+ /* don't delete l2cap channel if sk is owned by user */
+ if (sock_owned_by_user(sk)) {
+ sk->sk_state = BT_DISCONN;
+ l2cap_sock_clear_timer(sk);
+ l2cap_sock_set_timer(sk, HZ);
+ bh_unlock_sock(sk);
+ return 0;
+ }
+
l2cap_chan_del(sk, ECONNRESET);
bh_unlock_sock(sk);
@@ -3305,6 +3322,15 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
if (!sk)
return 0;
+ /* don't delete l2cap channel if sk is owned by user */
+ if (sock_owned_by_user(sk)) {
+ sk->sk_state = BT_DISCONN;
+ l2cap_sock_clear_timer(sk);
+ l2cap_sock_set_timer(sk, HZ);
+ bh_unlock_sock(sk);
+ return 0;
+ }
+
l2cap_chan_del(sk, 0);
bh_unlock_sock(sk);
--
1.7.0.4
^ permalink raw reply related
* [PATCHv3 2/2] Bluetooth: timer check sk is not owned before freeing
From: Emeltchenko Andrei @ 2010-10-29 13:43 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1288359781-4059-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
In timer context we might delete l2cap channel used by krfcommd.
The check makes sure that sk is not owned. If sk is owned we
restart timer for HZ/5.
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
net/bluetooth/l2cap.c | 32 ++++++++++++++++++++------------
1 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index b1344d8..c67b3c6 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -83,6 +83,18 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn,
static int l2cap_ertm_data_rcv(struct sock *sk, struct sk_buff *skb);
/* ---- L2CAP timers ---- */
+static void l2cap_sock_set_timer(struct sock *sk, long timeout)
+{
+ BT_DBG("sk %p state %d timeout %ld", sk, sk->sk_state, timeout);
+ sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
+}
+
+static void l2cap_sock_clear_timer(struct sock *sk)
+{
+ BT_DBG("sock %p state %d", sk, sk->sk_state);
+ sk_stop_timer(sk, &sk->sk_timer);
+}
+
static void l2cap_sock_timeout(unsigned long arg)
{
struct sock *sk = (struct sock *) arg;
@@ -92,6 +104,14 @@ static void l2cap_sock_timeout(unsigned long arg)
bh_lock_sock(sk);
+ if (sock_owned_by_user(sk)) {
+ /* sk is owned by user. Try again later */
+ l2cap_sock_set_timer(sk, HZ / 5);
+ bh_unlock_sock(sk);
+ sock_put(sk);
+ return;
+ }
+
if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONFIG)
reason = ECONNREFUSED;
else if (sk->sk_state == BT_CONNECT &&
@@ -108,18 +128,6 @@ static void l2cap_sock_timeout(unsigned long arg)
sock_put(sk);
}
-static void l2cap_sock_set_timer(struct sock *sk, long timeout)
-{
- BT_DBG("sk %p state %d timeout %ld", sk, sk->sk_state, timeout);
- sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
-}
-
-static void l2cap_sock_clear_timer(struct sock *sk)
-{
- BT_DBG("sock %p state %d", sk, sk->sk_state);
- sk_stop_timer(sk, &sk->sk_timer);
-}
-
/* ---- L2CAP channels ---- */
static struct sock *__l2cap_get_chan_by_dcid(struct l2cap_chan_list *l, u16 cid)
{
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH] Fix handling nco affiliation fields
From: Johan Hedberg @ 2010-10-29 13:53 UTC (permalink / raw)
To: Lukasz Pawlik; +Cc: linux-bluetooth
In-Reply-To: <1288344619-18133-1-git-send-email-lucas.pawlik@gmail.com>
Hi Lukasz,
On Fri, Oct 29, 2010, Lukasz Pawlik wrote:
> Previously when contact record was divided into separate replies,
> phonebook-tracker would use first reply and merge only phone numbers,
> addresses and emails from next. Now it's merging all fields dependent on the
> nco:hasAffiliation as well.
> ---
> plugins/phonebook-tracker.c | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)
You'll need to fix the coding style before this goes upstream:
> +static void add_affiliation(char **field, const char *value)
> +{
> + if(strlen(*field) != 0 || value == NULL || strlen(value) == 0)
Space between "if" and "(". Also, I'd prefer > 0 instead of != 0 for the
first strlen check. Then, you're also using spaces instead of tabs for
the indentation of all code in this patch. Did you forget to switch your
editor into bluez/kernel mode?
Johan
^ permalink raw reply
* Re: [PATCH] Fix issue when setting limited discoverable mode
From: Johan Hedberg @ 2010-10-29 13:57 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1288357083-24664-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Fri, Oct 29, 2010, Luiz Augusto von Dentz wrote:
> When setting limited discoverable mode it will always switch to
> discoverable on adapter_mode_changed which doesn't match the pending mode
> requested.
>
> To fix this MODE_LIMITED was removed since it didn't represent a real
> scan mode but a discoverable mode so that limited discoverable and
> general discoverable are now both represented by MODE_DISCOVERABLE.
>
> Also limited bits are now set at same point the timeout start and removed
> when mode change to something different then discoverable or when
> discoverable timeout is removed permanently (set to 0).
> ---
> src/adapter.c | 76 +++++++++++++++++++++++++-------------------------------
> src/hcid.h | 1 -
> 2 files changed, 34 insertions(+), 43 deletions(-)
The patch has been pushed upstream. Thanks.
Johan
^ permalink raw reply
* [PATCHv2] Fix handling nco affiliation fields
From: Lukasz Pawlik @ 2010-10-29 14:04 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lukasz Pawlik
Previously when contact record was divided into separate replies,
phonebook-tracker would use first reply and merge only phone numbers,
addresses and emails from next. Now it's merging all fields dependent on the
nco:hasAffiliation as well.
---
plugins/phonebook-tracker.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 96290a4..30be895 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -982,6 +982,15 @@ static void add_phone_number(struct phonebook_contact *contact,
contact->numbers = g_slist_append(contact->numbers, number);
}
+static void add_affiliation(char **field, const char *value)
+{
+ if (strlen(*field) > 0 || value == NULL || strlen(value) == 0)
+ return;
+
+ g_free(*field);
+ *field = g_strdup(value);
+}
+
static struct phonebook_email *find_email(GSList *emails, const char *address,
int type)
{
@@ -1196,6 +1205,13 @@ add_numbers:
g_free(home_addr);
g_free(work_addr);
+ /* Adding fields connected by nco:hasAffiliation - they may be in
+ * separate replies */
+ add_affiliation(&contact->title, reply[33]);
+ add_affiliation(&contact->company, reply[22]);
+ add_affiliation(&contact->department, reply[23]);
+ add_affiliation(&contact->role, reply[24]);
+
DBG("contact %p", contact);
/* Adding contacts data to wrapper struct - this data will be used to
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCHv2] Fix handling nco affiliation fields
From: Johan Hedberg @ 2010-10-29 14:40 UTC (permalink / raw)
To: Lukasz Pawlik; +Cc: linux-bluetooth
In-Reply-To: <1288361066-21203-1-git-send-email-lucas.pawlik@gmail.com>
Hi Lukasz,
On Fri, Oct 29, 2010, Lukasz Pawlik wrote:
> Previously when contact record was divided into separate replies,
> phonebook-tracker would use first reply and merge only phone numbers,
> addresses and emails from next. Now it's merging all fields dependent on the
> nco:hasAffiliation as well.
> ---
> plugins/phonebook-tracker.c | 16 ++++++++++++++++
> 1 files changed, 16 insertions(+), 0 deletions(-)
Pushed upstream with a couple of minor tweaks.
Johan
^ permalink raw reply
* Re: HFP Pulseaudio Source destroyed "too quickly" at the end of a call
From: Peter Dons Tychsen @ 2010-10-29 16:19 UTC (permalink / raw)
To: Thomas Wälti; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikQqPqJdFgLpUrKiqyAC0HNUFnj+8NtbPxazY18@mail.gmail.com>
On Wed, 2010-10-27 at 22:03 +0200, Thomas Wälti wrote:
> All works well except when ending the recording of Bluetooth
> Conversations: Once a party hangs up the call, the PulseAudio source
> and sink disappear before I can stop GStreamer recording (I'm
> listening to D-Bus events). Unfortunately, this causes my GStreamer
> pipeline to crash.
There are many reasons (especially for wireless device) that the source
or sink could disappear at any time. Audio disappearing before or after
the accompanying signaling furthermore a classic scenario which apps
should be built to handle. So fixing the race (which i am not sure is
really a race) does not seem like the correct solution. Trying to fix
such "races" can lead to inefficiency by using unneeded timers and
spin-locks, and will not provide robustness, as the audio streams can
disappear for other reasons anyway.
Maybe the crash is more interesting. What actually crashes when this
happens, and why?
Thanks,
/pedro
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Alan Cox @ 2010-10-29 16:22 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl
Cc: Arnd Bergmann, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <AANLkTinh3D8V8UaN8FyCouwUQ98A4RRWEHXU7Tr9XMbs@mail.gmail.com>
> > Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
>
> We also need the ldisc code to handle events from FM and GPS and since
> that is chip specific we cannot add that to the generic hci_ldisc
> code.
Agreed - it's a different protocol.
> I agree that we might run into problems if two drivers try to register
> the same line discipline. It might then be better to introduce a new
If your ldisc is written properly it shouldn't matter. Each tty has a
private ldisc pointer to keep the per ldisc instance data.
> line discipline then even though that could cause other problems. I do
> not know if it is possible to add a condition in Kconfig otherwise so
> the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> selected.
Not sure I follow the concern here ?
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Alan Cox @ 2010-10-29 16:24 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTinj93zAXJAipB3HjE62Xa1Z=9k9gPBxaT=+MLtg@mail.gmail.com>
> The reason is that the work is generated so often that a work is not
> finished before next work of same type comes. This is especially true
> for transmit and receive. Then I get 0 back when queuing the work and
> there is no real way to solve it from what I can see than to allocate
> new work structures every time.
So if that is the case what bounds your memory usage - can a busy box end
up with thousands of work queue slos used ? It sounds like your model is
perhaps wrong - if there is a continual stream of work maybe you should
simply have a kernel thread to handle it if it cannot be deferred
- remember ldisc code is able to sleep in most paths.
^ permalink raw reply
* Re: [PATCH 3/6] Bluetooth: Implement the first SMP commands
From: Anderson Lizardo @ 2010-10-29 20:28 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: Luiz Augusto von Dentz, Ville Tervo, Anderson Briglia,
linux-bluetooth@vger.kernel.org, Vinicius Costa Gomes
In-Reply-To: <20101028091733.GB15997@vigoh>
On Thu, Oct 28, 2010 at 5:17 AM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Yep, we may need a new smp.c file.
It seems that to support multiple C files, there should not be a .c
file with the same basename as the final module. This means we would
need to rename l2cap.c to something else.
Gustavo, I remember you had some patches to split l2cap.c... Do you
still intend to apply them? If so, having a separate "smp.c" file will
be much easier.
For now, in our branch we renamed "l2cap.c" to "l2cap_core.c" and
added the following to net/bluetooth/Makefile:
l2cap-objs := l2cap_core.o smp.o
Regards,
--
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 3/6] Bluetooth: Implement the first SMP commands
From: Gustavo F. Padovan @ 2010-10-29 20:45 UTC (permalink / raw)
To: Anderson Lizardo
Cc: Luiz Augusto von Dentz, Ville Tervo, Anderson Briglia,
linux-bluetooth@vger.kernel.org, Vinicius Costa Gomes
In-Reply-To: <AANLkTiny4xmZ3KBSvUCrpmQacD8m7dCRQymeJkc_hrDb@mail.gmail.com>
Hi Anderson,
* Anderson Lizardo <anderson.lizardo@openbossa.org> [2010-10-29 16:28:32 -0400]:
> On Thu, Oct 28, 2010 at 5:17 AM, Gustavo F. Padovan
> <padovan@profusion.mobi> wrote:
> > Yep, we may need a new smp.c file.
>
> It seems that to support multiple C files, there should not be a .c
> file with the same basename as the final module. This means we would
> need to rename l2cap.c to something else.
>
> Gustavo, I remember you had some patches to split l2cap.c... Do you
> still intend to apply them? If so, having a separate "smp.c" file will
> be much easier.
Yes, but I still have to discuss one part of the patch with Marcel.
>
> For now, in our branch we renamed "l2cap.c" to "l2cap_core.c" and
> added the following to net/bluetooth/Makefile:
I'm fine with it, that would done anyway in the future.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 2/6] Bluetooth: fix receiving L2CAP packets over LE
From: Gustavo F. Padovan @ 2010-10-29 20:50 UTC (permalink / raw)
To: Vinicius Gomes
Cc: Ville Tervo, ext Anderson Briglia,
linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTimTUFK9id6kQyggt_xWsB-RspwbPoB4m39XU2cP@mail.gmail.com>
Hi,
* Vinicius Gomes <vinicius.gomes@openbossa.org> [2010-10-29 09:41:55 -0400]:
> Hi Ville,
>
> On Fri, Oct 29, 2010 at 6:44 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > Hi Anderson,
> >
> > On Sat, Oct 23, 2010 at 01:56:56AM +0200, ext Anderson Briglia wrote:
> >> From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> >>
> >> As L2CAP packets coming over LE don't have any more encapsulation,
> >> other than L2CAP, we are able to process them as soon as they arrive.
> >
> > Why is this change needed? Was something broken without this patch?
> >
>
> This change is needed because without it the receiving side would
> always think that it was receiving continuation frames.
>
> As the flags parameter is zero, it would fall into the "} else {"
> condition, and because no frame was received before, conn->rx_len
> would be zero and the frame would be discarded. Without this patch I
> was seeing those "Unexpected continuation frame ..." messages on the
> receiving side.
>From what I understood the flags are only for ACL connections and LE
links doesn't have fragmentation, right? So we don't need to check flags
here, actually it seems we don't have flags for LE like ACL.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCHv3 2/2] Bluetooth: timer check sk is not owned before freeing
From: Gustavo F. Padovan @ 2010-10-29 21:17 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
In-Reply-To: <1288359781-4059-3-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-10-29 16:43:01 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> In timer context we might delete l2cap channel used by krfcommd.
> The check makes sure that sk is not owned. If sk is owned we
> restart timer for HZ/5.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
> net/bluetooth/l2cap.c | 32 ++++++++++++++++++++------------
> 1 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index b1344d8..c67b3c6 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -83,6 +83,18 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn,
> static int l2cap_ertm_data_rcv(struct sock *sk, struct sk_buff *skb);
>
> /* ---- L2CAP timers ---- */
> +static void l2cap_sock_set_timer(struct sock *sk, long timeout)
> +{
> + BT_DBG("sk %p state %d timeout %ld", sk, sk->sk_state, timeout);
> + sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
> +}
> +
> +static void l2cap_sock_clear_timer(struct sock *sk)
> +{
> + BT_DBG("sock %p state %d", sk, sk->sk_state);
> + sk_stop_timer(sk, &sk->sk_timer);
> +}
> +
> static void l2cap_sock_timeout(unsigned long arg)
> {
> struct sock *sk = (struct sock *) arg;
> @@ -92,6 +104,14 @@ static void l2cap_sock_timeout(unsigned long arg)
>
> bh_lock_sock(sk);
>
> + if (sock_owned_by_user(sk)) {
> + /* sk is owned by user. Try again later */
> + l2cap_sock_set_timer(sk, HZ / 5);
> + bh_unlock_sock(sk);
> + sock_put(sk);
You can't do a sock_put() here, you have to keep the referencee to the
socket while the timer is enabled.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCHv3 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
From: Gustavo F. Padovan @ 2010-10-29 21:27 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
In-Reply-To: <1288359781-4059-2-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-10-29 16:43:00 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> Check that socket sk is not locked in user process before removing
> l2cap connection handler.
>
> lock_sock and release_sock do not hold a normal spinlock directly but
> instead hold the owner field. This means bh_lock_sock can still execute
> even if the socket is "locked". More info can be found here:
> http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
>
> krfcommd kernel thread may be preempted with l2cap tasklet which remove
> l2cap_conn structure. If krfcommd is in process of sending of RFCOMM reply
> (like "RFCOMM UA" reply to "RFCOMM DISC") then kernel crash happens.
>
> ...
> [ 694.175933] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 694.184936] pgd = c0004000
> [ 694.187683] [00000000] *pgd=00000000
> [ 694.191711] Internal error: Oops: 5 [#1] PREEMPT
> [ 694.196350] last sysfs file: /sys/devices/platform/hci_h4p/firmware/hci_h4p/loading
> [ 694.260375] CPU: 0 Not tainted (2.6.32.10 #1)
> [ 694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
> [ 694.270721] LR is at 0xd7017303
> ...
> [ 694.525085] Backtrace:
> [ 694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap]) from [<c02f2cc8>] (sock_sendmsg+0xb8/0xd8)
> [ 694.537292] [<c02f2c10>] (sock_sendmsg+0x0/0xd8) from [<c02f3044>] (kernel_sendmsg+0x48/0x80)
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
> net/bluetooth/l2cap.c | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 6f931cc..b1344d8 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -3078,6 +3078,14 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> break;
>
> default:
> + /* don't delete l2cap channel if sk is owned by user */
> + if (sock_owned_by_user(sk)) {
> + sk->sk_state = BT_DISCONN;
> + l2cap_sock_clear_timer(sk);
> + l2cap_sock_set_timer(sk, HZ);
> + break;
> + }
> +
> l2cap_chan_del(sk, ECONNREFUSED);
> break;
> }
> @@ -3283,6 +3291,15 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
>
> sk->sk_shutdown = SHUTDOWN_MASK;
>
> + /* don't delete l2cap channel if sk is owned by user */
> + if (sock_owned_by_user(sk)) {
> + sk->sk_state = BT_DISCONN;
> + l2cap_sock_clear_timer(sk);
> + l2cap_sock_set_timer(sk, HZ);
> + bh_unlock_sock(sk);
> + return 0;
> + }
> +
> l2cap_chan_del(sk, ECONNRESET);
> bh_unlock_sock(sk);
>
> @@ -3305,6 +3322,15 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
> if (!sk)
> return 0;
>
> + /* don't delete l2cap channel if sk is owned by user */
> + if (sock_owned_by_user(sk)) {
> + sk->sk_state = BT_DISCONN;
> + l2cap_sock_clear_timer(sk);
> + l2cap_sock_set_timer(sk, HZ);
1 second is too much. Change it to HZ/5 as well. I think it is a
reasonable value.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH] bluetooth: Fix NULL pointer dereference issue
From: Marcel Holtmann @ 2010-10-29 21:43 UTC (permalink / raw)
To: Yuri Ershov
Cc: davem, padovan, jprvita, linux-bluetooth, ville.tervo,
andrei.emeltchenko
In-Reply-To: <456b20879a6fc3e434463389ef25c4d5cf8803d4.1288262806.git.ext-yuri.ershov@nokia.com>
Hi Yuri,
> This patch fixes NULL pointer dereference at running test with
> connect-transfer-disconnect in loop. The problem conditions are the
> following: there are 2 BT devices. The first one listens and
> receives (l2test -r), the second one makes "connect-disconnect-
> connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
> After some time this will cause the race between functions
> bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
> sets the socket state to BT_CLOSED, unlinks and kills the socket
> in the middle of bt_accept_dequeue, then at running the removed code
> kernel oops appears.
it could be that we have this in here for RFCOMM. Can you retest it with
this with using RFCOMM and see if we get dangling RFCOMM sockets. You
need to test both directions, incoming and outgoing.
Regards
Marcel
^ permalink raw reply
* Re: [stable] [PATCH 0/1] Bluetooth: fix crash in L2CAP
From: Greg KH @ 2010-10-29 22:05 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth, stable, linux-kernel
In-Reply-To: <20101025111530.GC7721@vigoh>
On Mon, Oct 25, 2010 at 09:15:30AM -0200, Gustavo F. Padovan wrote:
> Hi Greg,
>
> * Greg KH <greg@kroah.com> [2010-10-21 06:35:07 -0700]:
>
> > On Thu, Oct 21, 2010 at 03:19:52AM -0200, Gustavo F. Padovan wrote:
> > > Hi Greg,
> > >
> > > The following patch is good for 2.6.36.1. It arrived too late in linux-bluetooth
> > > and we didn't had time to put it into 2.6.36. It fixes a serious crash into
> > > the L2CAP layer. The issue isn't in 2.6.35 and below.
> >
> > It needs to get into Linus's tree before I can accept it into the
> > -stable trees. Please get it there and then send stable@kernel.org the
> > git commit id and I will add it.
>
> It is now on Linus' tree, sorry for doing this wrong first time. It was
> my first report to stable. ;)
No problem.
> commit d793fe8caa3911e6a1e826b45d4ee00d250cdec8
Now queued up, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-30 0:01 UTC (permalink / raw)
To: Alan Cox
Cc: Par-Gunnar Hjalmdahl, linus.walleij, linux-bluetooth,
linux-kernel, Lukasz.Rymanowski
In-Reply-To: <20101029172214.25b14c2a@pyx>
On Friday 29 October 2010, Alan Cox wrote:
> > line discipline then even though that could cause other problems. I do
> > not know if it is possible to add a condition in Kconfig otherwise so
> > the CG2900 ldisc cannot be active while the "normal" ldisc driver is
> > selected.
>
> Not sure I follow the concern here ?
It's about the ldisc number. Both bluetooth and cg2900 register themselves
to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
The solution would not be to make the two ldisc implementations mutually
exclusive, but to make cg2900 use a new ldisc number, e.g. N_HCI_CG2900.
However, I'm still not convinced that this is actually necessary, we might
be able to add hooks into the existing N_HCI implementation for the
extensions in cg2900.
Arnd
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-30 0:09 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl
Cc: Alan Cox, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <AANLkTimyR7f=PdQbxxBLCnznTA4PT1TWEFiNX3C_qaZo@mail.gmail.com>
On Friday 29 October 2010, Par-Gunnar Hjalmdahl wrote:
> I might have been a bit too quick there.
> The actual channel matching and packet creation is done in hci_h4.c
> while ldisc registration is done in hci_ldisc.c.
> So it might to be enough to create a new hci_h4-cg2900.c (or similar
> name) that can separate the right channels.
That sounds good, but would that still be h4?
There are currently six UART protocols that have drivers in Linux:
H4, bcsp, 3Weire, h4ds, ll and ath3k.
Can cg2900 be simply another one of those, or is it different
from the others?
> We must however do changes
> to hci_ldisc as well since it seems to always register to the
> Bluetooth stack here, which we definitely don't want since that is
> handled by btcg2900.c.
Can you elaborate? You said earlier that cg2900 is a standard HCI
with some extensions. If that's the case, why do you need your own
btcg2900 driver to handle bluetooth instead of the regular hci code?
> Also note that this ldisc issue is only valid when using UART as
> transport. We will also support SPI and then we will probably run into
> completely new, interesting problems. :-)
Is the link layer on SPI different from the UART variant? Maybe you cna
just add a SPI TTY driver if that doesn't exist yet and bind the same
ldisc to the SPI device.
Arnd
^ permalink raw reply
* pull request: bluetooth-2.6 2010-10-30
From: Gustavo F. Padovan @ 2010-10-30 3:15 UTC (permalink / raw)
To: linville; +Cc: linux-bluetooth, linux-wireless
Hi John,
The following batch contains some bugfixes for 2.6.37. A fix for unaligned
access in L2CAP, a Kconfig error, and two fixes related to security of
the Bluetooth links. There is also a one line patch from Matthew Garret,
that enables USB autosuspend for btusb module, which shall be completely
safe. Please pull, thanks.
The following changes since commit c1758012971e0410790b2bc96a77e26d7b286593:
igb: Fix unused variable warning. (2010-10-27 19:43:55 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth-2.6.git master
Gustavo F. Padovan (1):
Bluetooth: fix endianness conversion in L2CAP
Johan Hedberg (1):
Bluetooth: Fix non-SSP auth request for HIGH security level sockets
Luiz Augusto von Dentz (1):
Bluetooth: fix not setting security level when creating a rfcomm session
Matthew Garrett (1):
Bluetooth: Enable USB autosuspend by default on btusb
Randy Dunlap (1):
Bluetooth: fix hidp kconfig dependency warning
steven miao (1):
bluetooth: fix unaligned access to l2cap conf data
drivers/bluetooth/btusb.c | 2 ++
net/bluetooth/hci_event.c | 6 ++++++
net/bluetooth/hidp/Kconfig | 2 +-
net/bluetooth/l2cap.c | 8 ++++----
net/bluetooth/rfcomm/core.c | 13 ++++++++++---
5 files changed, 23 insertions(+), 8 deletions(-)
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
^ permalink raw reply
* Re: [PATCH 2/6] Bluetooth: fix receiving L2CAP packets over LE
From: Vinicius Gomes @ 2010-10-30 3:31 UTC (permalink / raw)
To: Gustavo F. Padovan
Cc: Ville Tervo, ext Anderson Briglia,
linux-bluetooth@vger.kernel.org
In-Reply-To: <20101029205033.GB14961@vigoh>
Hi Gustavo,
On Fri, Oct 29, 2010 at 5:50 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi,
>
> * Vinicius Gomes <vinicius.gomes@openbossa.org> [2010-10-29 09:41:55 -0400]:
>
>> Hi Ville,
>>
>> On Fri, Oct 29, 2010 at 6:44 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
>> > Hi Anderson,
>> >
>> > On Sat, Oct 23, 2010 at 01:56:56AM +0200, ext Anderson Briglia wrote:
>> >> From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
>> >>
>> >> As L2CAP packets coming over LE don't have any more encapsulation,
>> >> other than L2CAP, we are able to process them as soon as they arrive.
>> >
>> > Why is this change needed? Was something broken without this patch?
>> >
>>
>> This change is needed because without it the receiving side would
>> always think that it was receiving continuation frames.
>>
>> As the flags parameter is zero, it would fall into the "} else {"
>> condition, and because no frame was received before, conn->rx_len
>> would be zero and the frame would be discarded. Without this patch I
>> was seeing those "Unexpected continuation frame ..." messages on the
>> receiving side.
>
> From what I understood the flags are only for ACL connections and LE
> links doesn't have fragmentation, right? So we don't need to check flags
> here, actually it seems we don't have flags for LE like ACL.
>
Yeah, that is what the patch tries to solve. I was just trying to make
clear why the old code doesn't work (seems that I didn't do a good job
;-) .
> --
> Gustavo F. Padovan
> ProFUSION embedded systems - http://profusion.mobi
>
Cheers,
--
Vinicius
^ permalink raw reply
* Re: pull request: bluetooth-2.6 2010-10-30
From: Luis R. Rodriguez @ 2010-10-30 6:29 UTC (permalink / raw)
To: Gustavo F. Padovan, Matthew Garrett
Cc: linville, linux-bluetooth, linux-wireless
In-Reply-To: <20101030031503.GB24188@vigoh>
On Fri, Oct 29, 2010 at 8:15 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi John,
>
> The following batch contains some bugfixes for 2.6.37. A fix for unaligned
> access in L2CAP, a Kconfig error, and two fixes related to security of
> the Bluetooth links. There is also a one line patch from Matthew Garret,
> that enables USB autosuspend for btusb module, which shall be completely
> safe. Please pull, thanks.
> Matthew Garrett (1):
> Bluetooth: Enable USB autosuspend by default on btusb
BTW, is this the thing that powertop nags about?
Luis
^ permalink raw reply
* [PATCH] bluetooth: bnep: fix information leak to userland
From: Vasiliy Kulikov @ 2010-10-30 14:26 UTC (permalink / raw)
To: kernel-janitors
Cc: Marcel Holtmann, Gustavo F. Padovan, David S. Miller,
Eric Dumazet, Thadeu Lima de Souza Cascardo, Tejun Heo,
Jiri Kosina, linux-bluetooth, netdev, linux-kernel
Structure bnep_conninfo is copied to userland with the field "device"
that has the last elements unitialized. It leads to leaking of
contents of kernel stack memory.
Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
Compile tested.
net/bluetooth/bnep/core.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index f10b41f..5868597 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -648,6 +648,7 @@ int bnep_del_connection(struct bnep_conndel_req *req)
static void __bnep_copy_ci(struct bnep_conninfo *ci, struct bnep_session *s)
{
+ memset(ci, 0, sizeof(*ci));
memcpy(ci->dst, s->eh.h_source, ETH_ALEN);
strcpy(ci->device, s->dev->name);
ci->flags = s->flags;
--
1.7.0.4
^ permalink raw reply related
* [PATCH] bluetooth: cmtp: fix information leak to userland
From: Vasiliy Kulikov @ 2010-10-30 14:26 UTC (permalink / raw)
To: kernel-janitors
Cc: Marcel Holtmann, Gustavo F. Padovan, David S. Miller,
Eric Dumazet, linux-bluetooth, netdev, linux-kernel
Structure cmtp_conninfo is copied to userland with some padding fields
unitialized. It leads to leaking of contents of kernel stack memory.
Signed-off-by: Vasiliy Kulikov <segooon@gmail.com>
---
Compile tested.
net/bluetooth/cmtp/core.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index ec0a134..8e5f292 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -78,6 +78,7 @@ static void __cmtp_unlink_session(struct cmtp_session *session)
static void __cmtp_copy_session(struct cmtp_session *session, struct cmtp_conninfo *ci)
{
+ memset(ci, 0, sizeof(*ci));
bacpy(&ci->bdaddr, &session->bdaddr);
ci->flags = session->flags;
--
1.7.0.4
^ permalink raw reply related
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