Re: [PATCH] wcn36xx: handle rx skb allocation failure to avoid system crash

[Bob Copeland <me@bobcopeland.com>]

On Fri, Dec 11, 2015 at 09:14:04PM +0800, fengwei.yin wrote:
> 
> On 2015/12/2 13:27, Fengwei Yin wrote:
> >Lawrence reported that git clone could make system crash on a
> >Qualcomm ARM soc based device (DragonBoard, 1G memory without
> >swap) running 64bit Debian.
> >
> >It's turned out the crash is related with rx skb allocation
> >failure. git could consume more than 600MB anonymous memory.
> >And system is in extremely memory shortage case.
> >
> >But driver didn't handle the rx allocation failure case. This patch
> >doesn't submit skb to upper layer if rx skb allocation fails.
> >Instead, it reuse the old skb for rx DMA again. It's more like
> >drop the packets if system is in memory shortage case.
> >
> >With this change, git clone is OOMed instead of system crash.
> >
> >Reported-by: King, Lawrence <lking@qti.qualcomm.com>
> >Signed-off-by: Fengwei Yin <fengwei.yin@linaro.org>

Concept makes sense to me, but:

> >  		dma_addr = dxe->dst_addr_l;
> >-		wcn36xx_dxe_fill_skb(wcn->dev, ctl);
> >+		ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);
> >+		if (0 == ret) {

I find this "success handling" to be unclear and traditionally this
kind of thing is a source of bugs; how about instead:

> >+			/* new skb allocation ok. Use the new one and queue
> >+			 * the old one to network system.
> >+			 */
> >+			dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE,
> >+					DMA_FROM_DEVICE);
> >+			wcn36xx_rx_skb(wcn, skb);
> >+		}

           ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);

           /* skip this frame if we can't alloc a new rx buffer */
           if (ret)
                  goto drop;

> >  		switch (ch->ch_type) {
> >  		case WCN36XX_DXE_CH_RX_L:
> >@@ -495,9 +504,6 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
> >  			wcn36xx_warn("Unknown channel\n");
> >  		}
> >
> >-		dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE,
> >-				 DMA_FROM_DEVICE);
> >-		wcn36xx_rx_skb(wcn, skb);

drop:

> >  		ctl = ctl->next;
> >  		dxe = ctl->desc;
> >  	}

-- 
Bob Copeland %% http://bobcopeland.com/