Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Dan Carpenter]

On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> Reverting the patch changes the driver back to useing kzalloc() and
> memcpy() instead of  kmemdup.  Doing so has uncovered another bug, which
> causes an oops on memcpy()[1].  We are in the process of bisecting that
> one now and will provide the results.

The two bugs are the same it's that the code has shifted a little.  Mark
the commit as buggy and continue with the git bisect.

regards,
dan carpenter

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Joseph Salisbury]

On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>> Reverting the patch changes the driver back to useing kzalloc() and
>> memcpy() instead of  kmemdup.  Doing so has uncovered another bug, which
>> causes an oops on memcpy()[1].  We are in the process of bisecting that
>> one now and will provide the results.
> The two bugs are the same it's that the code has shifted a little.  Mark
> the commit as buggy and continue with the git bisect.
>
> regards,
> dan carpenter
Can you explain a little further?  Mark commit a4a23f6 as bad?  An
initial bisect already reported that was the first bad commit, so it
can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
is reverted.  The oops on memcpy() did not happen before a4a23f6 was
committed, so I assume this new oops was introduced by a change later.

Right now I'm bisecting down the oops on memcpy() by updating the bisect
with good or bad, depending if the test kernel hit the oops.  I then
revert a4a23f6, so that revert is the HEAD of the tree each time before
building the kernel again(As long as the commit spit out by bisect is
after when a4a23f6 was introduced).

Thanks,

Joe

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Dan Carpenter]

On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
> > On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
> >> Reverting the patch changes the driver back to useing kzalloc() and
> >> memcpy() instead of  kmemdup.  Doing so has uncovered another bug, which
> >> causes an oops on memcpy()[1].  We are in the process of bisecting that
> >> one now and will provide the results.
> > The two bugs are the same it's that the code has shifted a little.  Mark
> > the commit as buggy and continue with the git bisect.
> >
> > regards,
> > dan carpenter
> Can you explain a little further?  Mark commit a4a23f6 as bad?  An
> initial bisect already reported that was the first bad commit, so it
> can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
> is reverted.  The oops on memcpy() did not happen before a4a23f6 was
> committed, so I assume this new oops was introduced by a change later.
> 
> Right now I'm bisecting down the oops on memcpy() by updating the bisect
> with good or bad, depending if the test kernel hit the oops.  I then
> revert a4a23f6, so that revert is the HEAD of the tree each time before
> building the kernel again(As long as the commit spit out by bisect is
> after when a4a23f6 was introduced).

Yep.  Please continue bisecting the memcpy() oops.

kmemdup() is just a kzalloc() followed by a memcpy().  When we split it
apart by reverting the patch then we would expect the oops to move to
the memcpy() part.  Somehow "desc" is a bogus pointer, but I don't
immediately see how that is possible.

regards,
dan carpenter

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Joseph Salisbury]

On 09/16/2013 05:05 PM, Dan Carpenter wrote:
> On Mon, Sep 16, 2013 at 04:49:29PM -0400, Joseph Salisbury wrote:
>> On 09/16/2013 04:38 PM, Dan Carpenter wrote:
>>> On Mon, Sep 16, 2013 at 01:42:35PM -0400, Joseph Salisbury wrote:
>>>> Reverting the patch changes the driver back to useing kzalloc() and
>>>> memcpy() instead of  kmemdup.  Doing so has uncovered another bug, which
>>>> causes an oops on memcpy()[1].  We are in the process of bisecting that
>>>> one now and will provide the results.
>>> The two bugs are the same it's that the code has shifted a little.  Mark
>>> the commit as buggy and continue with the git bisect.
>>>
>>> regards,
>>> dan carpenter
>> Can you explain a little further?  Mark commit a4a23f6 as bad?  An
>> initial bisect already reported that was the first bad commit, so it
>> can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
>> is reverted.  The oops on memcpy() did not happen before a4a23f6 was
>> committed, so I assume this new oops was introduced by a change later.
>>
>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>> with good or bad, depending if the test kernel hit the oops.  I then
>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>> building the kernel again(As long as the commit spit out by bisect is
>> after when a4a23f6 was introduced).
> Yep.  Please continue bisecting the memcpy() oops.
>
> kmemdup() is just a kzalloc() followed by a memcpy().  When we split it
> apart by reverting the patch then we would expect the oops to move to
> the memcpy() part.  Somehow "desc" is a bogus pointer, but I don't
> immediately see how that is possible.
>
> regards,
> dan carpenter

Thanks for the details.  We'll continue the bisect and let you know how
it goes.

Thanks again,

Joe

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Jiri Kosina]

On Mon, 16 Sep 2013, Joseph Salisbury wrote:

> >> Can you explain a little further?  Mark commit a4a23f6 as bad?  An
> >> initial bisect already reported that was the first bad commit, so it
> >> can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
> >> is reverted.  The oops on memcpy() did not happen before a4a23f6 was
> >> committed, so I assume this new oops was introduced by a change later.
> >>
> >> Right now I'm bisecting down the oops on memcpy() by updating the bisect
> >> with good or bad, depending if the test kernel hit the oops.  I then
> >> revert a4a23f6, so that revert is the HEAD of the tree each time before
> >> building the kernel again(As long as the commit spit out by bisect is
> >> after when a4a23f6 was introduced).
> > Yep.  Please continue bisecting the memcpy() oops.
> >
> > kmemdup() is just a kzalloc() followed by a memcpy().  When we split it
> > apart by reverting the patch then we would expect the oops to move to
> > the memcpy() part.  Somehow "desc" is a bogus pointer, but I don't
> > immediately see how that is possible.
> >
> > regards,
> > dan carpenter
> 
> Thanks for the details.  We'll continue the bisect and let you know how
> it goes.

Did this please yield any useful result?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Joseph Salisbury]

On 09/24/2013 05:29 AM, Jiri Kosina wrote:
> On Mon, 16 Sep 2013, Joseph Salisbury wrote:
>
>>>> Can you explain a little further?  Mark commit a4a23f6 as bad?  An
>>>> initial bisect already reported that was the first bad commit, so it
>>>> can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
>>>> is reverted.  The oops on memcpy() did not happen before a4a23f6 was
>>>> committed, so I assume this new oops was introduced by a change later.
>>>>
>>>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>>>> with good or bad, depending if the test kernel hit the oops.  I then
>>>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>>>> building the kernel again(As long as the commit spit out by bisect is
>>>> after when a4a23f6 was introduced).
>>> Yep.  Please continue bisecting the memcpy() oops.
>>>
>>> kmemdup() is just a kzalloc() followed by a memcpy().  When we split it
>>> apart by reverting the patch then we would expect the oops to move to
>>> the memcpy() part.  Somehow "desc" is a bogus pointer, but I don't
>>> immediately see how that is possible.
>>>
>>> regards,
>>> dan carpenter
>> Thanks for the details.  We'll continue the bisect and let you know how
>> it goes.
> Did this please yield any useful result?
>
> Thanks,
>
Thanks for following up, Jiri.  It's been a little tricky narrowing this
one down.  We bisected a couple of times, and both bisects indicated the
following commits as the first bad:

commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <jkosina@suse.cz>
Date: Mon Jun 3 11:27:48 2013 +0200

    HID: core: fix reporting of raw events

However, reverting this commit does not stop the system from locking up,
when the wireless trackpad is connected.  I was thinking of maybe using
a brute force method and pulling out all the HID changes, added in
3.11-rc1 to ensure the bug goes away.  Then add a group back in at a
time to narrow down the commit that introduced this.  The bisect should
have done this, but I'm not sure why it didn't.  It would be greatly
appreciated if you had any other suggestions on tracking down the cause?

Thanks,

Joe

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Joseph Salisbury]

On 09/24/2013 05:29 AM, Jiri Kosina wrote:
> On Mon, 16 Sep 2013, Joseph Salisbury wrote:
>
>>>> Can you explain a little further?  Mark commit a4a23f6 as bad?  An
>>>> initial bisect already reported that was the first bad commit, so it
>>>> can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
>>>> is reverted.  The oops on memcpy() did not happen before a4a23f6 was
>>>> committed, so I assume this new oops was introduced by a change later.
>>>>
>>>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>>>> with good or bad, depending if the test kernel hit the oops.  I then
>>>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>>>> building the kernel again(As long as the commit spit out by bisect is
>>>> after when a4a23f6 was introduced).
>>> Yep.  Please continue bisecting the memcpy() oops.
>>>
>>> kmemdup() is just a kzalloc() followed by a memcpy().  When we split it
>>> apart by reverting the patch then we would expect the oops to move to
>>> the memcpy() part.  Somehow "desc" is a bogus pointer, but I don't
>>> immediately see how that is possible.
>>>
>>> regards,
>>> dan carpenter
>> Thanks for the details.  We'll continue the bisect and let you know how
>> it goes.
> Did this please yield any useful result?
>
> Thanks,
>

We also tested the 3.12-rc2 kernel, but it also produces an oops and
lockup.  In case it's of use, the 3.12-rc2 oops can be seen at:
https://launchpadlibrarian.net/151359441/kern.log

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Joseph Salisbury]

On 09/24/2013 05:29 AM, Jiri Kosina wrote:
> On Mon, 16 Sep 2013, Joseph Salisbury wrote:
>
>>>> Can you explain a little further?  Mark commit a4a23f6 as bad?  An
>>>> initial bisect already reported that was the first bad commit, so it
>>>> can't be marked bad.  The oops on memcpy() happens after commit a4a23f6
>>>> is reverted.  The oops on memcpy() did not happen before a4a23f6 was
>>>> committed, so I assume this new oops was introduced by a change later.
>>>>
>>>> Right now I'm bisecting down the oops on memcpy() by updating the bisect
>>>> with good or bad, depending if the test kernel hit the oops.  I then
>>>> revert a4a23f6, so that revert is the HEAD of the tree each time before
>>>> building the kernel again(As long as the commit spit out by bisect is
>>>> after when a4a23f6 was introduced).
>>> Yep.  Please continue bisecting the memcpy() oops.
>>>
>>> kmemdup() is just a kzalloc() followed by a memcpy().  When we split it
>>> apart by reverting the patch then we would expect the oops to move to
>>> the memcpy() part.  Somehow "desc" is a bogus pointer, but I don't
>>> immediately see how that is possible.
>>>
>>> regards,
>>> dan carpenter
>> Thanks for the details.  We'll continue the bisect and let you know how
>> it goes.
> Did this please yield any useful result?
>
> Thanks,
>

After further testing reverting the following commit does in fact
resolve the bug:

commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <jkosina@suse.cz>
Date: Mon Jun 3 11:27:48 2013 +0200

    HID: core: fix reporting of raw events

Reverting this commit in v3.12-rc2 prevents the system from locking up, which happens when connecting a bluetooth trackpad.

Jiri,  do you think we should revert this patch, or is there some further debugging/data collecting you would like to do?

Thanks,

Joe

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Jiri Kosina]

On Wed, 25 Sep 2013, Joseph Salisbury wrote:

> After further testing reverting the following commit does in fact
> resolve the bug:
> 
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina <jkosina@suse.cz>
> Date: Mon Jun 3 11:27:48 2013 +0200
> 
>     HID: core: fix reporting of raw events
> 
> Reverting this commit in v3.12-rc2 prevents the system from locking up, 
> which happens when connecting a bluetooth trackpad.
> 
> Jiri, do you think we should revert this patch, or is there some further 
> debugging/data collecting you would like to do?

Hi Joseph,

in this mail:

	Message-ID: <5241992E.3090805@canonical.com>
	Date: Tue, 24 Sep 2013 09:52:46 -0400

you said that reverting this commit doesn't prevent the lockups, so I am 
rather confused ... ?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Joseph Salisbury]

On 09/27/2013 06:50 AM, Jiri Kosina wrote:
> On Wed, 25 Sep 2013, Joseph Salisbury wrote:
>
>> After further testing reverting the following commit does in fact
>> resolve the bug:
>>
>> commit b1a1442a23776756b254b69786848a94d92445ba
>> Author: Jiri Kosina <jkosina@suse.cz>
>> Date: Mon Jun 3 11:27:48 2013 +0200
>>
>>     HID: core: fix reporting of raw events
>>
>> Reverting this commit in v3.12-rc2 prevents the system from locking up, 
>> which happens when connecting a bluetooth trackpad.
>>
>> Jiri, do you think we should revert this patch, or is there some further 
>> debugging/data collecting you would like to do?
> Hi Joseph,
>
> in this mail:
>
> 	Message-ID: <5241992E.3090805@canonical.com>
> 	Date: Tue, 24 Sep 2013 09:52:46 -0400
>
> you said that reverting this commit doesn't prevent the lockups, so I am 
> rather confused ... ?
>
> Thanks,
>
The testing was invalid.  Reverting commit b1a1442 does resolve the bug
and stop the lockups.

