2015-05-20 16:39:33

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 05/20/2015 01:17 AM, Haggai Eran wrote:
> On May 19, 2015 08:47, "Haggai Eran" <[email protected]
> <mailto:[email protected]>> wrote:
> >
> > With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
> > packet are set, but the packet length is too large. Truncate the packet
> > to prevent memory corruption.
> >
> > Signed-off-by: Haggai Eran <[email protected] <mailto:[email protected]>>
> > ---
> >
> > Hi,
> >
> > I think this solves the issue for me. I'll test it more thoroughly later. I
> > still don't know why a fragmented packet has such a large pkt_len value though.
> >
> > Thanks,
> > Haggai
> >
>
> I guess I was too quick with this patch. It prevents the kernel page faults, but
> with it I still see sometimes the connectivity disappear for a minute or two.

Is anything logged when that happens?

I'm still trying to see where that magic number of 1658 comes from, and how that
affects the RX buffer size.

When I unconditionally set alloc_sz to tmp_len as in the attached patch (I
remembered to refresh it this time), nothing bad has happened here yet. What
happens on your box?

Larry



Attachments:
rtl8712_prevent_buffer_overrun (4.89 kB)

2015-05-20 19:20:04

by Haggai Eran

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 20 May 2015 at 19:39, Larry Finger <[email protected]> wrote:
> On 05/20/2015 01:17 AM, Haggai Eran wrote:
>>
>> On May 19, 2015 08:47, "Haggai Eran" <[email protected]
>> <mailto:[email protected]>> wrote:
>> >
>> > With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
>> > packet are set, but the packet length is too large. Truncate the packet
>> > to prevent memory corruption.
>> >
>> > Signed-off-by: Haggai Eran <[email protected]
>> <mailto:[email protected]>>
>> > ---
>> >
>> > Hi,
>> >
>> > I think this solves the issue for me. I'll test it more thoroughly
>> later. I
>> > still don't know why a fragmented packet has such a large pkt_len value
>> though.
>> >
>> > Thanks,
>> > Haggai
>> >
>>
>> I guess I was too quick with this patch. It prevents the kernel page
>> faults, but
>> with it I still see sometimes the connectivity disappear for a minute or
>> two.
>
>
> Is anything logged when that happens?
No. I get once in a while the other corrupted entries I told you
about, but nothing special to these freezes

> I'm still trying to see where that magic number of 1658 comes from, and how
> that affects the RX buffer size.

I tried to look at the new driver (rtl8192su), but it doesn't seem to
handle this more-fragment bit at all.

> When I unconditionally set alloc_sz to tmp_len as in the attached patch (I
> remembered to refresh it this time), nothing bad has happened here yet. What
> happens on your box?

The same freezes still occur.

Thanks,
Haggai

2015-05-23 17:48:29

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 05/23/2015 12:24 PM, Haggai Eran wrote:
> On 20 May 2015 at 22:20, Haggai Eran <[email protected]> wrote:
>> On 20 May 2015 at 19:39, Larry Finger <[email protected]> wrote:
>>> On 05/20/2015 01:17 AM, Haggai Eran wrote:
>>>>
>>>> On May 19, 2015 08:47, "Haggai Eran" <[email protected]
>>>> <mailto:[email protected]>> wrote:
>>>> >
>>>> > With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
>>>> > packet are set, but the packet length is too large. Truncate the packet
>>>> > to prevent memory corruption.
>>>> >
>>>> > Signed-off-by: Haggai Eran <[email protected]
>>>> <mailto:[email protected]>>
>>>> > ---
>>>> >
>>>> > Hi,
>>>> >
>>>> > I think this solves the issue for me. I'll test it more thoroughly
>>>> later. I
>>>> > still don't know why a fragmented packet has such a large pkt_len value
>>>> though.
>>>> >
>>>> > Thanks,
>>>> > Haggai
>>>> >
>>>>
>>>> I guess I was too quick with this patch. It prevents the kernel page
>>>> faults, but
>>>> with it I still see sometimes the connectivity disappear for a minute or
>>>> two.
>>>
>>>
>>> Is anything logged when that happens?
>> No. I get once in a while the other corrupted entries I told you
>> about, but nothing special to these freezes
>>
>>> I'm still trying to see where that magic number of 1658 comes from, and how
>>> that affects the RX buffer size.
>>
>> I tried to look at the new driver (rtl8192su), but it doesn't seem to
>> handle this more-fragment bit at all.
>>
>>> When I unconditionally set alloc_sz to tmp_len as in the attached patch (I
>>> remembered to refresh it this time), nothing bad has happened here yet. What
>>> happens on your box?
>>
>> The same freezes still occur.
>
> I think the freezes I saw weren't related to the same issue. I was
> running a debugging kernel, and I saw the same freezes also with a
> different wifi adaptor. After switching to a non-debugging kernel, and
> using your patch, the freezes stopped.

