[PATCH 1/1] USBHID: correct start/stop cycle

`stop' left out usbhid->urb* pointers and so the next `start' thought
it needs to allocate nothing and used the memory pointers previously
pointed to. This led to memory corruption and device malfunction.

Also don't forget to clear disconnect flag on start which was left set
by the previous `stop'.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 18e5ddd..f0339ae 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -781,6 +781,8 @@ static int usbhid_start(struct hid_device *hid)
unsigned int n, insize = 0;
int ret;

+ clear_bit(HID_DISCONNECTED, &usbhid->iofl);
+
usbhid->bufsize = HID_MIN_BUFFER_SIZE;
hid_find_max_report(hid, HID_INPUT_REPORT, &usbhid->bufsize);
hid_find_max_report(hid, HID_OUTPUT_REPORT, &usbhid->bufsize);
@@ -888,6 +890,9 @@ fail:
usb_free_urb(usbhid->urbin);
usb_free_urb(usbhid->urbout);
usb_free_urb(usbhid->urbctrl);
+ usbhid->urbin = NULL;
+ usbhid->urbout = NULL;
+ usbhid->urbctrl = NULL;
hid_free_buffers(dev, hid);
mutex_unlock(&usbhid->setup);
return ret;
@@ -924,6 +929,9 @@ static void usbhid_stop(struct hid_device *hid)
usb_free_urb(usbhid->urbin);
usb_free_urb(usbhid->urbctrl);
usb_free_urb(usbhid->urbout);
+ usbhid->urbin = NULL; /* don't mess up next start */
+ usbhid->urbctrl = NULL;
+ usbhid->urbout = NULL;

hid_free_buffers(hid_to_usb_dev(hid), hid);
mutex_unlock(&usbhid->setup);
--
1.6.0.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On 11/02/2008 12:02 AM, Jiri Kosina wrote:[color=blue]
> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>[color=green]
>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>> it needs to allocate nothing and used the memory pointers previously
>> pointed to. This led to memory corruption and device malfunction.[/color][/color]
[...][color=blue]
> could you please verify whether this patch fixes the corruption you were
> experiencing?[/color]

btw. this is not expected to fix that, but if it does, the better ;).

This fixes
echo DEVICE > /sys/bus/hid/drivers/DRIVER/unbind
echo DEVICE > /sys/bus/hid/drivers/DRIVER/bind
failures.

But maybe parisc does something differently than x86 in bus handling so that it
triggers...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Sat, 1 Nov 2008, Jiri Slaby wrote:
[color=blue]
> `stop' left out usbhid->urb* pointers and so the next `start' thought
> it needs to allocate nothing and used the memory pointers previously
> pointed to. This led to memory corruption and device malfunction.
>
> Also don't forget to clear disconnect flag on start which was left set
> by the previous `stop'.
>
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> ---
> drivers/hid/usbhid/hid-core.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 18e5ddd..f0339ae 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -781,6 +781,8 @@ static int usbhid_start(struct hid_device *hid)
> unsigned int n, insize = 0;
> int ret;
>
> + clear_bit(HID_DISCONNECTED, &usbhid->iofl);
> +
> usbhid->bufsize = HID_MIN_BUFFER_SIZE;
> hid_find_max_report(hid, HID_INPUT_REPORT, &usbhid->bufsize);
> hid_find_max_report(hid, HID_OUTPUT_REPORT, &usbhid->bufsize);
> @@ -888,6 +890,9 @@ fail:
> usb_free_urb(usbhid->urbin);
> usb_free_urb(usbhid->urbout);
> usb_free_urb(usbhid->urbctrl);
> + usbhid->urbin = NULL;
> + usbhid->urbout = NULL;
> + usbhid->urbctrl = NULL;
> hid_free_buffers(dev, hid);
> mutex_unlock(&usbhid->setup);
> return ret;
> @@ -924,6 +929,9 @@ static void usbhid_stop(struct hid_device *hid)
> usb_free_urb(usbhid->urbin);
> usb_free_urb(usbhid->urbctrl);
> usb_free_urb(usbhid->urbout);
> + usbhid->urbin = NULL; /* don't mess up next start */
> + usbhid->urbctrl = NULL;
> + usbhid->urbout = NULL;
>
> hid_free_buffers(hid_to_usb_dev(hid), hid);
> mutex_unlock(&usbhid->setup);[/color]

Jeroen, Helge,

could you please verify whether this patch fixes the corruption you were
experiencing?

[ I will be offline for the upcoming ~9 days, will push the fix upstream
then, if it is not picked up through different channels in the
meantime ]

Thanks!

--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Jiri Slaby wrote:[color=blue]
> On 11/02/2008 12:02 AM, Jiri Kosina wrote:[color=green]
>> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>>[color=darkred]
>>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>>> it needs to allocate nothing and used the memory pointers previously
>>> pointed to. This led to memory corruption and device malfunction.[/color][/color]
> [...][color=green]
>> could you please verify whether this patch fixes the corruption you were
>> experiencing?[/color]
>
> btw. this is not expected to fix that, but if it does, the better ;).[/color]

I tried the patch and sadly it didn't fixed the parisc bug.

Helge
[color=blue]
> This fixes
> echo DEVICE > /sys/bus/hid/drivers/DRIVER/unbind
> echo DEVICE > /sys/bus/hid/drivers/DRIVER/bind
> failures.
>
> But maybe parisc does something differently than x86 in bus handling so that it
> triggers...[/color]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Helge Deller napsal(a):[color=blue]
> Jiri Slaby wrote:[color=green]
>> On 11/02/2008 12:02 AM, Jiri Kosina wrote:[color=darkred]
>>> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>>>
>>>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>>>> it needs to allocate nothing and used the memory pointers previously
>>>> pointed to. This led to memory corruption and device malfunction.[/color]
>> [...][color=darkred]
>>> could you please verify whether this patch fixes the corruption you
>>> were experiencing?[/color]
>>
>> btw. this is not expected to fix that, but if it does, the better ;).[/color]
>
> I tried the patch and sadly it didn't fixed the parisc bug.[/color]

Could you bisect it?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

Jiri Slaby wrote:[color=blue]
> Helge Deller napsal(a):[color=green]
>> Jiri Slaby wrote:[color=darkred]
>>> On 11/02/2008 12:02 AM, Jiri Kosina wrote:
>>>> On Sat, 1 Nov 2008, Jiri Slaby wrote:
>>>>
>>>>> `stop' left out usbhid->urb* pointers and so the next `start' thought
>>>>> it needs to allocate nothing and used the memory pointers previously
>>>>> pointed to. This led to memory corruption and device malfunction.
>>> [...]
>>>> could you please verify whether this patch fixes the corruption you
>>>> were experiencing?
>>> btw. this is not expected to fix that, but if it does, the better ;).[/color]
>> I tried the patch and sadly it didn't fixed the parisc bug.[/color]
>
> Could you bisect it?[/color]

