Re: Commit 05cf0d1bf4 ("net: stmmac: free an skb first when there are no longer any descriptors using it") breaks stmmac?

[Niklas Cassel <niklas.cassel@axis.com>]

On Mon, Nov 27, 2017 at 02:41:00PM +0000, Jose Abreu wrote:
> Hi Niklas,

Hello Jose,

> 
> I think your commit 05cf0d1bf4 ("net: stmmac: free an skb first
> when there are no longer any descriptors using it") is breaking
> stmmac driver in multi-queue configuration (this stacktrace may
> contain some extra characters as I was using serial port):
> 
> ------------->8-------------
> general protection fault: 0000 [#1] SMP
> Modules linked in: stmmac_pci stmmac libphy igb ptp pps_core
> x86_pkg_temp_thermal
> CPU: 5 PID: 0 Comm: swapper/5 Tainted: G        W       4.14.0-rc5 #2
> Hardware name: Default string Default string/SKYBAY, BIOS 5.0.1.1
> 10/06/2016
> task: ffffa2fe14d8b100 task.stack: ffffb8c6000b8000
> RIP: 0010:skb_release_data+0x66/0x110
> RSP: 0018:ffffa2fe2dd43d98 EFLAGS: 00010206
> RAX: 0000000000000030 RBX: ffffa2fe13fab100 RCX: 00000000000005aa
> RDX: ffffa2fe12a50000 RSI: 0000000000000000 RDI: fffcfffdfffbfffc
> RBP: ffffa2fe2dd43db0 R08: ffffa2fe2dfcd000 R09: 0000000000000001
> R10: ffffffffa06245d0 R11: ffffa2fe14c03700 R12: 0000000000000000
> R13: ffffa2fe11e686c0 R14: ffffa2fe13fab100 R15: ffffa2fe129b8940
> FS:  0000000000000000(0000) GS:ffffa2fe2dd40000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fe26c457000 CR3: 000000002b609003 CR4: 00000000003606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <IRQ>
>  skb_release_all+0x1f/0x30
>  consume_skb+0x1d/0x40
>  __dev_kfree_skb_any+0x2a/0x30
>  stmmac_tx_clean+0x230/0x4d0 [stmmac]
>  stmmac_poll+0x4b/0x980 [stmmac]
>  net_rx_action+0x1ad/0x290
>  __do_softirq+0xdd/0x1d6
>  irq_exit+0x77/0x80
>  do_IRQ+0x4a/0xc0
>  common_interrupt+0x93/0x93
>  </IRQ>
> RIP: 0010:cpuidle_enter_state+0x16a/0x210
> RSP: 0018:ffffb8c6000bbe90 EFLAGS: 00000286 ORIG_RAX:
> ffffffffffffffae
> RAX: ffffa2fe2dd575c0 RBX: ffffa2fe14560200 RCX: 000000000000001f
> RDX: 0000000000000000 RSI: 00000122dbd7ae06 RDI: 0000000000000000
> RBP: ffffb8c6000bbec0 R08: 0000000000000020 R09: 0000000000000002
> R10: ffffb8c6000bbe60 R11: 0000000000123400 R12: 0000004f3c11b47a
> R13: 0000000000000003 R14: ffffffffa0e3aa58 R15: 0000000000000003
>  cpuidle_enter+0x12/0x20
>  call_cpuidle+0x1e/0x40
>  do_idle+0x16a/0x1c0
>  cpu_startup_entry+0x18/0x20
>  start_secondary+0x10d/0x110
>  secondary_startup_64+0xa5/0xa5
> ------------->8-------------
> 
> Using tree with your commit I get this stacktrace upon streaming
> data at random time (stacktrace does not appear everytime),
> without the commit I get no errors.
> 
> I understand the reason for your commit but we already have
> dirty_tx in stmmac_tx_clean(), shouldn't it be enough to prevent
> skb use-after-free? Can you please review your patch to make sure
> nothing else is missing?

Dirty is not enough.

The problem that I thought I solved,
was if an single skb was spread out over
3 tx descs like this:

TX DESC #1: first descriptor bit set
TX DESC #2: 
TX DESC #3: last descriptor bit set

before my patch the
tx_q->tx_skbuff[first_entry] would have the skb pointer.

let's say that curr_tx is pointing to TX DESC4,
since curr_tx is supposed to point to the place where
we are going to start filling next time.

The problem I thought I solved was if stmmac_tx_clean
came and started cleaning, it could clean TX DESC #1,
and thus freeing the skb (which TX DESC #2 and TX DESC #3 uses.)

However, by reading the databook more carefully:
If an Ethernet packet is stored over data buffers in multiple
descriptors, the DMA will fetch all descriptors, and
will only update the own bit in all descriptors,
after the complete packet has been sent.

Because of this, I think that the case where
TX DESC #1 gets its own bit cleared, while the hardware
hasn't yet fetched the TX DESC #2 and TX DESC #3 cannot
happen, so my patch 05cf0d1bf4 ("net: stmmac: free an skb first
when there are no longer any descriptors using it")
has no real purpose, and could be reverted.



However, I don't see why is should cause your
kernel to crash.

It looks like skb_release_data is trying to access a member in a pointer
that is null, i.e. a double free of an skb.

I assume this is because the same skb address exists in several queues.

My guess is that the problem is that tx_q->tx_skbuff[] does not get set to
NULL properly when having multiple queues. (It obviously works for a single
tx queue).

stmmac_tx_clean() will do:
if (skb)
    tx_q->tx_skbuff[entry] = NULL;

so skbs should always be set to NULL.

However, one difference seems to be that stmmac_xmit and stmmac_tso_xmit
inside the nfrags for-loop, seems to set the skb pointer to NULL.

for (i = 0; i < nfrags; i++) {
    tx_q->tx_skbuff[entry] = NULL;

Considering this is already done in stmmac_tx_clean, this seems redundant.

But one difference is that before my patch,
all frags, except first TX DESC, would get NULL:ed
(first desc would be assigned the skb).

After my patch, all descs, except first TX DESC,
would get NULL:ed, last desc would get assigned
the skb, but first desc is not explicitly cleared.

Could you try to see if the following patch
solves your problem:
(still don't see why it should be needed,
considering stmmac_tx_clean() should set all non-NULL
entries to NULL)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1763e48c84e2..1d30b20b3740 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2830,6 +2830,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 
        tx_q->tx_skbuff_dma[first_entry].buf = des;
        tx_q->tx_skbuff_dma[first_entry].len = skb_headlen(skb);
+       tx_q->tx_skbuff[first_entry] = NULL;
 
        first->des0 = cpu_to_le32(des);
 
@@ -3003,6 +3004,8 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
        first = desc;
 
+       tx_q->tx_skbuff[first_entry] = NULL;
+
        enh_desc = priv->plat->enh_desc;
        /* To program the descriptors according to the size of the frame */
        if (enh_desc)