Re: [PATCH] Bluetooth: Incorrect locking when sending data in softirq

[Peter Hurley <peter@hurleysoftware.com>]

On 10/15/2014 06:23 PM, Alexander Aring wrote:
> Hi Peter,
> 
> On Wed, Oct 15, 2014 at 05:58:40PM -0400, Peter Hurley wrote:
>> On 10/15/2014 05:53 PM, Alexander Aring wrote:
>>> Hi Peter,
>>>
>>> On Wed, Oct 15, 2014 at 04:46:24PM -0400, Peter Hurley wrote:
>>> ...
>>>>
>>>> That's happening because 6lowpan.c:send_mcast_pkt() is disabling
>>>> interrupts with the read_lock_irqsave() before calling send_pkt().
>>>>
>>>> It's unclear browsing through the lowpan driver why the
>>>> irqflags save/restore read_lock flavors are being used; is there a
>>>> place where the bluetooth core is calling the driver in atomic
>>>> context (ie., where interrupts are disabled)?
>>>>
>>>
>>> In my opinion bt_xmit is called in atomic context. Make the stacktrace
>>> sense now?
>>
>> I don't think so.
>>
>> The network layer is very careful about keeping interrupts enabled
>> and calling network drivers from softirq, so that spin_lock_bh() is
>> typically all that's required.
>>
>>> It's the callback 'ndo_start_xmit' of 'struct net_device_ops' [0].
>>
>> The send_mcast_pkt() isn't showing in the stack trace because it's
>> being inlined; and send_mcast_pkt() is definitely disabling interrupts
>> and calling send_pkt().
>>
> 
> Thanks for explanation, now I know a little more about different context
> and handling of them. I need to say I am not an expert into this. :-)
> 
> 
> Nevertheless, I found something in "Documentation/networking/netdevices.txt"
> about context information for 'ndo_start_xmit':
> 
> <qoute>
> ...
> Context: Process with BHs disabled or BH (timer),
>          will be called with interrupts disabled by netconsole.
> ...
> </qoute>
> 
> Now I am not sure if this helps us and what exactly this means. But "...
> interrupts disabled..." is this atomic now?

Local interrupts disabled is only atomic for the same cpu. Which would mean
that spin_lock_bh() wouldn't be appropriate in the ndo_start_xmit() handler.
However, this driver doesn't support netconsole because it doesn't support
the netpoll api, so it won't be called from that context.

Regards,
Peter Hurley