Thanks,

Joe

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Dan Carpenter]

On Fri, Sep 27, 2013 at 10:42:10AM -0400, Joseph Salisbury wrote:
> On 09/27/2013 06:50 AM, Jiri Kosina wrote:
> > On Wed, 25 Sep 2013, Joseph Salisbury wrote:
> >
> >> After further testing reverting the following commit does in fact
> >> resolve the bug:
> >>
> >> commit b1a1442a23776756b254b69786848a94d92445ba
> >> Author: Jiri Kosina <jkosina@suse.cz>
> >> Date: Mon Jun 3 11:27:48 2013 +0200
> >>
> >>     HID: core: fix reporting of raw events
> >>
> >> Reverting this commit in v3.12-rc2 prevents the system from locking up, 
> >> which happens when connecting a bluetooth trackpad.
> >>
> >> Jiri, do you think we should revert this patch, or is there some further 
> >> debugging/data collecting you would like to do?
> > Hi Joseph,
> >
> > in this mail:
> >
> > 	Message-ID: <5241992E.3090805@canonical.com>
> > 	Date: Tue, 24 Sep 2013 09:52:46 -0400
> >
> > you said that reverting this commit doesn't prevent the lockups, so I am 
> > rather confused ... ?
> >
> > Thanks,
> >
> The testing was invalid.  Reverting commit b1a1442 does resolve the bug
> and stop the lockups.
> 

It looks like magicmouse_raw_event() returns 1 on success and 0 on
failure.

regards,
dan carpenter

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Dan Carpenter]

On Fri, Sep 27, 2013 at 06:24:12PM +0300, Dan Carpenter wrote:
> 
> It looks like magicmouse_raw_event() returns 1 on success and 0 on
> failure.

Fixing the return codes is a good idea but it won't fix the oops.

What's the point of returning 1 and 0?  In the current code no one
cares and both are treated the same.

Also if we decide to fix this instead of reverting the we could do this
cleanup as well:

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b8470b1..868ebaa 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1417,10 +1417,8 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
 
 	if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
 		ret = hdrv->raw_event(hid, report, data, size);
-		if (ret < 0) {
-			ret = ret < 0 ? ret : 0;
+		if (ret < 0)
 			goto unlock;
-		}
 	}
 
 	ret = hid_report_raw_event(hid, type, data, size, interrupt);

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Jiri Kosina]

On Fri, 27 Sep 2013, Dan Carpenter wrote:

> It looks like magicmouse_raw_event() returns 1 on success and 0 on
> failure.

Good catch indeed.

I am not completely sure whether we are going to fix an oops or not by 
this, as I haven't seen the actual oops anywhere in this thread :) But 
definitely this looks like a good fix.

Joseph, could you please test with that? Thanks.



diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 3b43d1c..c211eb9 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -334,7 +334,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 			size - 2 - data[1]);
 		break;
 	default:
-		return 0;
+		return 1;
 	}
 
 	if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
@@ -347,7 +347,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
 	}
 
 	input_sync(input);
-	return 1;
+	return 0;
 }
 
 static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hdev)

-- 
Jiri Kosina
SUSE Labs

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Dan Carpenter]

On Mon, Sep 30, 2013 at 04:35:47PM +0200, Jiri Kosina wrote:
> On Fri, 27 Sep 2013, Dan Carpenter wrote:
> 
> > It looks like magicmouse_raw_event() returns 1 on success and 0 on
> > failure.
> 
> Good catch indeed.
> 
> I am not completely sure whether we are going to fix an oops or not by 
> this, as I haven't seen the actual oops anywhere in this thread :) But 
> definitely this looks like a good fix.
> 
> Joseph, could you please test with that? Thanks.

In the new code both 0 and 1 are treated the same so this can't fix the
bug.

regards,
dan carpenter

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Jiri Kosina]

On Fri, 27 Sep 2013, Joseph Salisbury wrote:

> >> commit b1a1442a23776756b254b69786848a94d92445ba
> >> Author: Jiri Kosina <jkosina@suse.cz>
> >> Date: Mon Jun 3 11:27:48 2013 +0200
> >>
> >>     HID: core: fix reporting of raw events
> >>
> >> Reverting this commit in v3.12-rc2 prevents the system from locking up, 
> >> which happens when connecting a bluetooth trackpad.
> >>
> >> Jiri, do you think we should revert this patch, or is there some further 
> >> debugging/data collecting you would like to do?
> > Hi Joseph,
> >
> > in this mail:
> >
> > 	Message-ID: <5241992E.3090805@canonical.com>
> > 	Date: Tue, 24 Sep 2013 09:52:46 -0400
> >
> > you said that reverting this commit doesn't prevent the lockups, so I am 
> > rather confused ... ?
> >
> > Thanks,
> >
> The testing was invalid.  Reverting commit b1a1442 does resolve the bug
> and stop the lockups.

Okay, I finally got some sense of this, sorry for the delay.

Could you please test with the patch below, instead of reverting 
b1a1442a23? Thanks a lot.



diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 253fe23..81eacd3 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
 		csize--;
 	}
 
-	rsize = ((report->size - 1) >> 3) + 1;
+	rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
 
 	if (rsize > HID_MAX_BUFFER_SIZE)
 		rsize = HID_MAX_BUFFER_SIZE;

-- 
Jiri Kosina
SUSE Labs

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[David Herrmann]

Hi

On Mon, Dec 16, 2013 at 2:01 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Fri, 27 Sep 2013, Joseph Salisbury wrote:
>
>> >> commit b1a1442a23776756b254b69786848a94d92445ba
>> >> Author: Jiri Kosina <jkosina@suse.cz>
>> >> Date: Mon Jun 3 11:27:48 2013 +0200
>> >>
>> >>     HID: core: fix reporting of raw events
>> >>
>> >> Reverting this commit in v3.12-rc2 prevents the system from locking up,
>> >> which happens when connecting a bluetooth trackpad.
>> >>
>> >> Jiri, do you think we should revert this patch, or is there some further
>> >> debugging/data collecting you would like to do?
>> > Hi Joseph,
>> >
>> > in this mail:
>> >
>> >     Message-ID: <5241992E.3090805@canonical.com>
>> >     Date: Tue, 24 Sep 2013 09:52:46 -0400
>> >
>> > you said that reverting this commit doesn't prevent the lockups, so I am
>> > rather confused ... ?
>> >
>> > Thanks,
>> >
>> The testing was invalid.  Reverting commit b1a1442 does resolve the bug
>> and stop the lockups.
>
> Okay, I finally got some sense of this, sorry for the delay.
>
> Could you please test with the patch below, instead of reverting
> b1a1442a23? Thanks a lot.
>
>
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 253fe23..81eacd3 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>                 csize--;
>         }
>
> -       rsize = ((report->size - 1) >> 3) + 1;
> +       rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;

Isn't "report->id" already covered by "if (report_enum->numbered)"
above? The test for "id > 0" won't work here as in this case
"report_enum->numbered" must already be set to true by the hid-desc
parser, doesn't it?

Where exactly did you get the +7 from?

What worries me here is that we write over the @data buffer from the
hid-ll-driver if the report is too short. I don't think the BT driver
accounts for that, mhh.

David

>         if (rsize > HID_MAX_BUFFER_SIZE)
>                 rsize = HID_MAX_BUFFER_SIZE;
>
> --
> Jiri Kosina
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Jiri Kosina]

On Thu, 19 Dec 2013, David Herrmann wrote:

> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 253fe23..81eacd3 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
> >                 csize--;
> >         }
> >
> > -       rsize = ((report->size - 1) >> 3) + 1;
> > +       rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
> 
> Isn't "report->id" already covered by "if (report_enum->numbered)"
> above? The test for "id > 0" won't work here as in this case
> "report_enum->numbered" must already be set to true by the hid-desc
> parser, doesn't it?

Right, that one is not correct here, thanks.

> Where exactly did you get the +7 from?

Please see commit (the one I am not really proud of) 27ce405039bfe6d3.

