On Mon, 28 May 2012, Andrei Emeltchenko wrote:
Hi Mat,
On Mon, May 28, 2012 at 10:24:56AM +0300, Andrei Emeltchenko wrote:
The risk is not memory leaks - if the other device closes the ACL,
then the A2MP channel will be cleaned up. But if you had two BlueZ
devices with A2MP connected to each other, the ACL would never close
as long as the devices remained powered and in range! Nothing would
trigger the flush as long as the hci_conn is held.
Does it indicate that we have problem with rather freeing amp_mgr?
It looks like the amp_mgr will be freed if the channel is closed.
There's nothing that will close the channel except for:
* Remote device disconnects the ACL
* Remote device goes out of range
* Local BR/EDR controller is powered down or reset
I think the checks for hci_conn_put are necessary, even if they are
ugly.
I think that if I have hci_conn_hold when creating channel and
hci_conn_put when closing channel that does not differ from the case when
I do not have hci_conn_put/hold.
If there was something that closed the A2MP channel, there would be
no difference. But nothing closes the A2MP channel.
I actually was thinking about timers which could do clean up.
So you are probably referring to the case when hci_conn needs to be closed
but cannot because it has hci_conn_hold in a2mp channel?
Yes.
This also means that we still have L2CAP channel (a2mp) hanging.
This seems right to me if I did not miss something.
Since A2MP is a fixed channel, it is not opened and closed like
typical L2CAP channels. Consider this scenario:
1. Remote device decides to connect
2. ACL is created
3. Remote device sends A2MP discover
4. A2MP channel is set up, AMP manager is created, hci_conn_hold for
A2MP channel
5. Remote device connects L2CAP channel over BR/EDR. hci_conn_hold
for the data channel
6. A2MP communication to set up physical link
7. L2CAP channel move to AMP controller
8. Data sent over AMP.
9. L2CAP channel disconnected, hci_conn_put for the data channel
10. Device stays in range. hci_conn reference count is not 0, so
ACL is not closed.
My concern here if A2MP does not reference hci_conn can hci_conn be
removed while we still have A2MP chan in some situation?
hci_conn_hold and hci_conn_put are not reference counts on the
hci_conn structure in the typical way. They are reference counts for
the ACL. When you do the last hci_conn_put, the ACL is disconnected
after a timeout.
With or without hci_conn_hold for the A2MP channel, the ACL can
disappear at any time. The AMP manager must deal with that without
crashing like it does in your trace.
I just tested code with PTS and got one crash:
<------8<--------------------------------------------------------------------
| [ 275.100760] [5] hci_chan_del: hci0 conn f3ff0800 chan e818e540
| [ 275.109610] [19] l2cap_send_rr_or_rnr: chan e806b800, poll 0
| [ 275.127922] [19] l2cap_send_sframe: chan e806b800, control f5ef3ed8
| [ 275.133532] [19] l2cap_send_sframe: reqseq 2, final 0, poll 0, super 0
| [ 275.173033] [5] hci_conn_del: hci0 conn f3ff0800 handle 11
| [ 275.181131] [5] hci_chan_list_flush: conn f3ff0800
| [ 275.202645] [5] amp_mgr_put: mgr e8260600
<------8<--------------------------------------------------------------------
Here we deleted the last L2CAP channel
Ok. The AMP manager should not attempt to send anything after this.
<------8<------------------------------------------------------------------
| [ 275.295020] BUG: unable to handle kernel NULL pointer dereference at
| (null)
| [ 275.298791] IP: [<f8362e6c>] l2cap_do_send+0x1c/0xa0 [bluetooth]
| [ 275.298791] *pde = 00000000
| [ 275.298791] Oops: 0000 [#1] SMP
<------8<------------------------------------------------------------------
Then we got the crash probably hci_conn deleted faster then we get reply
from AMP controller
This looks like a crash when the AMP manager tries to send something.
It should not be trying to send anything at this point.
Once the AMP manager gets a state change callback with BT_CLOSED or a
a close callback, amp_mgr->l2cap_conn and amp_mgr->a2mp_chan are
invalid - even if the AMP manager has not been freed.
<------8<---------------------------------------------------------------------
| [ 275.298791] EIP: [<f8362e6c>] l2cap_do_send+0x1c/0xa0 [bluetooth]
| SS:ESP 0068:f5ef3e58
| [ 275.298791] CR2: 0000000000000000
| [ 276.553113] Bluetooth: hci1 command tx timeout
| [ 276.553706] ---[ end trace b08f23ddb2b49a2e ]---
| [ 276.577494] [30] hci_queue_cb: Queue cmd ed3d5540 opt e8260600
| [ 276.723885] [30] l2cap_segment_sdu: chan e806b800, msg f5919e94, len 22
| [ 276.790990] BUG: unable to handle kernel NULL pointer dereference at
| 00000010
| [ 276.794865] IP: [<f83694dd>] l2cap_chan_send+0x28d/0xb40 [bluetooth]
<------8<---------------------------------------------------------------------
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
