Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"

[Larry Finger <Larry.Finger@lwfinger.net>]

On 08/24/2012 11:23 AM, Eric Dumazet wrote:
> On Fri, 2012-08-24 at 18:18 +0200, Eric Dumazet wrote:
>> On Fri, 2012-08-24 at 10:58 -0500, Larry Finger wrote:
>>> On 08/24/2012 10:19 AM, David Miller wrote:
>>>>
>>>> This looks like full-on data corruption to me.
>>>
>>> I agree. The question is why does it happen with r8712u, and only after the
>>> commit in the subject. Drivers for other devices that I have are OK. Thus far, I
>>> have tested b43, rtl8187, ath9k_htc, and rtl8192cu. To my knowledge, there are
>>> no reports posted for this bug with any other device.
>>
>> bugs can sit unnoticed, and one change somewhere can uncover them.
>>
>> Really this driver must have a bug, if not half a dozen of bugs.
>>
>> For example this sequence of code is a clear bug :
>>
>> sub_skb = dev_alloc_skb(nSubframe_Length + 12);
>> skb_reserve(sub_skb, 12);
>>
>>
>> Also the free_recv_skb_queue looks really suspect to me
>>
>> What the hell is doing recv_tasklet() I really wonder.
>>
>> This code, combined with the skb_clone() in recvbuf2recvframe()
>> can clearly reuse an skb passed to upper stacks.
>>
>>
>> queueing one skb in free_recv_skb_queue should be done
>> only if no clone of this skb exist somewhere.
>>
>> Please someone fix this buggy driver.
>>
>
> Try the following patch for a start
>
> diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
> index 8e82ce2..88e3ca6 100644
> --- a/drivers/staging/rtl8712/rtl8712_recv.c
> +++ b/drivers/staging/rtl8712/rtl8712_recv.c
> @@ -1127,6 +1127,9 @@ static void recv_tasklet(void *priv)
>   		recvbuf2recvframe(padapter, pskb);
>   		skb_reset_tail_pointer(pskb);
>   		pskb->len = 0;
> -		skb_queue_tail(&precvpriv->free_recv_skb_queue, pskb);
> +		if (!skb_cloned(pskb))
> +			skb_queue_tail(&precvpriv->free_recv_skb_queue, pskb);
> +		else
> +			consume_skb(pskb);
>   	}
>   }

This one did not help. There is no doubt it is needed for the case where memory 
is tight, an allocation fails, and the driver clones the skb. In the present 
case, debug statements have shown that the skb_clone() call was not made.

In the long term, this driver will be replaced with one that uses mac80211, but 
in the short term, I am trying to fix it.

As I said earlier, my skb skills are minimal. Could you explain what is wrong 
with the following sequence?

  sub_skb = dev_alloc_skb(nSubframe_Length + 12);
  skb_reserve(sub_skb, 12);

Thanks,

Larry