-- 
Jiri Kosina
SUSE Labs

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 19 Dec 2013, David Herrmann wrote:
>
>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > index 253fe23..81eacd3 100644
>> > --- a/drivers/hid/hid-core.c
>> > +++ b/drivers/hid/hid-core.c
>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>> >                 csize--;
>> >         }
>> >
>> > -       rsize = ((report->size - 1) >> 3) + 1;
>> > +       rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>
>> Isn't "report->id" already covered by "if (report_enum->numbered)"
>> above? The test for "id > 0" won't work here as in this case
>> "report_enum->numbered" must already be set to true by the hid-desc
>> parser, doesn't it?
>
> Right, that one is not correct here, thanks.
>
>> Where exactly did you get the +7 from?
>
> Please see commit (the one I am not really proud of) 27ce405039bfe6d3.

Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit
really breaks BT drivers:

commit b1a1442a23776756b254b69786848a94d92445ba
Author: Jiri Kosina <jkosina@suse.cz>
Date:   Mon Jun 3 11:27:48 2013 +0200

    HID: core: fix reporting of raw events

Problem is, if the raw_event() callback returned 0 earlier, we just
skipped raw input reports. However, we now always call the
hid_report_raw_event() helper. Which is normally fine, but the helper
expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is
*not* true for HIDP. So the memset() writes over some random memory.

I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to
be allocated on the stack, but we're in atomic-context here so a
kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to
look into HIDP to make the skb big enough, but I'm not sure how we can
achieve that.

So off the top of my head, the best idea is to add "char
inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and
copy every input-report into the buffer before passing to
hid_input_report().

Ideas?
David

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[David Herrmann]

Hi

On Thu, Dec 19, 2013 at 11:08 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Thu, Dec 19, 2013 at 10:59 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>> On Thu, 19 Dec 2013, David Herrmann wrote:
>>
>>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> > index 253fe23..81eacd3 100644
>>> > --- a/drivers/hid/hid-core.c
>>> > +++ b/drivers/hid/hid-core.c
>>> > @@ -1334,7 +1334,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>>> >                 csize--;
>>> >         }
>>> >
>>> > -       rsize = ((report->size - 1) >> 3) + 1;
>>> > +       rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>>>
>>> Isn't "report->id" already covered by "if (report_enum->numbered)"
>>> above? The test for "id > 0" won't work here as in this case
>>> "report_enum->numbered" must already be set to true by the hid-desc
>>> parser, doesn't it?
>>
>> Right, that one is not correct here, thanks.
>>
>>> Where exactly did you get the +7 from?
>>
>> Please see commit (the one I am not really proud of) 27ce405039bfe6d3.
>
> Eh, I remember.. Ok, but the magic-mouse is BT right? So this commit
> really breaks BT drivers:
>
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina <jkosina@suse.cz>
> Date:   Mon Jun 3 11:27:48 2013 +0200
>
>     HID: core: fix reporting of raw events
>
> Problem is, if the raw_event() callback returned 0 earlier, we just
> skipped raw input reports. However, we now always call the
> hid_report_raw_event() helper. Which is normally fine, but the helper
> expects the input buffer to be of size HID_MAX_REPORT_SIZE, which is
> *not* true for HIDP. So the memset() writes over some random memory.
>
> I don't know exactly how to fix it. HID_MAX_BUFFER_SIZE is too big to
> be allocated on the stack, but we're in atomic-context here so a
> kzalloc(rsize, GFP_ATOMIC) seems overkill. So I guess we'd have to
> look into HIDP to make the skb big enough, but I'm not sure how we can
> achieve that.
>
> So off the top of my head, the best idea is to add "char
> inbuf[HID_MAX_BUFFER_SIZE];" to the hidp_session object in HIDP and
> copy every input-report into the buffer before passing to
> hid_input_report().

As this thread doesn't really contain any oops message nor the exact
driver name (except mentioning hyperv and magicmouse), I fixed a
related bug for HIDP:
  http://thread.gmane.org/gmane.linux.bluez.kernel/41969

A similar patch is *really* required for hid-hyperv.c as it also does
not provide a properly sized buffer to hid_input_report().

David

Re: [v3.11][Regression] HID: hyperv: convert alloc+memcpy to memdup

[Jiri Kosina]

On Thu, 19 Dec 2013, David Herrmann wrote:

> As this thread doesn't really contain any oops message nor the exact
> driver name (except mentioning hyperv and magicmouse), 

FWIW I recall the oopses being present somewhere in the ubuntu bug 
tracker, referenced in this thread.

Thanks,

-- 
Jiri Kosina
SUSE Labs