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

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

[Fengwei Yin]

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>
---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index f8dfa05..8887c0f 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -474,11 +474,20 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
 	struct wcn36xx_dxe_desc *dxe = ctl->desc;
 	dma_addr_t  dma_addr;
 	struct sk_buff *skb;
+	int ret = 0;
 
 	while (!(dxe->ctrl & WCN36XX_DXE_CTRL_VALID_MASK)) {
 		skb = ctl->skb;
 		dma_addr = dxe->dst_addr_l;
-		wcn36xx_dxe_fill_skb(wcn->dev, ctl);
+		ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);
+		if (0 == ret) {
+			/* 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);
+		}
 
 		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);
 		ctl = ctl->next;
 		dxe = ctl->desc;
 	}
-- 
2.1.4

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

[fengwei.yin]

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>
> ---
>   drivers/net/wireless/ath/wcn36xx/dxe.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
> index f8dfa05..8887c0f 100644
> --- a/drivers/net/wireless/ath/wcn36xx/dxe.c
> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
> @@ -474,11 +474,20 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
>   	struct wcn36xx_dxe_desc *dxe = ctl->desc;
>   	dma_addr_t  dma_addr;
>   	struct sk_buff *skb;
> +	int ret = 0;
>
>   	while (!(dxe->ctrl & WCN36XX_DXE_CTRL_VALID_MASK)) {
>   		skb = ctl->skb;
>   		dma_addr = dxe->dst_addr_l;
> -		wcn36xx_dxe_fill_skb(wcn->dev, ctl);
> +		ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);
> +		if (0 == ret) {
> +			/* 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);
> +		}
>
>   		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);
>   		ctl = ctl->next;
>   		dxe = ctl->desc;
>   	}
>

Ping.... I am sure this is a fix according to the test I did.

Regards
Yin, Fengwei

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

[Bob Copeland]

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/

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

[fengwei.yin]

On 2015/12/11 21:37, Bob Copeland wrote:
> 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:
Thanks for looking at it.

>
>>>   		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;
This can't work because we need to initialize the DMA for the old skb again.
Which is done in following
	switch (ch->ch_type) {
block.

Regards
Yin, Fengwei

>
>>>   		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;
>>>   	}
>

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

[Bob Copeland]

On Fri, Dec 11, 2015 at 09:44:54PM +0800, fengwei.yin wrote:
> >            /* skip this frame if we can't alloc a new rx buffer */
> >            if (ret)
> >                   goto drop;
> This can't work because we need to initialize the DMA for the old skb again.
> Which is done in following
> 	switch (ch->ch_type) {
> block.

Hmm, good point.  You could still move that out to a function like this:

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index f8dfa05..fd447bf 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -467,6 +467,27 @@ out_err:
 
 }
 
+/* or whatever name makes sense... */
+static void wcn36xx_restart_dma(struct wcn36xx *wcn,
+				struct wcn36xx_dxe_ch *ch,
+				struct wcn36xx_dxe_desc *dxe)
+{
+	switch (ch->ch_type) {
+	case WCN36XX_DXE_CH_RX_L:
+		dxe->ctrl = WCN36XX_DXE_CTRL_RX_L;
+		wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
+					   WCN36XX_DXE_INT_CH1_MASK);
+		break;
+	case WCN36XX_DXE_CH_RX_H:
+		dxe->ctrl = WCN36XX_DXE_CTRL_RX_H;
+		wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
+					   WCN36XX_DXE_INT_CH3_MASK);
+		break;
+	default:
+		wcn36xx_warn("Unknown channel\n");
+	}
+}
+
 static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
 				     struct wcn36xx_dxe_ch *ch)
 {
@@ -478,26 +499,18 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
 	while (!(dxe->ctrl & WCN36XX_DXE_CTRL_VALID_MASK)) {
 		skb = ctl->skb;
 		dma_addr = dxe->dst_addr_l;
-		wcn36xx_dxe_fill_skb(wcn->dev, ctl);
+		ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);
 
-		switch (ch->ch_type) {
-		case WCN36XX_DXE_CH_RX_L:
-			dxe->ctrl = WCN36XX_DXE_CTRL_RX_L;
-			wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
-						   WCN36XX_DXE_INT_CH1_MASK);
-			break;
-		case WCN36XX_DXE_CH_RX_H:
-			dxe->ctrl = WCN36XX_DXE_CTRL_RX_H;
-			wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
-						   WCN36XX_DXE_INT_CH3_MASK);
-			break;
-		default:
-			wcn36xx_warn("Unknown channel\n");
-		}
+		/* skip this frame in OOM condition */
+		if (ret)
+			goto drop;
 
 		dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE,
 				 DMA_FROM_DEVICE);
 		wcn36xx_rx_skb(wcn, skb);
