[PATCH net] brcmfmac: clear skb head state on xmit

[PATCH net] brcmfmac: clear skb head state on xmit

[Paolo Abeni]

the skbs can be held by the driver for a long time, so we need
to clear any state on xmit to avoid hanging other subsystems.
The skbs are already orphaned later in cmsg code, so we just
need to clear the nf/dst/secpath.
Do it early, while the relevant entries are hopefully still
hot in the cache.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 9e6f60a..5a8d57b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -22,6 +22,7 @@
 #include <net/rtnetlink.h>
 #include <net/addrconf.h>
 #include <net/ipv6.h>
+#include <net/xfrm.h>
 #include <brcmu_utils.h>
 #include <brcmu_wifi.h>
 
@@ -243,6 +244,13 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
 	if ((skb->priority == 0) || (skb->priority > 7))
 		skb->priority = cfg80211_classify8021d(skb, NULL);
 
+	/* we can keep the skb for a long time; avoid starving other
+	 * subsystems
+	 */
+	nf_reset(skb);
+	skb_dst_drop(skb);
+	secpath_reset(skb);
+
 	ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);
 	if (ret < 0)
 		brcmf_txfinalize(ifp, skb, false);
-- 
2.9.3

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Arend Van Spriel]

On 7-2-2017 17:50, Paolo Abeni wrote:
> the skbs can be held by the driver for a long time, so we need
> to clear any state on xmit to avoid hanging other subsystems.
> The skbs are already orphaned later in cmsg code, so we just
> need to clear the nf/dst/secpath.
> Do it early, while the relevant entries are hopefully still
> hot in the cache.

What is this about really? A bit more background about the issue might
help understanding the need for this patch. Is this really specific to
brcmfmac. For instance is something similar already done in mac80211?

Regards,
Arend

> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 9e6f60a..5a8d57b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -22,6 +22,7 @@
>  #include <net/rtnetlink.h>
>  #include <net/addrconf.h>
>  #include <net/ipv6.h>
> +#include <net/xfrm.h>
>  #include <brcmu_utils.h>
>  #include <brcmu_wifi.h>
>  
> @@ -243,6 +244,13 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>  	if ((skb->priority == 0) || (skb->priority > 7))
>  		skb->priority = cfg80211_classify8021d(skb, NULL);
>  
> +	/* we can keep the skb for a long time; avoid starving other
> +	 * subsystems
> +	 */
> +	nf_reset(skb);
> +	skb_dst_drop(skb);
> +	secpath_reset(skb);
> +
>  	ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);
>  	if (ret < 0)
>  		brcmf_txfinalize(ifp, skb, false);
> 

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Paolo Abeni]

On Tue, 2017-02-07 at 20:23 +0100, Arend Van Spriel wrote:
> On 7-2-2017 17:50, Paolo Abeni wrote:
> > the skbs can be held by the driver for a long time, so we need
> > to clear any state on xmit to avoid hanging other subsystems.
> > The skbs are already orphaned later in cmsg code, so we just
> > need to clear the nf/dst/secpath.
> > Do it early, while the relevant entries are hopefully still
> > hot in the cache.
> 
> What is this about really? A bit more background about the issue
> might
> help understanding the need for this patch. Is this really specific
> to
> brcmfmac. For instance is something similar already done in mac80211?

The issue is apparently driver specific, as reported in:

https://bugzilla.redhat.com/show_bug.cgi?id=1294415

This is caused by xmit skbs carrying a notrack ct entry not being freed
by the device driver in a timely manner. Removing the ct module waits
for such entries refcount going to zero and hangs the kernel in busy
loop (for several minutes).

The relevant skbs are icmp6 packets (ND if I recall correctly, they
bcast packets at the mac level).

The only other known device driver suffering for the issue is the
infiniband ipoib driver, I send a separate patch for it.

I lack the broadcom h/w, but with infiniband the bug can be reproduced
with the following steps:

- ensure ipv6 is enabled on the target device, and firewalld is running
(e.g. the module nf_conntrack_ipv6 is loaded)
- assign a static ip to the device
- shut down the firewall (e.g. try to remove the module nf_conntrack)

For the brcmfmac driver most probably it is necessary being
disassociated from the AP before shutting down the firewall (but I
can't double check). This is probably why mac80211 does not suffer this
issue.

The root cause for the issue could be actually a firmware issue, any
better clues are more than welcome!

Thank you,

Paolo

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Rafał Miłecki]

On 8 February 2017 at 09:38, Paolo Abeni <pabeni@redhat.com> wrote:
> On Tue, 2017-02-07 at 20:23 +0100, Arend Van Spriel wrote:
>> On 7-2-2017 17:50, Paolo Abeni wrote:
>> > the skbs can be held by the driver for a long time, so we need
>> > to clear any state on xmit to avoid hanging other subsystems.
>> > The skbs are already orphaned later in cmsg code, so we just
>> > need to clear the nf/dst/secpath.
>> > Do it early, while the relevant entries are hopefully still
>> > hot in the cache.
>>
>> What is this about really? A bit more background about the issue
>> might
>> help understanding the need for this patch. Is this really specific
>> to
>> brcmfmac. For instance is something similar already done in mac80211?
>
> The issue is apparently driver specific, as reported in:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=3D1294415
>
> This is caused by xmit skbs carrying a notrack ct entry not being freed
> by the device driver in a timely manner. Removing the ct module waits
> for such entries refcount going to zero and hangs the kernel in busy
> loop (for several minutes).
>
> The relevant skbs are icmp6 packets (ND if I recall correctly, they
> bcast packets at the mac level).
>
> The only other known device driver suffering for the issue is the
> infiniband ipoib driver, I send a separate patch for it.
>
> I lack the broadcom h/w, but with infiniband the bug can be reproduced
> with the following steps:
>
> - ensure ipv6 is enabled on the target device, and firewalld is running
> (e.g. the module nf_conntrack_ipv6 is loaded)
> - assign a static ip to the device
> - shut down the firewall (e.g. try to remove the module nf_conntrack)
>
> For the brcmfmac driver most probably it is necessary being
> disassociated from the AP before shutting down the firewall (but I
> can't double check). This is probably why mac80211 does not suffer this
> issue.
>
> The root cause for the issue could be actually a firmware issue, any
> better clues are more than welcome!

Do I get this correctly brcmfmac gets some skb for transmitting it and
doesn't free it for few minutes? It sounds like some bug that should
be fixed instead of hiding it.

--=20
Rafa=C5=82

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Paolo Abeni]

On Wed, 2017-02-08 at 09:52 +0100, Rafał Miłecki wrote:
> On 8 February 2017 at 09:38, Paolo Abeni <pabeni@redhat.com> wrote:
> > On Tue, 2017-02-07 at 20:23 +0100, Arend Van Spriel wrote:
> > > On 7-2-2017 17:50, Paolo Abeni wrote:
> > > > the skbs can be held by the driver for a long time, so we need
> > > > to clear any state on xmit to avoid hanging other subsystems.
> > > > The skbs are already orphaned later in cmsg code, so we just
> > > > need to clear the nf/dst/secpath.
> > > > Do it early, while the relevant entries are hopefully still
> > > > hot in the cache.
> > > 
> > > What is this about really? A bit more background about the issue
> > > might
> > > help understanding the need for this patch. Is this really
> > > specific
> > > to
> > > brcmfmac. For instance is something similar already done in
> > > mac80211?
> > 
> > The issue is apparently driver specific, as reported in:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1294415
> > 
> > This is caused by xmit skbs carrying a notrack ct entry not being
> > freed
> > by the device driver in a timely manner. Removing the ct module
> > waits
> > for such entries refcount going to zero and hangs the kernel in
> > busy
> > loop (for several minutes).
> > 
> > The relevant skbs are icmp6 packets (ND if I recall correctly, they
> > bcast packets at the mac level).
> > 
> > The only other known device driver suffering for the issue is the
> > infiniband ipoib driver, I send a separate patch for it.
> > 
> > I lack the broadcom h/w, but with infiniband the bug can be
> > reproduced
> > with the following steps:
> > 
> > - ensure ipv6 is enabled on the target device, and firewalld is
> > running
> > (e.g. the module nf_conntrack_ipv6 is loaded)
> > - assign a static ip to the device
> > - shut down the firewall (e.g. try to remove the module
> > nf_conntrack)
> > 
> > For the brcmfmac driver most probably it is necessary being
> > disassociated from the AP before shutting down the firewall (but I
> > can't double check). This is probably why mac80211 does not suffer
> > this
> > issue.
> > 
> > The root cause for the issue could be actually a firmware issue,
> > any
> > better clues are more than welcome!
> 
> Do I get this correctly brcmfmac gets some skb for transmitting it
> and
> doesn't free it for few minutes? It sounds like some bug that should
> be fixed instead of hiding it.

I mostly agreed, but please also note that early clearing the skb head
state makes sense from a performance pov: we can do the costly,
required, atomic operations while the data is still hot in the cpu L1
cache.

I'm unable to find anything obvious at the driver level, and I think
the root cause could be in the firmware. I hope some more knowledgeable
than me on this topics can have a better look.

Thank you,

Paolo

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Arend Van Spriel]

