* [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt()
@ 2012-09-10 19:22 Eric Dumazet
2012-09-10 19:40 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-09-10 19:22 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Larry Finger, netdev, Dave Jones
From: Eric Dumazet <edumazet@google.com>
64bit arches have a buggy r8712u driver, let's fix it.
skb->tail must be set properly or network stack behavior is undefined.
Addresses https://bugzilla.redhat.com/show_bug.cgi?id=847525
Addresses https://bugzilla.kernel.org/show_bug.cgi?id=45071
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Dave Jones <davej@redhat.com>
Cc: stable@vger.kernel.org [2.6.37+]
---
drivers/staging/rtl8712/recv_linux.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8712/recv_linux.c b/drivers/staging/rtl8712/recv_linux.c
index 0e26d5f..495ee12 100644
--- a/drivers/staging/rtl8712/recv_linux.c
+++ b/drivers/staging/rtl8712/recv_linux.c
@@ -117,13 +117,8 @@ void r8712_recv_indicatepkt(struct _adapter *padapter,
if (skb == NULL)
goto _recv_indicatepkt_drop;
skb->data = precv_frame->u.hdr.rx_data;
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
- skb->tail = (sk_buff_data_t)(precv_frame->u.hdr.rx_tail -
- precv_frame->u.hdr.rx_head);
-#else
- skb->tail = (sk_buff_data_t)precv_frame->u.hdr.rx_tail;
-#endif
skb->len = precv_frame->u.hdr.len;
+ skb_set_tail_pointer(skb, skb->len);
if ((pattrib->tcpchk_valid == 1) && (pattrib->tcp_chkrpt == 1))
skb->ip_summed = CHECKSUM_UNNECESSARY;
else
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt()
2012-09-10 19:22 [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt() Eric Dumazet
@ 2012-09-10 19:40 ` Larry Finger
0 siblings, 0 replies; 7+ messages in thread
From: Larry Finger @ 2012-09-10 19:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Greg Kroah-Hartman, netdev, Dave Jones
On 09/10/2012 02:22 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> 64bit arches have a buggy r8712u driver, let's fix it.
>
> skb->tail must be set properly or network stack behavior is undefined.
>
> Addresses https://bugzilla.redhat.com/show_bug.cgi?id=847525
> Addresses https://bugzilla.kernel.org/show_bug.cgi?id=45071
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Dave Jones <davej@redhat.com>
> Cc: stable@vger.kernel.org [2.6.37+]
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
Larry
> ---
> drivers/staging/rtl8712/recv_linux.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/recv_linux.c b/drivers/staging/rtl8712/recv_linux.c
> index 0e26d5f..495ee12 100644
> --- a/drivers/staging/rtl8712/recv_linux.c
> +++ b/drivers/staging/rtl8712/recv_linux.c
> @@ -117,13 +117,8 @@ void r8712_recv_indicatepkt(struct _adapter *padapter,
> if (skb == NULL)
> goto _recv_indicatepkt_drop;
> skb->data = precv_frame->u.hdr.rx_data;
> -#ifdef NET_SKBUFF_DATA_USES_OFFSET
> - skb->tail = (sk_buff_data_t)(precv_frame->u.hdr.rx_tail -
> - precv_frame->u.hdr.rx_head);
> -#else
> - skb->tail = (sk_buff_data_t)precv_frame->u.hdr.rx_tail;
> -#endif
> skb->len = precv_frame->u.hdr.len;
> + skb_set_tail_pointer(skb, skb->len);
> if ((pattrib->tcpchk_valid == 1) && (pattrib->tcp_chkrpt == 1))
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> else
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt()
@ 2012-09-12 17:52 Aki Parviainen
2012-09-12 18:25 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Aki Parviainen @ 2012-09-12 17:52 UTC (permalink / raw)
To: edumazet; +Cc: netdev
>From: Eric Dumazet <edumazet@google.com>
>
>64bit arches have a buggy r8712u driver, let's fix it.
>
>skb->tail must be set properly or network stack behavior is undefined.
>
>Addresses https://bugzilla.redhat.com/show_bug.cgi?id=847525
>Addresses https://bugzilla.kernel.org/show_bug.cgi?id=45071
>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Larry Finger <Larry.Finger@lwfinger.net>
>Cc: Dave Jones <davej@redhat.com>
>Cc: stable@vger.kernel.org [2.6.37+]
Tested this patch on 64bit linux-3.6-rc5 & fixes problem for me. Thank
you very much for fixing this very annoying & long time open bug!
Tested-by: Aki Parviainen <aki.parviainen@iki.fi>
Aki
>---
> drivers/staging/rtl8712/recv_linux.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
>diff --git a/drivers/staging/rtl8712/recv_linux.c b/drivers/staging/rtl8712/recv_linux.c
>index 0e26d5f..495ee12 100644
>--- a/drivers/staging/rtl8712/recv_linux.c
>+++ b/drivers/staging/rtl8712/recv_linux.c
>@@ -117,13 +117,8 @@ void r8712_recv_indicatepkt(struct _adapter *padapter,
> if (skb == NULL)
> goto _recv_indicatepkt_drop;
> skb->data = precv_frame->u.hdr.rx_data;
>-#ifdef NET_SKBUFF_DATA_USES_OFFSET
>- skb->tail = (sk_buff_data_t)(precv_frame->u.hdr.rx_tail -
>- precv_frame->u.hdr.rx_head);
>-#else
>- skb->tail = (sk_buff_data_t)precv_frame->u.hdr.rx_tail;
>-#endif
> skb->len = precv_frame->u.hdr.len;
>+ skb_set_tail_pointer(skb, skb->len);
> if ((pattrib->tcpchk_valid == 1) && (pattrib->tcp_chkrpt == 1))
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> else
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
@ 2012-08-23 20:57 Larry Finger
2012-08-23 21:26 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2012-08-23 20:57 UTC (permalink / raw)
To: Eric Dumazet
Cc: Neal Cardwell, David S. Miller, John W Linville, linux-wireless,
LKML
On 08/22/2012 11:03 PM, Eric Dumazet wrote:
>
> Changing the allocation size removes the problem ? thats really strange.
>
> If you try different sizes in the 9100-30720 range, can you pinpoint the
> failure threshold ?
The allocation size change did not fix the problem. It turned out that 10 tries
from a secure web page were not enough to trigger this intermittent problem that
particular test.
Based on DaveM's comment that skb->truesize could be wrong, I tried setting
truesize after every netdev_alloc_skb() call. Of course, that had no effect. I
then found https://lkml.org/lkml/2010/11/19/505I, which clearly states why this
need not be done.
What skb modifications require that truesize be adjusted? The driver never
resets skb->len or skb->data_len for any buffers, other than setting skb->len to
zero.
Thanks,
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-23 20:57 Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use" Larry Finger
@ 2012-08-23 21:26 ` Eric Dumazet
2012-08-24 14:09 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-08-23 21:26 UTC (permalink / raw)
To: Larry Finger
Cc: Neal Cardwell, David S. Miller, John W Linville, linux-wireless,
LKML
On Thu, 2012-08-23 at 15:57 -0500, Larry Finger wrote:
> On 08/22/2012 11:03 PM, Eric Dumazet wrote:
> >
> > Changing the allocation size removes the problem ? thats really strange.
> >
> > If you try different sizes in the 9100-30720 range, can you pinpoint the
> > failure threshold ?
>
> The allocation size change did not fix the problem. It turned out that 10 tries
> from a secure web page were not enough to trigger this intermittent problem that
> particular test.
>
> Based on DaveM's comment that skb->truesize could be wrong, I tried setting
> truesize after every netdev_alloc_skb() call. Of course, that had no effect. I
> then found https://lkml.org/lkml/2010/11/19/505I, which clearly states why this
> need not be done.
>
> What skb modifications require that truesize be adjusted? The driver never
> resets skb->len or skb->data_len for any buffers, other than setting skb->len to
> zero.
skb->truesize is adjusted when a frag is added to one skb, or when
skb->head is re-allocated.
Are you sure you dont have another problem, because as I said commit
c8628155ece3 had a bug, so a bisect is not very useful.
How many reloads are needed to trigger the bug, do you have a script to
reproduce it ?
Could it be a PMTU problem ? (check
http://git.kernel.org/?p=linux/kernel/git/davem/net.git;a=commit;h=9b04f350057863d1fad1ba071e09362a1da3503e )
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-23 21:26 ` Eric Dumazet
@ 2012-08-24 14:09 ` Larry Finger
2012-08-24 15:19 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2012-08-24 14:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: Neal Cardwell, David S. Miller, John W Linville, linux-wireless,
LKML
On 08/23/2012 04:26 PM, Eric Dumazet wrote:
>
> skb->truesize is adjusted when a frag is added to one skb, or when
> skb->head is re-allocated.
>
> Are you sure you dont have another problem, because as I said commit
> c8628155ece3 had a bug, so a bisect is not very useful.
>
> How many reloads are needed to trigger the bug, do you have a script to
> reproduce it ?
I was doing the tests with a browser; however, it seems that a wget of that same
page is much more effective at triggering the problem. Now I have a script that
tries 60 passes of
wget https://bugzilla.redhat.com/show_bug.cgi?id=847525
I usually get failures on the first or second try. A kernel built from "git
checkout c862815" gets the following wget output:
============================================================================
--2012-08-23 21:50:32-- https://bugzilla.redhat.com/show_bug.cgi?id=847525
Resolving bugzilla.redhat.com (bugzilla.redhat.com)... 209.132.183.69
Connecting to bugzilla.redhat.com (bugzilla.redhat.com)|209.132.183.69|:443...
connected.
OpenSSL: error:0D0680A8:asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag
OpenSSL: error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 error
OpenSSL: error:0D08303A:asn1 encoding routines:ASN1_TEMPLATE_NOEXP_D2I:nested
asn1 error
OpenSSL: error:0D08303A:asn1 encoding routines:ASN1_TEMPLATE_NOEXP_D2I:nested
asn1 error
OpenSSL: error:1409000D:SSL routines:SSL3_GET_SERVER_CERTIFICATE:ASN1 lib
Unable to establish SSL connection.
============================================================================
With kernel 3.6-rc2, the error changes to the following:
============================================================================
--2012-08-24 08:26:42-- https://bugzilla.redhat.com/show_bug.cgi?id=847525
Resolving bugzilla.redhat.com (bugzilla.redhat.com)... 209.132.183.69
Connecting to bugzilla.redhat.com (bugzilla.redhat.com)|209.132.183.69|:443...
connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/html]
Saving to: `show_bug.cgi?id=847525.3'
[ <=> ] 0 --.-K/s in 0s
2012-08-24 08:26:45 (0.00 B/s) - Read error at byte 0 (error:1408F119:SSL
routines:SSL3_GET_RECORD:decryption failed or bad record mac).Retrying.
============================================================================
> Could it be a PMTU problem ? (check
> http://git.kernel.org/?p=linux/kernel/git/davem/net.git;a=commit;h=9b04f350057863d1fad1ba071e09362a1da3503e )
I added the PMTU patch to kernel 3.6-rc2 - nothing changed.
Anything else to try?
Thanks,
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 14:09 ` Larry Finger
@ 2012-08-24 15:19 ` David Miller
2012-08-24 15:58 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2012-08-24 15:19 UTC (permalink / raw)
To: Larry.Finger
Cc: eric.dumazet, ncardwell, linville, linux-wireless, linux-kernel
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Fri, 24 Aug 2012 09:09:57 -0500
> I usually get failures on the first or second try. A kernel built from
> "git checkout c862815" gets the following wget output:
>
> ============================================================================
> --2012-08-23 21:50:32--
> --https://bugzilla.redhat.com/show_bug.cgi?id=847525
> Resolving bugzilla.redhat.com (bugzilla.redhat.com)... 209.132.183.69
> Connecting to bugzilla.redhat.com
> (bugzilla.redhat.com)|209.132.183.69|:443... connected.
> OpenSSL: error:0D0680A8:asn1 encoding routines:ASN1_CHECK_TLEN:wrong
> tag
> OpenSSL: error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested
> asn1 error
> OpenSSL: error:0D08303A:asn1 encoding
> routines:ASN1_TEMPLATE_NOEXP_D2I:nested asn1 error
> OpenSSL: error:0D08303A:asn1 encoding
> routines:ASN1_TEMPLATE_NOEXP_D2I:nested asn1 error
> OpenSSL: error:1409000D:SSL routines:SSL3_GET_SERVER_CERTIFICATE:ASN1
> lib
> Unable to establish SSL connection.
> ============================================================================
This looks like full-on data corruption to me.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 15:19 ` David Miller
@ 2012-08-24 15:58 ` Larry Finger
2012-08-24 16:18 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2012-08-24 15:58 UTC (permalink / raw)
To: David Miller
Cc: eric.dumazet, ncardwell, linville, linux-wireless, linux-kernel
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.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 15:58 ` Larry Finger
@ 2012-08-24 16:18 ` Eric Dumazet
2012-08-24 16:23 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-08-24 16:18 UTC (permalink / raw)
To: Larry Finger
Cc: David Miller, ncardwell, linville, linux-wireless, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 16:18 ` Eric Dumazet
@ 2012-08-24 16:23 ` Eric Dumazet
2012-08-24 16:58 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-08-24 16:23 UTC (permalink / raw)
To: Larry Finger
Cc: David Miller, ncardwell, linville, linux-wireless, linux-kernel
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);
}
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 16:23 ` Eric Dumazet
@ 2012-08-24 16:58 ` Larry Finger
2012-08-24 17:47 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2012-08-24 16:58 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, ncardwell, linville, linux-wireless, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 16:58 ` Larry Finger
@ 2012-08-24 17:47 ` Eric Dumazet
2012-08-27 17:55 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-08-24 17:47 UTC (permalink / raw)
To: Larry Finger
Cc: David Miller, ncardwell, linville, linux-wireless, linux-kernel
On Fri, 2012-08-24 at 11:58 -0500, Larry Finger wrote:
> 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);
dev_alloc_skb() can return NULL
-> crash
skb_clone() can also return NULL
-> crash
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-24 17:47 ` Eric Dumazet
@ 2012-08-27 17:55 ` Larry Finger
2012-09-10 8:39 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2012-08-27 17:55 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, ncardwell, linville, linux-wireless, linux-kernel
On 08/24/2012 12:47 PM, Eric Dumazet wrote:
>
> dev_alloc_skb() can return NULL
>
> -> crash
>
> skb_clone() can also return NULL
>
> -> crash
I have prepared a patch to fix all the unchecked allocations.
Over the weekend I made some progress. To test the latest vendor driver, I
installed a 32-bit system. Their driver is not compatible with a 64-bit system.
I found that not only did the vendor driver work with secure sites, but so did
the in-kernel version. I now have tcpdump output for the 32-bit case that works,
and the 64-bit case that fails. It seems likely that I missed some 32/64 bit
incompatibility when I did the conversion.
Thanks for all your help in trying to resolve this issue.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-08-27 17:55 ` Larry Finger
@ 2012-09-10 8:39 ` Eric Dumazet
2012-09-10 14:53 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-09-10 8:39 UTC (permalink / raw)
To: Larry Finger; +Cc: linville, linux-wireless, netdev
On Mon, 2012-08-27 at 12:55 -0500, Larry Finger wrote:
> I have prepared a patch to fix all the unchecked allocations.
>
> Over the weekend I made some progress. To test the latest vendor driver, I
> installed a 32-bit system. Their driver is not compatible with a 64-bit system.
> I found that not only did the vendor driver work with secure sites, but so did
> the in-kernel version. I now have tcpdump output for the 32-bit case that works,
> and the 64-bit case that fails. It seems likely that I missed some 32/64 bit
> incompatibility when I did the conversion.
>
> Thanks for all your help in trying to resolve this issue.
>
> Larry
>
>
Hi Larry
It appears I have a D-Link N300 (DWA-131) nano USB adapter, using
staging/rtl8712 driver.
I tried many kernel versions (including 3.3) and none seems to work
reliably.
Sometime, I have some traffic but only for about 50 frames...
It might be because my access point is a netgear wndr3800, because I
have following warning a bit before the freezes :
r8712u: [r8712_got_addbareq_event_callback] mac = 20:4e:7f:5a:cd:30, sea = 80, tid = 0
Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-09-10 8:39 ` Eric Dumazet
@ 2012-09-10 14:53 ` Larry Finger
2012-09-10 15:04 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Larry Finger @ 2012-09-10 14:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linville, linux-wireless, netdev
On 09/10/2012 03:39 AM, Eric Dumazet wrote:
> On Mon, 2012-08-27 at 12:55 -0500, Larry Finger wrote:
>
>> I have prepared a patch to fix all the unchecked allocations.
>>
>> Over the weekend I made some progress. To test the latest vendor driver, I
>> installed a 32-bit system. Their driver is not compatible with a 64-bit system.
>> I found that not only did the vendor driver work with secure sites, but so did
>> the in-kernel version. I now have tcpdump output for the 32-bit case that works,
>> and the 64-bit case that fails. It seems likely that I missed some 32/64 bit
>> incompatibility when I did the conversion.
>>
>> Thanks for all your help in trying to resolve this issue.
>>
>> Larry
>>
>>
>
> Hi Larry
>
> It appears I have a D-Link N300 (DWA-131) nano USB adapter, using
> staging/rtl8712 driver.
>
> I tried many kernel versions (including 3.3) and none seems to work
> reliably.
>
> Sometime, I have some traffic but only for about 50 frames...
> It might be because my access point is a netgear wndr3800, because I
> have following warning a bit before the freezes :
>
> r8712u: [r8712_got_addbareq_event_callback] mac = 20:4e:7f:5a:cd:30, sea = 80, tid = 0
Eric,
What is the md5sum for the firmware file /lib/firmware/rtlwifi/rtl8712u.bin?
Over the weekend, there was a report of another device that had problems with
firmware that was added to the linux-firmware repo in July with md5sum of
c6f3b7b880aefb7b3f249428d659bdbb. An older version with md5sum of
200fd952db3cc9259b1fd05e3e51966f works in that case. I'll send you this one
privately.
I have a Netgear WNDR3300 and also get the addbareq events, but I do not get
freezes. I'm not sure the message is correlated.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use"
2012-09-10 14:53 ` Larry Finger
@ 2012-09-10 15:04 ` Eric Dumazet
2012-09-10 17:55 ` [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt() Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-09-10 15:04 UTC (permalink / raw)
To: Larry Finger; +Cc: linville, linux-wireless, netdev
On Mon, 2012-09-10 at 09:53 -0500, Larry Finger wrote:
> Eric,
>
> What is the md5sum for the firmware file /lib/firmware/rtlwifi/rtl8712u.bin?
> Over the weekend, there was a report of another device that had problems with
> firmware that was added to the linux-firmware repo in July with md5sum of
> c6f3b7b880aefb7b3f249428d659bdbb. An older version with md5sum of
> 200fd952db3cc9259b1fd05e3e51966f works in that case. I'll send you this one
> privately.
>
> I have a Netgear WNDR3300 and also get the addbareq events, but I do not get
> freezes. I'm not sure the message is correlated.
>
It seems I have the c6f3b7b880aefb7b3f249428d659bdbb one
# md5sum /data/src/linux-firmware/rtlwifi/rtl8712u.bin /lib/firmware/rtlwifi/rtl8712u.bin
c6f3b7b880aefb7b3f249428d659bdbb /data/src/linux-firmware/rtlwifi/rtl8712u.bin
c6f3b7b880aefb7b3f249428d659bdbb /lib/firmware/rtlwifi/rtl8712u.bin
Since I have linux-firmware tree, should I go back to initial commit
commit 8f919160792e4702c6b7a67a243cea4f757407e4
Author: Larry Finger <Larry.Finger@lwfinger.net>
Date: Mon Nov 1 23:56:52 2010 -0500
linux-firmware: Add firmware files for Realtek RTL8712U and
RTL8192CE
Thanks !
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt()
2012-09-10 15:04 ` Eric Dumazet
@ 2012-09-10 17:55 ` Eric Dumazet
2012-09-10 18:34 ` Larry Finger
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2012-09-10 17:55 UTC (permalink / raw)
To: Larry Finger; +Cc: linville, linux-wireless, netdev
From: Eric Dumazet <edumazet@google.com>
64bit arches have a buggy r8712u driver, let's fix it.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
drivers/staging/rtl8712/recv_linux.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8712/recv_linux.c b/drivers/staging/rtl8712/recv_linux.c
index 0e26d5f..495ee12 100644
--- a/drivers/staging/rtl8712/recv_linux.c
+++ b/drivers/staging/rtl8712/recv_linux.c
@@ -117,13 +117,8 @@ void r8712_recv_indicatepkt(struct _adapter *padapter,
if (skb == NULL)
goto _recv_indicatepkt_drop;
skb->data = precv_frame->u.hdr.rx_data;
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
- skb->tail = (sk_buff_data_t)(precv_frame->u.hdr.rx_tail -
- precv_frame->u.hdr.rx_head);
-#else
- skb->tail = (sk_buff_data_t)precv_frame->u.hdr.rx_tail;
-#endif
skb->len = precv_frame->u.hdr.len;
+ skb_set_tail_pointer(skb, skb->len);
if ((pattrib->tcpchk_valid == 1) && (pattrib->tcp_chkrpt == 1))
skb->ip_summed = CHECKSUM_UNNECESSARY;
else
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt()
2012-09-10 17:55 ` [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt() Eric Dumazet
@ 2012-09-10 18:34 ` Larry Finger
0 siblings, 0 replies; 7+ messages in thread
From: Larry Finger @ 2012-09-10 18:34 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linville, linux-wireless, netdev, Greg Kroah-Hartman
On 09/10/2012 12:55 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> 64bit arches have a buggy r8712u driver, let's fix it.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> drivers/staging/rtl8712/recv_linux.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/recv_linux.c b/drivers/staging/rtl8712/recv_linux.c
> index 0e26d5f..495ee12 100644
> --- a/drivers/staging/rtl8712/recv_linux.c
> +++ b/drivers/staging/rtl8712/recv_linux.c
> @@ -117,13 +117,8 @@ void r8712_recv_indicatepkt(struct _adapter *padapter,
> if (skb == NULL)
> goto _recv_indicatepkt_drop;
> skb->data = precv_frame->u.hdr.rx_data;
> -#ifdef NET_SKBUFF_DATA_USES_OFFSET
> - skb->tail = (sk_buff_data_t)(precv_frame->u.hdr.rx_tail -
> - precv_frame->u.hdr.rx_head);
> -#else
> - skb->tail = (sk_buff_data_t)precv_frame->u.hdr.rx_tail;
> -#endif
> skb->len = precv_frame->u.hdr.len;
> + skb_set_tail_pointer(skb, skb->len);
> if ((pattrib->tcpchk_valid == 1) && (pattrib->tcp_chkrpt == 1))
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> else
Eric,
Thanks for this. It works for me. I had looked at this code a number of times,
but for some reason, I thought that the u.hdr.rx_yyyy parameters were like
32-bit systems and that all 4 values were pointers, and not like 64-bit systems,
thus the funny conversion. Strange that the bug was never triggered until commit
c8628155ece3 - this code has not changed since the driver was accepted into 2.6.37.
A few points on the patch. As the driver is in staging, the patch needs to go to
Greg Kroah-Hartman. In addition, please add the "Cc: Stable
<stable@vger.kernel.org>" line. You may also give an a s-o-b for me. Finally,
this patch should fix https://bugzilla.redhat.com/show_bug.cgi?id=847525, and
https://bugzilla.kernel.org/show_bug.cgi?id=45071.
Larry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt()
@ 2012-09-10 18:34 ` Larry Finger
0 siblings, 0 replies; 7+ messages in thread
From: Larry Finger @ 2012-09-10 18:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: linville-2XuSBdqkA4R54TAoqtyWWQ,
linux-wireless-u79uwXL29TY76Z2rM5mHXA, netdev, Greg Kroah-Hartman
On 09/10/2012 12:55 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> 64bit arches have a buggy r8712u driver, let's fix it.
>
> Signed-off-by: Eric Dumazet <edumazet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> drivers/staging/rtl8712/recv_linux.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/recv_linux.c b/drivers/staging/rtl8712/recv_linux.c
> index 0e26d5f..495ee12 100644
> --- a/drivers/staging/rtl8712/recv_linux.c
> +++ b/drivers/staging/rtl8712/recv_linux.c
> @@ -117,13 +117,8 @@ void r8712_recv_indicatepkt(struct _adapter *padapter,
> if (skb == NULL)
> goto _recv_indicatepkt_drop;
> skb->data = precv_frame->u.hdr.rx_data;
> -#ifdef NET_SKBUFF_DATA_USES_OFFSET
> - skb->tail = (sk_buff_data_t)(precv_frame->u.hdr.rx_tail -
> - precv_frame->u.hdr.rx_head);
> -#else
> - skb->tail = (sk_buff_data_t)precv_frame->u.hdr.rx_tail;
> -#endif
> skb->len = precv_frame->u.hdr.len;
> + skb_set_tail_pointer(skb, skb->len);
> if ((pattrib->tcpchk_valid == 1) && (pattrib->tcp_chkrpt == 1))
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> else
Eric,
Thanks for this. It works for me. I had looked at this code a number of times,
but for some reason, I thought that the u.hdr.rx_yyyy parameters were like
32-bit systems and that all 4 values were pointers, and not like 64-bit systems,
thus the funny conversion. Strange that the bug was never triggered until commit
c8628155ece3 - this code has not changed since the driver was accepted into 2.6.37.
A few points on the patch. As the driver is in staging, the patch needs to go to
Greg Kroah-Hartman. In addition, please add the "Cc: Stable
<stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>" line. You may also give an a s-o-b for me. Finally,
this patch should fix https://bugzilla.redhat.com/show_bug.cgi?id=847525, and
https://bugzilla.kernel.org/show_bug.cgi?id=45071.
Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-09-12 18:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-10 19:22 [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt() Eric Dumazet
2012-09-10 19:40 ` Larry Finger
-- strict thread matches above, loose matches on Subject: below --
2012-09-12 17:52 Aki Parviainen
2012-09-12 18:25 ` Eric Dumazet
2012-08-23 20:57 Regression associated with commit c8628155ece3 - "tcp: reduce out_of_order memory use" Larry Finger
2012-08-23 21:26 ` Eric Dumazet
2012-08-24 14:09 ` Larry Finger
2012-08-24 15:19 ` David Miller
2012-08-24 15:58 ` Larry Finger
2012-08-24 16:18 ` Eric Dumazet
2012-08-24 16:23 ` Eric Dumazet
2012-08-24 16:58 ` Larry Finger
2012-08-24 17:47 ` Eric Dumazet
2012-08-27 17:55 ` Larry Finger
2012-09-10 8:39 ` Eric Dumazet
2012-09-10 14:53 ` Larry Finger
2012-09-10 15:04 ` Eric Dumazet
2012-09-10 17:55 ` [PATCH] staging: r8712u: fix bug in r8712_recv_indicatepkt() Eric Dumazet
2012-09-10 18:34 ` Larry Finger
2012-09-10 18:34 ` Larry Finger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.