+
+drop:
+		wcn36xx_restart_dma(wcn, ch, dxe);
 		ctl = ctl->next;
 		dxe = ctl->desc;
 	}



...that said, not really sure it's worth it now that the 'goto' is only
skipping two lines.  So, I would be ok with the original patch too.

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

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

[fengwei.yin]

On 2015/12/11 22:08, Bob Copeland wrote:
> On Fri, Dec 11, 2015 at 09:44:54PM +0800, fengwei.yin wrote:
>>>             /* skip this frame if we can't alloc a new rx buffer */
>>>             if (ret)
>>>                    goto drop;
>> This can't work because we need to initialize the DMA for the old skb again.
>> Which is done in following
>> 	switch (ch->ch_type) {
>> block.
>
> Hmm, good point.  You could still move that out to a function like this:
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
> index f8dfa05..fd447bf 100644
> --- a/drivers/net/wireless/ath/wcn36xx/dxe.c
> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
> @@ -467,6 +467,27 @@ out_err:
>
>   }
>
> +/* or whatever name makes sense... */
> +static void wcn36xx_restart_dma(struct wcn36xx *wcn,
> +				struct wcn36xx_dxe_ch *ch,
> +				struct wcn36xx_dxe_desc *dxe)
> +{
> +	switch (ch->ch_type) {
> +	case WCN36XX_DXE_CH_RX_L:
> +		dxe->ctrl = WCN36XX_DXE_CTRL_RX_L;
> +		wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> +					   WCN36XX_DXE_INT_CH1_MASK);
> +		break;
> +	case WCN36XX_DXE_CH_RX_H:
> +		dxe->ctrl = WCN36XX_DXE_CTRL_RX_H;
> +		wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> +					   WCN36XX_DXE_INT_CH3_MASK);
> +		break;
> +	default:
> +		wcn36xx_warn("Unknown channel\n");
> +	}
> +}
> +
>   static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
>   				     struct wcn36xx_dxe_ch *ch)
>   {
> @@ -478,26 +499,18 @@ static int wcn36xx_rx_handle_packets(struct wcn36xx *wcn,
>   	while (!(dxe->ctrl & WCN36XX_DXE_CTRL_VALID_MASK)) {
>   		skb = ctl->skb;
>   		dma_addr = dxe->dst_addr_l;
> -		wcn36xx_dxe_fill_skb(wcn->dev, ctl);
> +		ret = wcn36xx_dxe_fill_skb(wcn->dev, ctl);
>
> -		switch (ch->ch_type) {
> -		case WCN36XX_DXE_CH_RX_L:
> -			dxe->ctrl = WCN36XX_DXE_CTRL_RX_L;
> -			wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> -						   WCN36XX_DXE_INT_CH1_MASK);
> -			break;
> -		case WCN36XX_DXE_CH_RX_H:
> -			dxe->ctrl = WCN36XX_DXE_CTRL_RX_H;
> -			wcn36xx_dxe_write_register(wcn, WCN36XX_DXE_ENCH_ADDR,
> -						   WCN36XX_DXE_INT_CH3_MASK);
> -			break;
> -		default:
> -			wcn36xx_warn("Unknown channel\n");
> -		}
> +		/* skip this frame in OOM condition */
> +		if (ret)
> +			goto drop;
>
>   		dma_unmap_single(wcn->dev, dma_addr, WCN36XX_PKT_SIZE,
>   				 DMA_FROM_DEVICE);
>   		wcn36xx_rx_skb(wcn, skb);
> +
> +drop:
> +		wcn36xx_restart_dma(wcn, ch, dxe);
>   		ctl = ctl->next;
>   		dxe = ctl->desc;
>   	}
>
>
>
> ...that said, not really sure it's worth it now that the 'goto' is only
> skipping two lines.  So, I would be ok with the original patch too.
>
I don't want to introduce "goto". But I really like your choice to create
wcn36xx_restart_dma. I will keep some original patch to avoid "goto" and
adopt the function wcn36xx_restart_dma. Will send the patch out.

Regards
Yin, Fengwei