On 8-2-2017 10:29, Paolo Abeni wrote:
> On Wed, 2017-02-08 at 09:52 +0100, Rafał Miłecki wrote:
>> On 8 February 2017 at 09:38, Paolo Abeni <pabeni@redhat.com> wrote:
>>> On Tue, 2017-02-07 at 20:23 +0100, Arend Van Spriel wrote:
>>>> On 7-2-2017 17:50, Paolo Abeni wrote:
>>>>> the skbs can be held by the driver for a long time, so we need
>>>>> to clear any state on xmit to avoid hanging other subsystems.
>>>>> The skbs are already orphaned later in cmsg code, so we just
>>>>> need to clear the nf/dst/secpath.
>>>>> Do it early, while the relevant entries are hopefully still
>>>>> hot in the cache.
>>>>
>>>> What is this about really? A bit more background about the issue
>>>> might
>>>> help understanding the need for this patch. Is this really
>>>> specific
>>>> to
>>>> brcmfmac. For instance is something similar already done in
>>>> mac80211?
>>>
>>> The issue is apparently driver specific, as reported in:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1294415
>>>
>>> This is caused by xmit skbs carrying a notrack ct entry not being
>>> freed
>>> by the device driver in a timely manner. Removing the ct module
>>> waits
>>> for such entries refcount going to zero and hangs the kernel in
>>> busy
>>> loop (for several minutes).
>>>
>>> The relevant skbs are icmp6 packets (ND if I recall correctly, they
>>> bcast packets at the mac level).
>>>
>>> The only other known device driver suffering for the issue is the
>>> infiniband ipoib driver, I send a separate patch for it.
>>>
>>> I lack the broadcom h/w, but with infiniband the bug can be
>>> reproduced
>>> with the following steps:
>>>
>>> - ensure ipv6 is enabled on the target device, and firewalld is
>>> running
>>> (e.g. the module nf_conntrack_ipv6 is loaded)
>>> - assign a static ip to the device
>>> - shut down the firewall (e.g. try to remove the module
>>> nf_conntrack)
>>>
>>> For the brcmfmac driver most probably it is necessary being
>>> disassociated from the AP before shutting down the firewall (but I
>>> can't double check). This is probably why mac80211 does not suffer
>>> this
>>> issue.
>>>
>>> The root cause for the issue could be actually a firmware issue,
>>> any
>>> better clues are more than welcome!
>>
>> Do I get this correctly brcmfmac gets some skb for transmitting it
>> and
>> doesn't free it for few minutes? It sounds like some bug that should
>> be fixed instead of hiding it.
> 
> I mostly agreed, but please also note that early clearing the skb head
> state makes sense from a performance pov: we can do the costly,
> required, atomic operations while the data is still hot in the cpu L1
> cache.
> 
> I'm unable to find anything obvious at the driver level, and I think
> the root cause could be in the firmware. I hope some more knowledgeable
> than me on this topics can have a better look.

Hi Paolo,

I agree with Rafał that several minutes for an skb to be freed is a red
flag. Now I know our driver and firmware but not played to much with
IPv6 and firewalls. Hopefully I have all the netfilter stuff in my
kernel when I try to reproduce it over here.

Regards,
Arend

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Kalle Valo]

Paolo Abeni <pabeni@redhat.com> writes:

> On Tue, 2017-02-07 at 20:23 +0100, Arend Van Spriel wrote:
>> On 7-2-2017 17:50, Paolo Abeni wrote:
>> > the skbs can be held by the driver for a long time, so we need
>> > to clear any state on xmit to avoid hanging other subsystems.
>> > The skbs are already orphaned later in cmsg code, so we just
>> > need to clear the nf/dst/secpath.
>> > Do it early, while the relevant entries are hopefully still
>> > hot in the cache.
>> 
>> What is this about really? A bit more background about the issue
>> might
>> help understanding the need for this patch. Is this really specific
>> to
>> brcmfmac. For instance is something similar already done in mac80211?
>
> The issue is apparently driver specific, as reported in:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1294415
>
> This is caused by xmit skbs carrying a notrack ct entry not being freed
> by the device driver in a timely manner. Removing the ct module waits
> for such entries refcount going to zero and hangs the kernel in busy
> loop (for several minutes).
>
> The relevant skbs are icmp6 packets (ND if I recall correctly, they
> bcast packets at the mac level).
>
> The only other known device driver suffering for the issue is the
> infiniband ipoib driver, I send a separate patch for it.
>
> I lack the broadcom h/w, but with infiniband the bug can be reproduced
> with the following steps:
>
> - ensure ipv6 is enabled on the target device, and firewalld is running
> (e.g. the module nf_conntrack_ipv6 is loaded)
> - assign a static ip to the device
> - shut down the firewall (e.g. try to remove the module nf_conntrack)
>
> For the brcmfmac driver most probably it is necessary being
> disassociated from the AP before shutting down the firewall (but I
> can't double check). This is probably why mac80211 does not suffer this
> issue.
>
> The root cause for the issue could be actually a firmware issue, any
> better clues are more than welcome!

BTW, you should have added all this to the commit log. After reading the
original description it left more questions open than answered them.

-- 
Kalle Valo

Re: [PATCH net] brcmfmac: clear skb head state on xmit

[Paolo Abeni]

On Wed, 2017-02-08 at 12:27 +0200, Kalle Valo wrote:
> Paolo Abeni <pabeni@redhat.com> writes:
> 
> > On Tue, 2017-02-07 at 20:23 +0100, Arend Van Spriel wrote:
> > > On 7-2-2017 17:50, Paolo Abeni wrote:
> > > > the skbs can be held by the driver for a long time, so we need
> > > > to clear any state on xmit to avoid hanging other subsystems.
> > > > The skbs are already orphaned later in cmsg code, so we just
> > > > need to clear the nf/dst/secpath.
> > > > Do it early, while the relevant entries are hopefully still
> > > > hot in the cache.
> > > 
> > > What is this about really? A bit more background about the issue
> > > might
> > > help understanding the need for this patch. Is this really specific
> > > to
> > > brcmfmac. For instance is something similar already done in mac80211?
> > 
> > The issue is apparently driver specific, as reported in:
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1294415
> > 
> > This is caused by xmit skbs carrying a notrack ct entry not being freed
> > by the device driver in a timely manner. Removing the ct module waits
> > for such entries refcount going to zero and hangs the kernel in busy
> > loop (for several minutes).
> > 
> > The relevant skbs are icmp6 packets (ND if I recall correctly, they
> > bcast packets at the mac level).
> > 
> > The only other known device driver suffering for the issue is the
> > infiniband ipoib driver, I send a separate patch for it.
> > 
> > I lack the broadcom h/w, but with infiniband the bug can be reproduced
> > with the following steps:
> > 
> > - ensure ipv6 is enabled on the target device, and firewalld is running
> > (e.g. the module nf_conntrack_ipv6 is loaded)
> > - assign a static ip to the device
> > - shut down the firewall (e.g. try to remove the module nf_conntrack)
> > 
> > For the brcmfmac driver most probably it is necessary being
> > disassociated from the AP before shutting down the firewall (but I
> > can't double check). This is probably why mac80211 does not suffer this
> > issue.
> > 
> > The root cause for the issue could be actually a firmware issue, any
> > better clues are more than welcome!
> 
> BTW, you should have added all this to the commit log. After reading the
> original description it left more questions open than answered them.

I'm sorry for the lack of clarity, thank you for the advice, will do
next time. 

I tried to keep the commit message short since others reviewer in other
area of the kernel really prefer shorter ones.

Cheers,

Paolo