I fully bisected it, and the final "buggy" patch seems to have been
Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f
(see
[url]http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f[/url])
Denys: Any reason you removed "!prev" in front of "expand_stack" ? It
seems wrong to remove the prev-check, else it would crash in
expand_upwards(in the CONFIG_STACK_GROWSUP case).
This is parisc architecture (stack grows up, big-endian).

Sadly reverting just this patch, didn't fixed the bugzilla bug either:
[url]http://bugzilla.kernel.org/show_bug.cgi?id=11913[/url]
I think I need to redo all of my bisecting again... sigh...

Helge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Sunday 02 November 2008 17:50, Helge Deller wrote:[color=blue]
> Jiri Slaby wrote:[color=green][color=darkred]
> >>> btw. this is not expected to fix that, but if it does, the better ;).
> >> I tried the patch and sadly it didn't fixed the parisc bug.[/color]
> >
> > Could you bisect it?[/color]
>
> I fully bisected it, and the final "buggy" patch seems to have been
> Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f
> (see
> [url]http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f[/url])
> Denys: Any reason you removed "!prev" in front of "expand_stack"?[/color]

Looks like I erroneously thought it can't be NULL,
or that expand_upwards() is ok with getting NULL vma parameter.

I looked again and neither is true.

Sorry, looks like I indeed broke this.
--
vda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Sat, 1 Nov 2008, Jiri Slaby wrote:
[color=blue]
> `stop' left out usbhid->urb* pointers and so the next `start' thought
> it needs to allocate nothing and used the memory pointers previously
> pointed to. This led to memory corruption and device malfunction.
>
> Also don't forget to clear disconnect flag on start which was left set
> by the previous `stop'.[/color]

Applied, thanks Jiri.

--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Sun, 2 Nov 2008, Denys Vlasenko wrote:
[color=blue][color=green]
> > I fully bisected it, and the final "buggy" patch seems to have been
> > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > [url]http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f[/url])
> > Denys: Any reason you removed "!prev" in front of "expand_stack"?[/color]
> Looks like I erroneously thought it can't be NULL,
> or that expand_upwards() is ok with getting NULL vma parameter.
> I looked again and neither is true.
> Sorry, looks like I indeed broke this.[/color]

Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?

--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Wednesday 12 November 2008 00:22, Jiri Kosina wrote:[color=blue]
> On Sun, 2 Nov 2008, Denys Vlasenko wrote:[color=green][color=darkred]
> > > I fully bisected it, and the final "buggy" patch seems to have been
> > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > > [url]http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f[/url])
> > > Denys: Any reason you removed "!prev" in front of "expand_stack"?[/color][/color]
>[color=green]
> > Looks like I erroneously thought it can't be NULL,
> > or that expand_upwards() is ok with getting NULL vma parameter.
> > I looked again and neither is true.
> > Sorry, looks like I indeed broke this.[/color]
>
> Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?[/color]

I found my original email in "sent" folder. The patch in that mail
does NOT remove !prev. That change had beed added by someone else.
See attached file with original email.

Ok, I think we are not much interested in who did it, let's
fix it for good.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
--
vda


--- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
+++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
@@ -1704,7 +1704,7 @@
vma = find_vma_prev(mm, addr, &prev);
if (vma && (vma->vm_start <= addr))
return vma;
- if (expand_stack(prev, addr))
+ if (!prev || expand_stack(prev, addr))
return NULL;
if (prev->vm_flags & VM_LOCKED) {
if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)

On Wed, 12 Nov 2008, Denys Vlasenko wrote:
[color=blue][color=green][color=darkred]
> > > > I fully bisected it, and the final "buggy" patch seems to have been
> > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > > > [url]http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f[/url])
> > > > Denys: Any reason you removed "!prev" in front of "expand_stack"?
> > > Looks like I erroneously thought it can't be NULL,
> > > or that expand_upwards() is ok with getting NULL vma parameter.
> > > I looked again and neither is true.
> > > Sorry, looks like I indeed broke this.[/color]
> > Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?[/color]
> I found my original email in "sent" folder. The patch in that mail does
> NOT remove !prev. That change had beed added by someone else. See
> attached file with original email.
> Ok, I think we are not much interested in who did it,[/color]

Hmm, I in fact think we would like to know who removed the check and
folded it into your original patch. I have added all the Signoffs and CCs
to the recepient list.

Andrew usually puts

[someone@somewhere.com: added this and that to the patch]

marker into the changelog, but it's not the case here.

Having your name in Signed-off-by field of something you are not aware of
should make you feel at least a little bit nervous IMHO.
[color=blue]
> let's fix it for good.
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> --- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
> +++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
> @@ -1704,7 +1704,7 @@
> vma = find_vma_prev(mm, addr, &prev);
> if (vma && (vma->vm_start <= addr))
> return vma;
> - if (expand_stack(prev, addr))
> + if (!prev || expand_stack(prev, addr))
> return NULL;
> if (prev->vm_flags & VM_LOCKED) {
> if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)[/color]

Thanks,

--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]

On Wed, 12 Nov 2008 01:34:54 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:
[color=blue]
> On Wed, 12 Nov 2008, Denys Vlasenko wrote:
>[color=green][color=darkred]
> > > > > I fully bisected it, and the final "buggy" patch seems to have been
> > > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
> > > > > [url]http://github.com/jonsmirl/digispeaker/commit/cb8f488c33539f096580e202f5438a809195008f[/url])
> > > > > Denys: Any reason you removed "!prev" in front of "expand_stack"?
> > > > Looks like I erroneously thought it can't be NULL,
> > > > or that expand_upwards() is ok with getting NULL vma parameter.
> > > > I looked again and neither is true.
> > > > Sorry, looks like I indeed broke this.
> > > Hmm, so ... ? Seems like this didn't get fixed in Linus' tree yet?[/color]
> > I found my original email in "sent" folder. The patch in that mail does
> > NOT remove !prev. That change had beed added by someone else. See
> > attached file with original email.
> > Ok, I think we are not much interested in who did it,[/color]
>
> Hmm, I in fact think we would like to know who removed the check and
> folded it into your original patch. I have added all the Signoffs and CCs
> to the recepient list.
>
> Andrew usually puts
>
> [someone@somewhere.com: added this and that to the patch]
>
> marker into the changelog, but it's not the case here.
>
> Having your name in Signed-off-by field of something you are not aware of
> should make you feel at least a little bit nervous IMHO.
>[color=green]
> > let's fix it for good.
> > Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> > --- linux-2.6.28-rc4/mm/mmap.c Mon Nov 10 01:36:15 2008
> > +++ linux-2.6.28-rc4.fix/mm/mmap.c Wed Nov 12 01:21:39 2008
> > @@ -1704,7 +1704,7 @@
> > vma = find_vma_prev(mm, addr, &prev);
> > if (vma && (vma->vm_start <= addr))
> > return vma;
> > - if (expand_stack(prev, addr))
> > + if (!prev || expand_stack(prev, addr))
> > return NULL;
> > if (prev->vm_flags & VM_LOCKED) {
> > if (mlock_vma_pages_range(prev, addr, prev->vm_end) < 0)[/color]
>[/color]

It looks like this was caused by me fixing rejects. That was the fancy
include-lots-of-context-so-it-wont-apply patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email]majordomo@vger.kernel.org[/email]
More majordomo info at [url]http://vger.kernel.org/majordomo-info.html[/url]
Please read the FAQ at [url]http://www.tux.org/lkml/[/url]