Hi Marcel,
On Thu, Aug 12, 2010 at 2:27 PM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> Hi Andrei,
>
>> >> Check that socket sk is not locked in user process before removing
>> >> l2cap connection handler.
>> >>
>> >> 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)
>> >> ...
>> >>
>> >> Modified version after comments of Gustavo F. Padovan <gustavo@xxxxxxxxxxx>
>> >>
>> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
>> >
>> > the patch seems to be fine, but I have some extra questions/concerns.
>> >
>> > Who is now taking care of deleting the channel in this case? I think you
>> > need to show that the code flow is still valid.
>>
>> I have the other version of the I have sent already to ML where I use
>> standard l2cap
>> timer which will delete channel like the code below:
>>
>> + /* 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;
>> + }
>>
>> > Also the question is how RFCOMM can send this UA or DISC with not
>> > locking the socket. The comment on l2cap_chan_del clearly states that
>> > the socket must be locked and inside L2CAP we do that. Is RFCOMM maybe
>> > at fault here?
>>
>> when RFCOMM send packets it lock_sock which marks sk as owned
>> sk->sk_lock.owned = 1;
>> and then can be preempted.
>
> I need a new patch with a proper and most likely lengthy commit message
> explaining every single detail here. Since right now you lost me.
I am sending other version of the patch with l2cap timer which clears
L2CAP channels.
This way we are sure that channel will be deleted despite the
technique does not look
perfect...
BTW: The crash is easy to reproduce on ARM with our test tools :-)
Regards,
Andrei
--
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