That is good news. Perhaps the debugging kernel is overflowing the stack. Did
your debugging kernel have "Check for stack overflows" set in the "Memory
Debugging" section of the configuration? I did not have that turned on until
today, but it seems like a good idea.

Do you want to prepare the final version of the patch, or should I?

Larry


2015-05-23 17:24:19

by Haggai Eran

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 20 May 2015 at 22:20, Haggai Eran <[email protected]> wrote:
> On 20 May 2015 at 19:39, Larry Finger <[email protected]> wrote:
>> On 05/20/2015 01:17 AM, Haggai Eran wrote:
>>>
>>> On May 19, 2015 08:47, "Haggai Eran" <[email protected]
>>> <mailto:[email protected]>> wrote:
>>> >
>>> > With an RTL8191SU USB adaptor, sometimes the hints for a fragmented
>>> > packet are set, but the packet length is too large. Truncate the packet
>>> > to prevent memory corruption.
>>> >
>>> > Signed-off-by: Haggai Eran <[email protected]
>>> <mailto:[email protected]>>
>>> > ---
>>> >
>>> > Hi,
>>> >
>>> > I think this solves the issue for me. I'll test it more thoroughly
>>> later. I
>>> > still don't know why a fragmented packet has such a large pkt_len value
>>> though.
>>> >
>>> > Thanks,
>>> > Haggai
>>> >
>>>
>>> I guess I was too quick with this patch. It prevents the kernel page
>>> faults, but
>>> with it I still see sometimes the connectivity disappear for a minute or
>>> two.
>>
>>
>> Is anything logged when that happens?
> No. I get once in a while the other corrupted entries I told you
> about, but nothing special to these freezes
>
>> I'm still trying to see where that magic number of 1658 comes from, and how
>> that affects the RX buffer size.
>
> I tried to look at the new driver (rtl8192su), but it doesn't seem to
> handle this more-fragment bit at all.
>
>> When I unconditionally set alloc_sz to tmp_len as in the attached patch (I
>> remembered to refresh it this time), nothing bad has happened here yet. What
>> happens on your box?
>
> The same freezes still occur.

I think the freezes I saw weren't related to the same issue. I was
running a debugging kernel, and I saw the same freezes also with a
different wifi adaptor. After switching to a non-debugging kernel, and
using your patch, the freezes stopped.

Thanks,
Haggai

2015-05-23 18:09:32

by Haggai Eran

[permalink] [raw]
Subject: Re: [PATCH] staging: rtl8712: prevent buffer overrun in recvbuf2recvframe

On 23 May 2015 at 20:48, Larry Finger <[email protected]> wrote:
> That is good news. Perhaps the debugging kernel is overflowing the stack.
> Did your debugging kernel have "Check for stack overflows" set in the
> "Memory Debugging" section of the configuration? I did not have that turned
> on until today, but it seems like a good idea.
No, I don't think they have it implemented for ARM.

> Do you want to prepare the final version of the patch, or should I?

I can send an updated patch.

Thanks,
Haggai