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

This is a discussion on [PATCH 1/1] USBHID: correct start/stop cycle - Kernel ; `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 ...

+ Reply to Thread
Results 1 to 12 of 12

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

  1. [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
    ---
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    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 .

    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    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.
    >
    > Also don't forget to clear disconnect flag on start which was left set
    > by the previous `stop'.
    >
    > Signed-off-by: Jiri Slaby
    > ---
    > 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);


    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    Jiri Slaby wrote:
    > 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 .


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

    Helge

    > 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    Helge Deller napsal(a):
    > Jiri Slaby wrote:
    >> 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 .

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


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

  6. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    Jiri Slaby wrote:
    > Helge Deller napsal(a):
    >> Jiri Slaby wrote:
    >>> 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 .

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

    >
    > Could you bisect it?


    I fully bisected it, and the final "buggy" patch seems to have been
    Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f
    (see
    http://github.com/jonsmirl/digispeak...438a809195008f)
    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:
    http://bugzilla.kernel.org/show_bug.cgi?id=11913
    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    On Sunday 02 November 2008 17:50, Helge Deller wrote:
    > Jiri Slaby wrote:
    > >>> 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.

    > >
    > > Could you bisect it?

    >
    > I fully bisected it, and the final "buggy" patch seems to have been
    > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f
    > (see
    > http://github.com/jonsmirl/digispeak...438a809195008f)
    > 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.
    --
    vda
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    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.
    >
    > Also don't forget to clear disconnect flag on start which was left set
    > by the previous `stop'.


    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    On Sun, 2 Nov 2008, Denys Vlasenko wrote:

    > > I fully bisected it, and the final "buggy" patch seems to have been
    > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
    > > http://github.com/jonsmirl/digispeak...438a809195008f)
    > > 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?

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

  10. Re: [PATCH 1/1] USBHID: correct start/stop cycle

    On Wednesday 12 November 2008 00:22, Jiri Kosina wrote:
    > On Sun, 2 Nov 2008, Denys Vlasenko wrote:
    > > > I fully bisected it, and the final "buggy" patch seems to have been
    > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
    > > > http://github.com/jonsmirl/digispeak...438a809195008f)
    > > > 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?


    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-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)


  11. Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

    On Wed, 12 Nov 2008, Denys Vlasenko wrote:

    > > > > I fully bisected it, and the final "buggy" patch seems to have been
    > > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
    > > > > http://github.com/jonsmirl/digispeak...438a809195008f)
    > > > > 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?

    > 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,


    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.

    > let's fix it for good.
    > Signed-off-by: Denys Vlasenko
    > --- 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)


    Thanks,

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

  12. Re: Who broke cb8f488c33 patch? (was Re: [PATCH 1/1] USBHID: correct start/stop cycle)

    On Wed, 12 Nov 2008 01:34:54 +0100 (CET)
    Jiri Kosina wrote:

    > On Wed, 12 Nov 2008, Denys Vlasenko wrote:
    >
    > > > > > I fully bisected it, and the final "buggy" patch seems to have been
    > > > > > Denys Vlasenko's patch: cb8f488c33539f096580e202f5438a809195008f (see
    > > > > > http://github.com/jonsmirl/digispeak...438a809195008f)
    > > > > > 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?

    > > 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,

    >
    > 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.
    >
    > > let's fix it for good.
    > > Signed-off-by: Denys Vlasenko
    > > --- 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)

    >


    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread