This patch aims to reduce the amount of copying in the receive path when
decompressing by checking for headroom and calling pskb_expand_head if required.
Another benefit of this is that the skb pointer passed to the decompression
routine will be the same skb after decompression. This will be a step towards
freeing the skb within the receive function and not within all the error paths
of the decompression routine.
Bluetooth and 802.15.4 have been changed to ensure that the skb isn't shared
before calling the decompression routine as this is a requirement of
pskb_expand_head.
v1 -> v2
Use const int for new headroom size.
Remove blank line.
Use skb_unshare instead of skb_copy_expand.
Use consume_skb.
v2 -> v3
Remove cases where we are trying to free a NULL skb.
v3 -> v4
Now uses skb_share_check to ensure the skb isn't shared before decompressing
v4 -> v5
Remove skb_share_check from 802.15.4 lowpan_rcv as it's already done at the
top of the function.
v5 -> v6
Use skb_cow instead of pskb_expand_head, adjusted commit message to reflect
this.
Moved skb_share_check to start of recv_pkt function for bluetooth.
Martin Townsend (1):
6lowpan: Use skb_cow in IPHC decompression.
net/6lowpan/iphc.c | 47 +++++++++++++++++++++--------------------------
net/bluetooth/6lowpan.c | 4 ++++
2 files changed, 25 insertions(+), 26 deletions(-)
--
1.9.1
Hi Martin,
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> skb_cow which will check to see if there is enough headroom
> first to ensure it's only done if necessary and will handle
> alignment issues for cache.
> As skb_cow uses pskb_expand_head we ensure the skb isn't shared from
> bluetooth and ieee802.15.4 code that use the IPHC decompression.
>
> Signed-off-by: Martin Townsend <[email protected]>
> ---
> net/6lowpan/iphc.c | 47 +++++++++++++++++++++--------------------------
> net/bluetooth/6lowpan.c | 4 ++++
> 2 files changed, 25 insertions(+), 26 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
Hi Jukka,
On Tue, Oct 14, 2014 at 11:35:05AM +0300, Jukka Rissanen wrote:
> Hi Alex,
>
> On ma, 2014-10-13 at 19:22 +0200, Alexander Aring wrote:
> > Hi Jukka,
> >
> > sorry.
> >
> > I was a little too fast here, because I am sure now this should solve your
> > lockdep issue.
>
> Unfortunately this patch did not help with the issue.
>
mhh, ok.
> >
> > On Mon, Oct 13, 2014 at 07:11:11PM +0200, Alexander Aring wrote:
> > > Hi Jukka,
> > >
> > > On Mon, Oct 13, 2014 at 06:09:14PM +0300, Jukka Rissanen wrote:
> > > > Hi Martin,
> > > >
> > > > On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > > > > Hi Jukka,
> > > > >
> > > > > Does this patch help?
> > > >
> > > > Unfortunately no, I still see inconsistent lock state. It would probably
> > > > have been too easy :)
> > > >
> > >
> > > I remeber something, I think 802.15.4 had similar issues long time ago.
> > >
> > s/remeber/remember/
> >
> > > The fix was 20e7c4e80dcd01dad5e6c8b32455228b8fe9c619 ("6lowpan: fix
> > > lockdep splats"). Please check that, you need something like that!
> > >
> >
> > Something like that:
> >
> > diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> > index 4ebc806..02fd21a 100644
> > --- a/net/bluetooth/6lowpan.c
> > +++ b/net/bluetooth/6lowpan.c
> > @@ -642,7 +642,27 @@ static netdev_tx_t bt_xmit(struct sk_buff *skb, struct net_device *netdev)
> > return err < 0 ? NET_XMIT_DROP : err;
> > }
> >
> > +static struct lock_class_key bt_tx_busylock;
> > +static struct lock_class_key bt_netdev_xmit_lock_key;
> > +
> > +static void bt_set_lockdep_class_one(struct net_device *dev,
> > + struct netdev_queue *txq,
> > + void *_unused)
> > +{
> > + lockdep_set_class(&txq->_xmit_lock,
> > + &bt_netdev_xmit_lock_key);
> > +}
> > +
> > +
> > +static int bt_dev_init(struct net_device *dev)
> > +{
> > + netdev_for_each_tx_queue(dev, bt_set_lockdep_class_one, NULL);
> > + dev->qdisc_tx_busylock = &bt_tx_busylock;
> > + return 0;
> > +}
> > +
> > static const struct net_device_ops netdev_ops = {
> > + .ndo_init = bt_dev_init,
> > .ndo_start_xmit = bt_xmit,
> > };
> >
> >
> > - Alex
> >
> > [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=20e7c4e80dcd01dad5e6c8b32455228b8fe9c619
>
>
> Martin had an idea about the issue being related to work queues, I will
> investigate that further.
>
ok. The above one should fix if some dev_queue_xmit call another
dev_queue_xmit. Don't know if you have some architecture like this in
bluetooth.
- Alex
Hi Alex,
On ma, 2014-10-13 at 19:22 +0200, Alexander Aring wrote:
> Hi Jukka,
>
> sorry.
>
> I was a little too fast here, because I am sure now this should solve your
> lockdep issue.
Unfortunately this patch did not help with the issue.
>
> On Mon, Oct 13, 2014 at 07:11:11PM +0200, Alexander Aring wrote:
> > Hi Jukka,
> >
> > On Mon, Oct 13, 2014 at 06:09:14PM +0300, Jukka Rissanen wrote:
> > > Hi Martin,
> > >
> > > On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > > > Hi Jukka,
> > > >
> > > > Does this patch help?
> > >
> > > Unfortunately no, I still see inconsistent lock state. It would probably
> > > have been too easy :)
> > >
> >
> > I remeber something, I think 802.15.4 had similar issues long time ago.
> >
> s/remeber/remember/
>
> > The fix was 20e7c4e80dcd01dad5e6c8b32455228b8fe9c619 ("6lowpan: fix
> > lockdep splats"). Please check that, you need something like that!
> >
>
> Something like that:
>
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 4ebc806..02fd21a 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -642,7 +642,27 @@ static netdev_tx_t bt_xmit(struct sk_buff *skb, struct net_device *netdev)
> return err < 0 ? NET_XMIT_DROP : err;
> }
>
> +static struct lock_class_key bt_tx_busylock;
> +static struct lock_class_key bt_netdev_xmit_lock_key;
> +
> +static void bt_set_lockdep_class_one(struct net_device *dev,
> + struct netdev_queue *txq,
> + void *_unused)
> +{
> + lockdep_set_class(&txq->_xmit_lock,
> + &bt_netdev_xmit_lock_key);
> +}
> +
> +
> +static int bt_dev_init(struct net_device *dev)
> +{
> + netdev_for_each_tx_queue(dev, bt_set_lockdep_class_one, NULL);
> + dev->qdisc_tx_busylock = &bt_tx_busylock;
> + return 0;
> +}
> +
> static const struct net_device_ops netdev_ops = {
> + .ndo_init = bt_dev_init,
> .ndo_start_xmit = bt_xmit,
> };
>
>
> - Alex
>
> [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=20e7c4e80dcd01dad5e6c8b32455228b8fe9c619
Martin had an idea about the issue being related to work queues, I will
investigate that further.
Cheers,
Jukka
Hi Jukka,
sorry.
I was a little too fast here, because I am sure now this should solve your
lockdep issue.
On Mon, Oct 13, 2014 at 07:11:11PM +0200, Alexander Aring wrote:
> Hi Jukka,
>
> On Mon, Oct 13, 2014 at 06:09:14PM +0300, Jukka Rissanen wrote:
> > Hi Martin,
> >
> > On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > > Hi Jukka,
> > >
> > > Does this patch help?
> >
> > Unfortunately no, I still see inconsistent lock state. It would probably
> > have been too easy :)
> >
>
> I remeber something, I think 802.15.4 had similar issues long time ago.
>
s/remeber/remember/
> The fix was 20e7c4e80dcd01dad5e6c8b32455228b8fe9c619 ("6lowpan: fix
> lockdep splats"). Please check that, you need something like that!
>
Something like that:
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 4ebc806..02fd21a 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -642,7 +642,27 @@ static netdev_tx_t bt_xmit(struct sk_buff *skb, struct net_device *netdev)
return err < 0 ? NET_XMIT_DROP : err;
}
+static struct lock_class_key bt_tx_busylock;
+static struct lock_class_key bt_netdev_xmit_lock_key;
+
+static void bt_set_lockdep_class_one(struct net_device *dev,
+ struct netdev_queue *txq,
+ void *_unused)
+{
+ lockdep_set_class(&txq->_xmit_lock,
+ &bt_netdev_xmit_lock_key);
+}
+
+
+static int bt_dev_init(struct net_device *dev)
+{
+ netdev_for_each_tx_queue(dev, bt_set_lockdep_class_one, NULL);
+ dev->qdisc_tx_busylock = &bt_tx_busylock;
+ return 0;
+}
+
static const struct net_device_ops netdev_ops = {
+ .ndo_init = bt_dev_init,
.ndo_start_xmit = bt_xmit,
};
- Alex
[0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=20e7c4e80dcd01dad5e6c8b32455228b8fe9c619
Hi Jukka,
On Mon, Oct 13, 2014 at 06:09:14PM +0300, Jukka Rissanen wrote:
> Hi Martin,
>
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > Hi Jukka,
> >
> > Does this patch help?
>
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
>
I remeber something, I think 802.15.4 had similar issues long time ago.
The fix was 20e7c4e80dcd01dad5e6c8b32455228b8fe9c619 ("6lowpan: fix
lockdep splats"). Please check that, you need something like that!
- Alex
[0]
Hi,
On Mon, Oct 13, 2014, Jukka Rissanen wrote:
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> > Hi Jukka,
> >
> > Does this patch help?
>
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
> >
> > ---
> > net/bluetooth/l2cap_core.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index b6f9777..fb7b2ff 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> > if (credits > max_credits) {
> > BT_ERR("LE credits overflow");
> > l2cap_send_disconn_req(chan, ECONNRESET);
> > + l2cap_chan_unlock(chan);
> >
> > /* Return 0 so that we don't trigger an unnecessary
> > * command reject packet.
I'd appreciate if you could still send a proper patch for this since
it's clearly a locking bug (something even worth sending to stable
trees).
Johan
Hi Jukka,
>From the last trace it looks like a transmit has been started from the receive worker thread. I notice that both tx and rx use the same workerqueue structure, e.g.
hci_send_xxx
...
queue_work(hdev->workqueue, &hdev->tx_work);
hci_recv_frame
...
queue_work(hdev->workqueue, &hdev->rx_work);
Would this cause problems. I don't know enough about work queues but in workqueue_struct there is a mutex which may be held by the rx worker and if this rx worker start a transmit maybe this would cause the lock? Could test by creating separate queues for rx and tx?
Anyway just a thought.
- Martin.
On 13/10/14 16:09, Jukka Rissanen wrote:
> Hi Martin,
>
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> Does this patch help?
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
>
>> ---
>> net/bluetooth/l2cap_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index b6f9777..fb7b2ff 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
>> if (credits > max_credits) {
>> BT_ERR("LE credits overflow");
>> l2cap_send_disconn_req(chan, ECONNRESET);
>> + l2cap_chan_unlock(chan);
>>
>> /* Return 0 so that we don't trigger an unnecessary
>> * command reject packet.
>
> Cheers,
> Jukka
>
>
Hi Jukka,
>From the last trace it looks like a transmit has been started from the receive worker thread. I notice that both tx and rx use the same workerqueue structure, e.g.
hci_send_xxx <http://lxr.free-electrons.com/ident?i=hci_send_acl>
...
queue_work <http://lxr.free-electrons.com/ident?i=queue_work>(hdev <http://lxr.free-electrons.com/ident?i=hdev>->workqueue <http://lxr.free-electrons.com/ident?i=workqueue>, &hdev <http://lxr.free-electrons.com/ident?i=hdev>->tx_work);
hci_recv_frame <http://lxr.free-electrons.com/ident?i=hci_recv_frame>
...
queue_work <http://lxr.free-electrons.com/ident?i=queue_work>(hdev <http://lxr.free-electrons.com/ident?i=hdev>->workqueue <http://lxr.free-electrons.com/ident?i=workqueue>, &hdev <http://lxr.free-electrons.com/ident?i=hdev>->rx_work <http://lxr.free-electrons.com/ident?i=rx_work>);
Would this cause problems. I don't know enough about work queues but in workqueue_struct there is a mutex which may be held by the rx worker and if this rx worker start a transmit maybe this would cause the lock? Could test by creating separate queues for rx and tx?
Anyway just a thought.
- Martin.
On 13/10/14 16:09, Jukka Rissanen wrote:
> Hi Martin,
>
> On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> Does this patch help?
> Unfortunately no, I still see inconsistent lock state. It would probably
> have been too easy :)
>
>> ---
>> net/bluetooth/l2cap_core.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index b6f9777..fb7b2ff 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
>> if (credits > max_credits) {
>> BT_ERR("LE credits overflow");
>> l2cap_send_disconn_req(chan, ECONNRESET);
>> + l2cap_chan_unlock(chan);
>>
>> /* Return 0 so that we don't trigger an unnecessary
>> * command reject packet.
>
> Cheers,
> Jukka
>
>
Hi Martin,
On ma, 2014-10-13 at 15:56 +0100, Martin Townsend wrote:
> Hi Jukka,
>
> Does this patch help?
Unfortunately no, I still see inconsistent lock state. It would probably
have been too easy :)
>
> ---
> net/bluetooth/l2cap_core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index b6f9777..fb7b2ff 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> if (credits > max_credits) {
> BT_ERR("LE credits overflow");
> l2cap_send_disconn_req(chan, ECONNRESET);
> + l2cap_chan_unlock(chan);
>
> /* Return 0 so that we don't trigger an unnecessary
> * command reject packet.
Cheers,
Jukka
Hi Jukka,
Does this patch help?
---
net/bluetooth/l2cap_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b6f9777..fb7b2ff 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5494,6 +5494,7 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
if (credits > max_credits) {
BT_ERR("LE credits overflow");
l2cap_send_disconn_req(chan, ECONNRESET);
+ l2cap_chan_unlock(chan);
/* Return 0 so that we don't trigger an unnecessary
* command reject packet.
--
1.9.1
-Martin.
On 13/10/14 15:44, Jukka Rissanen wrote:
> Hi Martin,
>
> and thanks for your analysis.
>
> On ma, 2014-10-13 at 14:10 +0100, Martin Townsend wrote:
>> Hi Jukka,
>>
>> I think there's a lock checking option in the kernel hacking configuration menu. Might be worth trying this to get more info.
>> I had a quick look through the code and there maybe a potential locking problem in l2cap_le_credits
>> it calls l2cap_get_chan_by_dcid which locks the channel lock (chan->lock) which is one of the locks in the deadlock below. If credits > max_credits in l2cap_le_credits it returns 0 but no unlock. Now l2cap_send_disconn_req may do this, I tried searching through but it called a state_change op so I gave up.
>> http://lxr.free-electrons.com/source/net/bluetooth/l2cap_core.c#L5539
>>
>> You could try sticking a l2cap_chan_unlock(chan); in to see if the problem goes away.
>>
> I managed to trigger the locking issue (by running ssh over bt 6lowpan)
> even without your patch. So I am acking the v6 of this patch. I try to
> dig the root cause to that deadlock issue I am seeing.
>
> Signed-off-by: Jukka Rissanen <[email protected]>
>
>
> Cheers,
> Jukka
>
>
Hi Martin,
and thanks for your analysis.
On ma, 2014-10-13 at 14:10 +0100, Martin Townsend wrote:
> Hi Jukka,
>
> I think there's a lock checking option in the kernel hacking configuration menu. Might be worth trying this to get more info.
> I had a quick look through the code and there maybe a potential locking problem in l2cap_le_credits
> it calls l2cap_get_chan_by_dcid which locks the channel lock (chan->lock) which is one of the locks in the deadlock below. If credits > max_credits in l2cap_le_credits it returns 0 but no unlock. Now l2cap_send_disconn_req may do this, I tried searching through but it called a state_change op so I gave up.
> http://lxr.free-electrons.com/source/net/bluetooth/l2cap_core.c#L5539
>
> You could try sticking a l2cap_chan_unlock(chan); in to see if the problem goes away.
>
I managed to trigger the locking issue (by running ssh over bt 6lowpan)
even without your patch. So I am acking the v6 of this patch. I try to
dig the root cause to that deadlock issue I am seeing.
Signed-off-by: Jukka Rissanen <[email protected]>
Cheers,
Jukka
Hi Jukka,
I think there's a lock checking option in the kernel hacking configuration menu. Might be worth trying this to get more info.
I had a quick look through the code and there maybe a potential locking problem in l2cap_le_credits
it calls l2cap_get_chan_by_dcid which locks the channel lock (chan->lock) which is one of the locks in the deadlock below. If credits > max_credits in l2cap_le_credits it returns 0 but no unlock. Now l2cap_send_disconn_req may do this, I tried searching through but it called a state_change op so I gave up.
http://lxr.free-electrons.com/source/net/bluetooth/l2cap_core.c#L5539
You could try sticking a l2cap_chan_unlock(chan); in to see if the problem goes away.
Alex have you tried this patch in a virtual emulator or on real HW for 802.15.4? we would know if it's a bluetooth or general problem.
- Martin
On 13/10/14 12:49, Jukka Rissanen wrote:
> Hi Martin,
>
> with this v6 I started to see similar issues locking issues as in some
> earlier patch version. I will try v5 and v4 just to make sure what is
> going on here.
>
>
> [ 308.736047] =================================
> [ 308.736047] [ INFO: inconsistent lock state ]
> [ 308.736047] 3.17.0-rc1-bt6lowpan #1 Not tainted
> [ 308.736047] ---------------------------------
> [ 308.736047] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
> [ 308.736047] kworker/u3:2/404 [HC0[0]:SC0[0]:HE1:SE1] takes:
> [ 308.736047] (&(&list->lock)->rlock#6){+.?...}, at: [<f82a8d4c>]
> hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] {IN-SOFTIRQ-W} state was registered at:
> [ 308.736047] [<c10915a3>] __lock_acquire+0x6d3/0x1d20
> [ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
> [ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
> [ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
> [ 308.736047] [<f82cd7c0>] l2cap_chan_send+0x7f0/0x10e0 [bluetooth]
> [ 308.736047] [<f850d91e>] send_pkt+0x4e/0xa0 [bluetooth_6lowpan]
> [ 308.736047] [<f850dd20>] bt_xmit+0x3b0/0x770 [bluetooth_6lowpan]
> [ 308.736047] [<c17742f4>] dev_hard_start_xmit+0x344/0x670
> [ 308.736047] [<c17749ad>] __dev_queue_xmit+0x38d/0x680
> [ 308.736047] [<c1774caf>] dev_queue_xmit+0xf/0x20
> [ 308.736047] [<c177b8b0>] neigh_connected_output+0x130/0x1a0
> [ 308.736047] [<c1812a63>] ip6_finish_output2+0x173/0x8c0
> [ 308.736047] [<c18182db>] ip6_finish_output+0x7b/0x1b0
> [ 308.736047] [<c18184a7>] ip6_output+0x97/0x2a0
> [ 308.736047] [<c183a46b>] mld_sendpack+0x5eb/0x650
> [ 308.736047] [<c183acc1>] mld_ifc_timer_expire+0x191/0x2f0
> [ 308.736047] [<c10ac385>] call_timer_fn+0x85/0x1c0
> [ 308.736047] [<c10acb72>] run_timer_softirq+0x192/0x280
> [ 308.736047] [<c104fd84>] __do_softirq+0xd4/0x300
> [ 308.736047] [<c10049fc>] do_softirq_own_stack+0x2c/0x40
> [ 308.736047] [<c1050136>] irq_exit+0x86/0xb0
> [ 308.736047] [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
> [ 308.736047] [<c188b6ce>] apic_timer_interrupt+0x32/0x38
> [ 308.736047] [<c115e6b2>] kmem_cache_alloc+0x1c2/0x290
> [ 308.736047] [<c116a9ae>] create_object+0x2e/0x280
> [ 308.736047] [<c187f10c>] kmemleak_alloc+0x3c/0xb0
> [ 308.736047] [<c115e693>] kmem_cache_alloc+0x1a3/0x290
> [ 308.736047] [<c1178835>] getname_flags+0x25/0x110
> [ 308.736047] [<c117d21e>] user_path_at_empty+0x1e/0x80
> [ 308.736047] [<c1173317>] SyS_readlinkat+0x57/0x100
> [ 308.736047] [<c11733ec>] SyS_readlink+0x2c/0x30
> [ 308.736047] [<c188aeb6>] syscall_after_call+0x0/0x4
> [ 308.736047] irq event stamp: 37665
> [ 308.736047] hardirqs last enabled at (37665): [<c188a065>]
> _raw_spin_unlock_irqrestore+0x55/0x70
> [ 308.736047] hardirqs last disabled at (37664): [<c1889e03>]
> _raw_spin_lock_irqsave+0x23/0x90
> [ 308.736047] softirqs last enabled at (37378): [<c104ff5c>]
> __do_softirq+0x2ac/0x300
> [ 308.736047] softirqs last disabled at (37359): [<c10049fc>]
> do_softirq_own_stack+0x2c/0x40
> [ 308.736047]
> [ 308.736047] other info that might help us debug this:
> [ 308.736047] Possible unsafe locking scenario:
> [ 308.736047]
> [ 308.736047] CPU0
> [ 308.736047] ----
> [ 308.736047] lock(&(&list->lock)->rlock#6);
> [ 308.736047] <Interrupt>
> [ 308.736047] lock(&(&list->lock)->rlock#6);
> [ 308.736047]
> [ 308.736047] *** DEADLOCK ***
> [ 308.736047]
> [ 308.736047] 3 locks held by kworker/u3:2/404:
> [ 308.736047] #0: ("%s"hdev->name#2){.+.+.+}, at: [<c10622c3>]
> process_one_work+0x113/0x4a0
> [ 308.736047] #1: ((&hdev->rx_work)){+.+.+.}, at: [<c10622c3>]
> process_one_work+0x113/0x4a0
> [ 308.736047] #2: (&chan->lock){+.+.+.}, at: [<f82c9179>]
> l2cap_get_chan_by_dcid+0x89/0x90 [bluetooth]
> [ 308.736047]
> [ 308.736047] stack backtrace:
> [ 308.736047] CPU: 0 PID: 404 Comm: kworker/u3:2 Not tainted
> 3.17.0-rc1-bt6lowpan #1
> [ 308.736047] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
> VirtualBox 12/01/2006
> [ 308.736047] Workqueue: hci0 hci_rx_work [bluetooth]
> [ 308.736047] efe3d780 00000000 efedbb90 c18821c1 c2345c10 efedbbb0
> c1880a94 c1ad9264
> [ 308.736047] c1ad9201 c1ad95f8 efe3dd94 00000006 c108e0c0 efedbbe0
> c108ea8c 00000006
> [ 308.736047] efe3d780 efedbffc e21128f4 00000047 00000004 efe3d780
> efe3dd98 00000003
> [ 308.736047] Call Trace:
> [ 308.736047] [<c18821c1>] dump_stack+0x4b/0x75
> [ 308.736047] [<c1880a94>] print_usage_bug.part.36+0x209/0x213
> [ 308.736047] [<c108e0c0>] ? check_usage_forwards+0x110/0x110
> [ 308.736047] [<c108ea8c>] mark_lock+0x11c/0x6e0
> [ 308.736047] [<c10915e4>] __lock_acquire+0x714/0x1d20
> [ 308.736047] [<c1004b6f>] ? dump_trace+0xcf/0x1f0
> [ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
> [ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
> [ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
> [ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
> [ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
> [ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
> [ 308.736047] [<c188a065>] ? _raw_spin_unlock_irqrestore+0x55/0x70
> [ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
> [ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
> [ 308.736047] [<c188a051>] ? _raw_spin_unlock_irqrestore+0x41/0x70
> [ 308.736047] [<c1763125>] ? skb_dequeue+0x45/0x60
> [ 308.736047] [<f82d4389>] l2cap_recv_frame+0x23d9/0x2db0 [bluetooth]
> [ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
> [ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
> [ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
> [ 308.736047] [<c10761ef>] ? sched_clock_cpu+0x10f/0x160
> [ 308.736047] [<c107183b>] ? get_parent_ip+0xb/0x40
> [ 308.736047] [<c10718bb>] ? preempt_count_add+0x4b/0xa0
> [ 308.736047] [<c13d09e2>] ? debug_smp_processor_id+0x12/0x20
> [ 308.736047] [<c108cc04>] ? get_lock_stats+0x24/0x40
> [ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
> [ 308.736047] [<c188716d>] ? __mutex_unlock_slowpath+0xcd/0x1b0
> [ 308.736047] [<c13d09ff>] ? __this_cpu_preempt_check+0xf/0x20
> [ 308.736047] [<c108f1d4>] ? trace_hardirqs_on_caller+0xf4/0x240
> [ 308.736047] [<c108c9a6>] ? trace_hardirqs_off_caller+0xb6/0x160
> [ 308.736047] [<f82d62a9>] l2cap_recv_acldata+0x2f9/0x340 [bluetooth]
> [ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
> [ 308.736047] [<f82a1c69>] hci_rx_work+0x369/0x4a0 [bluetooth]
> [ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
> [ 308.736047] [<c106234a>] process_one_work+0x19a/0x4a0
> [ 308.736047] [<c10622c3>] ? process_one_work+0x113/0x4a0
> [ 308.736047] [<c10629e9>] worker_thread+0x39/0x440
> [ 308.736047] [<c10629b0>] ? init_pwq+0xc0/0xc0
> [ 308.736047] [<c1066dc8>] kthread+0xa8/0xc0
> [ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
> [ 308.736047] [<c188ad01>] ret_from_kernel_thread+0x21/0x30
> [ 308.736047] [<c1066d20>] ? kthread_create_on_node+0x160/0x160
>
>
>
> Cheers,
> Jukka
>
>
Hi Martin,
with this v6 I started to see similar issues locking issues as in some
earlier patch version. I will try v5 and v4 just to make sure what is
going on here.
[ 308.736047] =================================
[ 308.736047] [ INFO: inconsistent lock state ]
[ 308.736047] 3.17.0-rc1-bt6lowpan #1 Not tainted
[ 308.736047] ---------------------------------
[ 308.736047] inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
[ 308.736047] kworker/u3:2/404 [HC0[0]:SC0[0]:HE1:SE1] takes:
[ 308.736047] (&(&list->lock)->rlock#6){+.?...}, at: [<f82a8d4c>]
hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] {IN-SOFTIRQ-W} state was registered at:
[ 308.736047] [<c10915a3>] __lock_acquire+0x6d3/0x1d20
[ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
[ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
[ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
[ 308.736047] [<f82cd7c0>] l2cap_chan_send+0x7f0/0x10e0 [bluetooth]
[ 308.736047] [<f850d91e>] send_pkt+0x4e/0xa0 [bluetooth_6lowpan]
[ 308.736047] [<f850dd20>] bt_xmit+0x3b0/0x770 [bluetooth_6lowpan]
[ 308.736047] [<c17742f4>] dev_hard_start_xmit+0x344/0x670
[ 308.736047] [<c17749ad>] __dev_queue_xmit+0x38d/0x680
[ 308.736047] [<c1774caf>] dev_queue_xmit+0xf/0x20
[ 308.736047] [<c177b8b0>] neigh_connected_output+0x130/0x1a0
[ 308.736047] [<c1812a63>] ip6_finish_output2+0x173/0x8c0
[ 308.736047] [<c18182db>] ip6_finish_output+0x7b/0x1b0
[ 308.736047] [<c18184a7>] ip6_output+0x97/0x2a0
[ 308.736047] [<c183a46b>] mld_sendpack+0x5eb/0x650
[ 308.736047] [<c183acc1>] mld_ifc_timer_expire+0x191/0x2f0
[ 308.736047] [<c10ac385>] call_timer_fn+0x85/0x1c0
[ 308.736047] [<c10acb72>] run_timer_softirq+0x192/0x280
[ 308.736047] [<c104fd84>] __do_softirq+0xd4/0x300
[ 308.736047] [<c10049fc>] do_softirq_own_stack+0x2c/0x40
[ 308.736047] [<c1050136>] irq_exit+0x86/0xb0
[ 308.736047] [<c188bd98>] smp_apic_timer_interrupt+0x38/0x50
[ 308.736047] [<c188b6ce>] apic_timer_interrupt+0x32/0x38
[ 308.736047] [<c115e6b2>] kmem_cache_alloc+0x1c2/0x290
[ 308.736047] [<c116a9ae>] create_object+0x2e/0x280
[ 308.736047] [<c187f10c>] kmemleak_alloc+0x3c/0xb0
[ 308.736047] [<c115e693>] kmem_cache_alloc+0x1a3/0x290
[ 308.736047] [<c1178835>] getname_flags+0x25/0x110
[ 308.736047] [<c117d21e>] user_path_at_empty+0x1e/0x80
[ 308.736047] [<c1173317>] SyS_readlinkat+0x57/0x100
[ 308.736047] [<c11733ec>] SyS_readlink+0x2c/0x30
[ 308.736047] [<c188aeb6>] syscall_after_call+0x0/0x4
[ 308.736047] irq event stamp: 37665
[ 308.736047] hardirqs last enabled at (37665): [<c188a065>]
_raw_spin_unlock_irqrestore+0x55/0x70
[ 308.736047] hardirqs last disabled at (37664): [<c1889e03>]
_raw_spin_lock_irqsave+0x23/0x90
[ 308.736047] softirqs last enabled at (37378): [<c104ff5c>]
__do_softirq+0x2ac/0x300
[ 308.736047] softirqs last disabled at (37359): [<c10049fc>]
do_softirq_own_stack+0x2c/0x40
[ 308.736047]
[ 308.736047] other info that might help us debug this:
[ 308.736047] Possible unsafe locking scenario:
[ 308.736047]
[ 308.736047] CPU0
[ 308.736047] ----
[ 308.736047] lock(&(&list->lock)->rlock#6);
[ 308.736047] <Interrupt>
[ 308.736047] lock(&(&list->lock)->rlock#6);
[ 308.736047]
[ 308.736047] *** DEADLOCK ***
[ 308.736047]
[ 308.736047] 3 locks held by kworker/u3:2/404:
[ 308.736047] #0: ("%s"hdev->name#2){.+.+.+}, at: [<c10622c3>]
process_one_work+0x113/0x4a0
[ 308.736047] #1: ((&hdev->rx_work)){+.+.+.}, at: [<c10622c3>]
process_one_work+0x113/0x4a0
[ 308.736047] #2: (&chan->lock){+.+.+.}, at: [<f82c9179>]
l2cap_get_chan_by_dcid+0x89/0x90 [bluetooth]
[ 308.736047]
[ 308.736047] stack backtrace:
[ 308.736047] CPU: 0 PID: 404 Comm: kworker/u3:2 Not tainted
3.17.0-rc1-bt6lowpan #1
[ 308.736047] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[ 308.736047] Workqueue: hci0 hci_rx_work [bluetooth]
[ 308.736047] efe3d780 00000000 efedbb90 c18821c1 c2345c10 efedbbb0
c1880a94 c1ad9264
[ 308.736047] c1ad9201 c1ad95f8 efe3dd94 00000006 c108e0c0 efedbbe0
c108ea8c 00000006
[ 308.736047] efe3d780 efedbffc e21128f4 00000047 00000004 efe3d780
efe3dd98 00000003
[ 308.736047] Call Trace:
[ 308.736047] [<c18821c1>] dump_stack+0x4b/0x75
[ 308.736047] [<c1880a94>] print_usage_bug.part.36+0x209/0x213
[ 308.736047] [<c108e0c0>] ? check_usage_forwards+0x110/0x110
[ 308.736047] [<c108ea8c>] mark_lock+0x11c/0x6e0
[ 308.736047] [<c10915e4>] __lock_acquire+0x714/0x1d20
[ 308.736047] [<c1004b6f>] ? dump_trace+0xcf/0x1f0
[ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
[ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
[ 308.736047] [<c109325d>] lock_acquire+0x9d/0x140
[ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<c1889c25>] _raw_spin_lock+0x45/0x80
[ 308.736047] [<f82a8d4c>] ? hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<f82a8d4c>] hci_send_acl+0xac/0x290 [bluetooth]
[ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
[ 308.736047] [<c188a065>] ? _raw_spin_unlock_irqrestore+0x55/0x70
[ 308.736047] [<f82c9bf0>] l2cap_do_send+0x60/0x100 [bluetooth]
[ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
[ 308.736047] [<c188a051>] ? _raw_spin_unlock_irqrestore+0x41/0x70
[ 308.736047] [<c1763125>] ? skb_dequeue+0x45/0x60
[ 308.736047] [<f82d4389>] l2cap_recv_frame+0x23d9/0x2db0 [bluetooth]
[ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
[ 308.736047] [<c100a5f8>] ? sched_clock+0x8/0x10
[ 308.736047] [<c1075da9>] ? sched_clock_local+0x49/0x180
[ 308.736047] [<c10761ef>] ? sched_clock_cpu+0x10f/0x160
[ 308.736047] [<c107183b>] ? get_parent_ip+0xb/0x40
[ 308.736047] [<c10718bb>] ? preempt_count_add+0x4b/0xa0
[ 308.736047] [<c13d09e2>] ? debug_smp_processor_id+0x12/0x20
[ 308.736047] [<c108cc04>] ? get_lock_stats+0x24/0x40
[ 308.736047] [<c108f0b4>] ? mark_held_locks+0x64/0x90
[ 308.736047] [<c188716d>] ? __mutex_unlock_slowpath+0xcd/0x1b0
[ 308.736047] [<c13d09ff>] ? __this_cpu_preempt_check+0xf/0x20
[ 308.736047] [<c108f1d4>] ? trace_hardirqs_on_caller+0xf4/0x240
[ 308.736047] [<c108c9a6>] ? trace_hardirqs_off_caller+0xb6/0x160
[ 308.736047] [<f82d62a9>] l2cap_recv_acldata+0x2f9/0x340 [bluetooth]
[ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
[ 308.736047] [<f82a1c69>] hci_rx_work+0x369/0x4a0 [bluetooth]
[ 308.736047] [<f82a1a13>] ? hci_rx_work+0x113/0x4a0 [bluetooth]
[ 308.736047] [<c106234a>] process_one_work+0x19a/0x4a0
[ 308.736047] [<c10622c3>] ? process_one_work+0x113/0x4a0
[ 308.736047] [<c10629e9>] worker_thread+0x39/0x440
[ 308.736047] [<c10629b0>] ? init_pwq+0xc0/0xc0
[ 308.736047] [<c1066dc8>] kthread+0xa8/0xc0
[ 308.736047] [<c108f32b>] ? trace_hardirqs_on+0xb/0x10
[ 308.736047] [<c188ad01>] ret_from_kernel_thread+0x21/0x30
[ 308.736047] [<c1066d20>] ? kthread_create_on_node+0x160/0x160
Cheers,
Jukka
On Mon, Oct 13, 2014 at 11:00:56AM +0100, Martin Townsend wrote:
> Currently there are potentially 2 skb_copy_expand calls in IPHC
> decompression. This patch replaces this with one call to
> skb_cow which will check to see if there is enough headroom
> first to ensure it's only done if necessary and will handle
> alignment issues for cache.
> As skb_cow uses pskb_expand_head we ensure the skb isn't shared from
> bluetooth and ieee802.15.4 code that use the IPHC decompression.
>
> Signed-off-by: Martin Townsend <[email protected]>
Okay, we don't use the goto drop; below and currently this will always
return -EINVAL. You will fix that in your next patches, so it doesn't
matter right now. Also this will correct a little bit the errno value.
Great job Martin.
Acked-by: Alexander Aring <[email protected]>
Marcel, please wait for jukka's acked here, then we are sure that we
didn't break anything in bluetooth 6lowpan.
- Alex
Currently there are potentially 2 skb_copy_expand calls in IPHC
decompression. This patch replaces this with one call to
skb_cow which will check to see if there is enough headroom
first to ensure it's only done if necessary and will handle
alignment issues for cache.
As skb_cow uses pskb_expand_head we ensure the skb isn't shared from
bluetooth and ieee802.15.4 code that use the IPHC decompression.
Signed-off-by: Martin Townsend <[email protected]>
---
net/6lowpan/iphc.c | 47 +++++++++++++++++++++--------------------------
net/bluetooth/6lowpan.c | 4 ++++
2 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 142eef5..747b3cc 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -174,30 +174,22 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
struct net_device *dev, skb_delivery_cb deliver_skb)
{
- struct sk_buff *new;
int stat;
- new = skb_copy_expand(skb, sizeof(struct ipv6hdr), skb_tailroom(skb),
- GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb_push(new, sizeof(struct ipv6hdr));
- skb_reset_network_header(new);
- skb_copy_to_linear_data(new, hdr, sizeof(struct ipv6hdr));
+ skb_push(skb, sizeof(struct ipv6hdr));
+ skb_reset_network_header(skb);
+ skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
- new->protocol = htons(ETH_P_IPV6);
- new->pkt_type = PACKET_HOST;
- new->dev = dev;
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = dev;
raw_dump_table(__func__, "raw skb data dump before receiving",
- new->data, new->len);
+ skb->data, skb->len);
- stat = deliver_skb(new, dev);
+ stat = deliver_skb(skb, dev);
- kfree_skb(new);
+ consume_skb(skb);
return stat;
}
@@ -460,7 +452,7 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* UDP data uncompression */
if (iphc0 & LOWPAN_IPHC_NH_C) {
struct udphdr uh;
- struct sk_buff *new;
+ const int needed = sizeof(struct udphdr) + sizeof(hdr);
if (uncompress_udp_header(skb, &uh))
goto drop;
@@ -468,14 +460,11 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
/* replace the compressed UDP head by the uncompressed UDP
* header
*/
- new = skb_copy_expand(skb, sizeof(struct udphdr),
- skb_tailroom(skb), GFP_ATOMIC);
- kfree_skb(skb);
-
- if (!new)
- return -ENOMEM;
-
- skb = new;
+ err = skb_cow(skb, needed);
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
skb_push(skb, sizeof(struct udphdr));
skb_reset_transport_header(skb);
@@ -485,6 +474,12 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
(u8 *)&uh, sizeof(uh));
hdr.nexthdr = UIP_PROTO_UDP;
+ } else {
+ err = skb_cow(skb, sizeof(hdr));
+ if (unlikely(err)) {
+ kfree_skb(skb);
+ return err;
+ }
}
hdr.payload_len = htons(skb->len);
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index c2e0d14..4ebc806 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -316,6 +316,10 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
if (dev->type != ARPHRD_6LOWPAN)
goto drop;
+ skb = skb_share_check(skb, GFP_ATOMIC);
+ if (!skb)
+ goto drop;
+
/* check that it's our buffer */
if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
/* Copy the packet so that the IPv6 header is
--
1.9.1