2.6.33 bugs (USBFS, Intel graphic)

2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

* http://lkml.org/lkml/2010/2/19/192
.6.32-stable review patch.  If anyone has any objections, please let us know.

------------------
From: Greg KH <greg@kroah.com>

commit d4a4683ca054ed9917dfc9e3ff0f7ecf74ad90d6 upstream.

We need to only copy the data received by the device to userspace, not
the whole kernel buffer, which can contain "stale" data.

Thanks to Marcus Meissner for pointing this out and testing the fix.

Reported-by: Marcus Meissner <meissner@suse.de>
Tested-by: Marcus Meissner <meissner@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


this patch breaks isochronous USBFS support, please revert that patch!

http://sundtek.de/images/tvtime-bildfehler.jpg

with the patch reverted:
http://sundtek.de/images/tvtime-working.png

* Intel graphic flickers now

Markus

On Sat, Feb 27, 2010 at 2:43 AM,  <werner@guyane.dyn-o-saur.com> wrote:
>
>
>
>
>
> =======================
> Feb 26 16:21:53 werner kernel: PERCPU: allocation failed, size=2048 align=8, failed to allocate new chunk
> Feb 26 16:21:53 werner kernel: Pid: 5308, comm: modprobe Tainted: G         C 2.6.33 #1
> Feb 26 16:21:53 werner kernel: Call Trace:
> Feb 26 16:21:53 werner kernel:  [<c1c682ca>] ? printk+0x14/0x16
> Feb 26 16:21:53 werner kernel:  [<c10bdfa3>] pcpu_alloc+0x6ba/0x73b
> Feb 26 16:21:53 werner kernel:  [<c10bb0be>] ? get_slab+0x8/0x50
> Feb 26 16:21:53 werner kernel:  [<c1b7283d>] ? neigh_parms_alloc+0x55/0xd1
> Feb 26 16:21:53 werner kernel:  [<c10be047>] __alloc_percpu+0xf/0x14
> Feb 26 16:21:53 werner kernel:  [<c1bb26cb>] snmp_mib_init+0x22/0x5a
> Feb 26 16:21:53 werner kernel:  [<feb505af>] ipv6_add_dev+0x191/0x30b [ipv6]
> Feb 26 16:21:53 werner kernel:  [<fd268000>] ? inet6_init+0x0/0x2a2 [ipv6]
> Feb 26 16:21:53 werner kernel:  [<fd2682ec>] addrconf_init+0x3b/0x11b [ipv6]
> Feb 26 16:21:53 werner kernel:  [<fd268195>] inet6_init+0x195/0x2a2 [ipv6]
> Feb 26 16:21:53 werner kernel:  [<c1001143>] do_one_initcall+0x51/0x13f
> Feb 26 16:21:53 werner kernel:  [<c1066570>] sys_init_module+0xac/0x1e0
> Feb 26 16:21:53 werner kernel:  [<c1c6acec>] syscall_call+0x7/0xb
> Feb 26 16:22:05 werner kdm_greet[5398]: Can't open default user face
> Feb 26 16:23:22 werner named[5935]: /etc/named.conf:3: option 'multiple-cnames' is obsolete
> Feb 26 16:23:22 werner kernel: PERCPU: allocation failed, size=2048 align=8, failed to allocate new chunk
> Feb 26 16:23:22 werner kernel: Pid: 5940, comm: modprobe Tainted: G         C 2.6.33 #1
> Feb 26 16:23:22 werner kernel: Call Trace:
> Feb 26 16:23:22 werner kernel:  [<c1c682ca>] ? printk+0x14/0x16
> Feb 26 16:23:22 werner kernel:  [<c10bdfa3>] pcpu_alloc+0x6ba/0x73b
> Feb 26 16:23:22 werner kernel:  [<c10bb0be>] ? get_slab+0x8/0x50
> Feb 26 16:23:22 werner kernel:  [<c1b7283d>] ? neigh_parms_alloc+0x55/0xd1
> Feb 26 16:23:22 werner kernel:  [<c10be047>] __alloc_percpu+0xf/0x14
> Feb 26 16:23:22 werner kernel:  [<c1bb26cb>] snmp_mib_init+0x22/0x5a
> Feb 26 16:23:22 werner kernel:  [<feb505af>] ipv6_add_dev+0x191/0x30b [ipv6]
> Feb 26 16:23:22 werner kernel:  [<f9195000>] ? inet6_init+0x0/0x2a2 [ipv6]
> Feb 26 16:23:22 werner kernel:  [<f91952ec>] addrconf_init+0x3b/0x11b [ipv6]
> Feb 26 16:23:22 werner kernel:  [<f9195195>] inet6_init+0x195/0x2a2 [ipv6]
> Feb 26 16:23:22 werner kernel:  [<c1001143>] do_one_initcall+0x51/0x13f
> Feb 26 16:23:22 werner kernel:  [<c1066570>] sys_init_module+0xac/0x1e0
> Feb 26 16:23:22 werner kernel:  [<c1c6acec>] syscall_call+0x7/0xb
>
> ================
> Feb 26 16:23:45 werner kernel: PERCPU: allocation failed, size=2048 align=8, failed to allocate new chunk
> Feb 26 16:23:45 werner kernel: Pid: 6065, comm: modprobe Tainted: G         C 2.6.33 #1
> Feb 26 16:23:45 werner kernel: Call Trace:
> Feb 26 16:23:45 werner kernel:  [<c1c682ca>] ? printk+0x14/0x16
> Feb 26 16:23:45 werner kernel:  [<c10bdfa3>] pcpu_alloc+0x6ba/0x73b
> Feb 26 16:23:45 werner kernel:  [<c10bb0be>] ? get_slab+0x8/0x50
> Feb 26 16:23:45 werner kernel:  [<c1b7283d>] ? neigh_parms_alloc+0x55/0xd1
> Feb 26 16:23:45 werner kernel:  [<c10be047>] __alloc_percpu+0xf/0x14
> Feb 26 16:23:45 werner kernel:  [<c1bb26cb>] snmp_mib_init+0x22/0x5a
> Feb 26 16:23:45 werner kernel:  [<fed1b5af>] ipv6_add_dev+0x191/0x30b [ipv6]
> Feb 26 16:23:45 werner kernel:  [<fd20a000>] ? inet6_init+0x0/0x2a2 [ipv6]
> Feb 26 16:23:45 werner kernel:  [<fd20a2ec>] addrconf_init+0x3b/0x11b [ipv6]
> Feb 26 16:23:45 werner kernel:  [<fd20a195>] inet6_init+0x195/0x2a2 [ipv6]
> Feb 26 16:23:45 werner kernel:  [<c1001143>] do_one_initcall+0x51/0x13f
> Feb 26 16:23:45 werner kernel:  [<c1066570>] sys_init_module+0xac/0x1e0
> Feb 26 16:23:45 werner kernel:  [<c1c6acec>] syscall_call+0x7/0xb
> Feb 26 16:23:45 werner kernel: BUG: unable to handle kernel paging request at feb748d4
> Feb 26 16:23:45 werner kernel: IP: [<c1b68700>] unregister_pernet_operations+0x21/0x93
> Feb 26 16:23:45 werner kernel: *pde = 36683067 *pte = 00000000
> Feb 26 16:23:45 werner kernel: Oops: 0002 [#1] PREEMPT SMP
> Feb 26 16:23:45 werner kernel: last sysfs file: /sys/devices/pci0000:00/0000:00:0d.0/boot_vga
> Feb 26 16:23:45 werner kernel: Modules linked in: ipv6(+) bnep rfcomm hidp l2cap bluetooth snd_usb_audio snd_usb_lib snd_rawmidi snd_seq_device rt2860sta(C) uvcvideo usbvideo lp snd_hda_codec_analog rtc_cmos rtc_core rtc_lib tpm_tis tpm rtl8187 tpm_bios mac80211 snd_hda_intel led_class snd_hda_codec k8temp hwmon snd_hwdep cfg80211 8139too snd_pcm rfkill snd_timer snd forcedeth soundcore snd_page_alloc i2c_nforce2
> Feb 26 16:23:45 werner kernel:
> Feb 26 16:23:45 werner kernel: Pid: 6065, comm: modprobe Tainted: G         C 2.6.33 #1 M2N-VM DH/System Product Name
> Feb 26 16:23:45 werner kernel: EIP: 0060:[<c1b68700>] EFLAGS: 00010246 CPU: 0
> Feb 26 16:23:45 werner kernel: EIP is at unregister_pernet_operations+0x21/0x93
> Feb 26 16:23:45 werner kernel: EAX: feb748d4 EBX: fed3f850 ECX: f29a3f58 EDX: fed3f8d4
> Feb 26 16:23:45 werner kernel: ESI: fed3f850 EDI: fd20a000 EBP: f29a3f6c ESP: f29a3f58
> Feb 26 16:23:45 werner kernel:  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> Feb 26 16:23:45 werner kernel: Process modprobe (pid: 6065, ti=f29a2000 task=f296ef20 task.ti=f29a2000)
> Feb 26 16:23:45 werner kernel: Stack:
> Feb 26 16:23:45 werner kernel:  f29a3f58 f29a3f58 fed3f850 00000000 fd20a000 f29a3f78 c1b687c6 fffffff4
> Feb 26 16:23:45 werner kernel: <0> f29a3f84 fd20a257 fed41834 f29a3f9c c1001143 00000000 fed41834 00000000
> Feb 26 16:23:45 werner kernel: <0> 0805e188 f29a3fac c1066570 b7549008 0805e138 f29a2000 c1c6acec b7549008
> Feb 26 16:23:45 werner kernel: Call Trace:
> Feb 26 16:23:45 werner kernel:  [<fd20a000>] ? inet6_init+0x0/0x2a2 [ipv6]
> Feb 26 16:23:45 werner kernel:  [<c1b687c6>] ? unregister_pernet_subsys+0x1c/0x29
> Feb 26 16:23:45 werner kernel:  [<fd20a257>] ? inet6_init+0x257/0x2a2 [ipv6]
> Feb 26 16:23:45 werner kernel:  [<c1001143>] ? do_one_initcall+0x51/0x13f
> Feb 26 16:23:45 werner kernel:  [<c1066570>] ? sys_init_module+0xac/0x1e0
> Feb 26 16:23:45 werner kernel:  [<c1c6acec>] ? syscall_call+0x7/0xb
> Feb 26 16:23:45 werner kernel: Code: c2 75 e5 e8 78 0a 51 ff eb 97 55 89 e5 57 56 53 83 ec 08 e8 03 b2 49 ff 89 c6 8d 4d ec 89 4d ec 89 4d f0 8b 10 8b 40 04 89 42 04 <89> 10 c7 06 00 01 10 00 c7 46 04 00 02 20 00 a1 b8 b7 10 c2 8d
> Feb 26 16:23:45 werner kernel: EIP: [<c1b68700>] unregister_pernet_operations+0x21/0x93 SS:ESP 0068:f29a3f58
> Feb 26 16:23:45 werner kernel: CR2: 00000000feb748d4
> Feb 26 16:23:45 werner kernel: ---[ end trace dda352e53880a4df ]---
>

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Sat, Feb 27, 2010 at 04:42:53AM +0100, Markus Rechberger wrote:
> * http://lkml.org/lkml/2010/2/19/192
> .6.32-stable review patch.  If anyone has any objections, please let us know.
> 
> ------------------
> From: Greg KH <greg@kroah.com>
> 
> commit d4a4683ca054ed9917dfc9e3ff0f7ecf74ad90d6 upstream.
> 
> We need to only copy the data received by the device to userspace, not
> the whole kernel buffer, which can contain "stale" data.
> 
> Thanks to Marcus Meissner for pointing this out and testing the fix.
> 
> Reported-by: Marcus Meissner <meissner@suse.de>
> Tested-by: Marcus Meissner <meissner@suse.de>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> 
> this patch breaks isochronous USBFS support, please revert that patch!
> 
> http://sundtek.de/images/tvtime-bildfehler.jpg
> 
> with the patch reverted:
> http://sundtek.de/images/tvtime-working.png
> 
> * Intel graphic flickers now

That is very strange.  What userspace program is using usbfs for video
feeds?  And why would it be relying on the invalid data on the end of an
urb?

thanks,

greg k-h

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Sat, Feb 27, 2010 at 4:56 AM, Greg KH <greg@kroah.com> wrote:
> On Sat, Feb 27, 2010 at 04:42:53AM +0100, Markus Rechberger wrote:
>> * http://lkml.org/lkml/2010/2/19/192
>> .6.32-stable review patch.  If anyone has any objections, please let us know.
>>
>> ------------------
>> From: Greg KH <greg@kroah.com>
>>
>> commit d4a4683ca054ed9917dfc9e3ff0f7ecf74ad90d6 upstream.
>>
>> We need to only copy the data received by the device to userspace, not
>> the whole kernel buffer, which can contain "stale" data.
>>
>> Thanks to Marcus Meissner for pointing this out and testing the fix.
>>
>> Reported-by: Marcus Meissner <meissner@suse.de>
>> Tested-by: Marcus Meissner <meissner@suse.de>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>>
>>
>> this patch breaks isochronous USBFS support, please revert that patch!
>>
>> http://sundtek.de/images/tvtime-bildfehler.jpg
>>
>> with the patch reverted:
>> http://sundtek.de/images/tvtime-working.png
>>
>> * Intel graphic flickers now
>
> That is very strange.  What userspace program is using usbfs for video
> feeds?  And why would it be relying on the invalid data on the end of an
> urb?
>

there are 2 userspace videowrapper available for linux and freebsd
which do videoprocessing in userland rather than in kernelspace.
Well isochronous works slightly different than BULK transfer (which I
guess was tested)
urb->iso_frame_desc[i].actual_length
each microframe has it's own length field for ISO.

Markus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Sat, Feb 27, 2010 at 05:05:41AM +0100, Markus Rechberger wrote:
> On Sat, Feb 27, 2010 at 4:56 AM, Greg KH <greg@kroah.com> wrote:
> > On Sat, Feb 27, 2010 at 04:42:53AM +0100, Markus Rechberger wrote:
> >> * http://lkml.org/lkml/2010/2/19/192
> >> .6.32-stable review patch. ?If anyone has any objections, please let us know.
> >>
> >> ------------------
> >> From: Greg KH <greg@kroah.com>
> >>
> >> commit d4a4683ca054ed9917dfc9e3ff0f7ecf74ad90d6 upstream.
> >>
> >> We need to only copy the data received by the device to userspace, not
> >> the whole kernel buffer, which can contain "stale" data.
> >>
> >> Thanks to Marcus Meissner for pointing this out and testing the fix.
> >>
> >> Reported-by: Marcus Meissner <meissner@suse.de>
> >> Tested-by: Marcus Meissner <meissner@suse.de>
> >> Cc: Alan Stern <stern@rowland.harvard.edu>
> >> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> >> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> >>
> >>
> >> this patch breaks isochronous USBFS support, please revert that patch!
> >>
> >> http://sundtek.de/images/tvtime-bildfehler.jpg
> >>
> >> with the patch reverted:
> >> http://sundtek.de/images/tvtime-working.png
> >>
> >> * Intel graphic flickers now
> >
> > That is very strange. ?What userspace program is using usbfs for video
> > feeds? ?And why would it be relying on the invalid data on the end of an
> > urb?
> >
> 
> there are 2 userspace videowrapper available for linux and freebsd
> which do videoprocessing in userland rather than in kernelspace.
> Well isochronous works slightly different than BULK transfer (which I
> guess was tested)
> urb->iso_frame_desc[i].actual_length
> each microframe has it's own length field for ISO.

Yes, and that patch didn't touch the iso frames.  That happens later on
in the functions that were modified.  The patch should not have had any
affect on iso transfers.  Unless I'm missing something?

confused,

greg k-h

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Linus Torvalds]

On Fri, 26 Feb 2010, Greg KH wrote:
> 
> Yes, and that patch didn't touch the iso frames.  That happens later on
> in the functions that were modified.  The patch should not have had any
> affect on iso transfers.  Unless I'm missing something?

Hmm. What seems to happen is that for an isochronous transfer, the buffer 
is split for each microframe. No?

So the total length may be in 'urb->actual_length', but the actual data in 
the buffer may not be contiguous, because it's created from multiple 
smaller frames, some of which might not be full length?

I dunno. That would explain the problem - actual_length is correct, but 
the 'copy_to_user()' still doesn't copy all the data, because it's 
fragmented.

		Linus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Sat, Feb 27, 2010 at 5:29 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Fri, 26 Feb 2010, Greg KH wrote:
>>
>> Yes, and that patch didn't touch the iso frames.  That happens later on
>> in the functions that were modified.  The patch should not have had any
>> affect on iso transfers.  Unless I'm missing something?
>
> Hmm. What seems to happen is that for an isochronous transfer, the buffer
> is split for each microframe. No?
>

exactly. and each microframe has its own buffer length identifier.

the current behaviour breaks VMware, QEMU and virtualbox .. probably
other things too.


> So the total length may be in 'urb->actual_length', but the actual data in
> the buffer may not be contiguous, because it's created from multiple
> smaller frames, some of which might not be full length?
>

yes, it's only contiguous for BULK.

> I dunno. That would explain the problem - actual_length is correct, but
> the 'copy_to_user()' still doesn't copy all the data, because it's
> fragmented.
>

no you got it, but your patch does not work. The best way would be to
revert it if someone wants to speed up BULK it should go down another
path, leaving the old working implementation untouched.

Markus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Sat, Feb 27, 2010 at 05:34:27AM +0100, Markus Rechberger wrote:
> On Sat, Feb 27, 2010 at 5:29 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> >
> > On Fri, 26 Feb 2010, Greg KH wrote:
> >>
> >> Yes, and that patch didn't touch the iso frames. ?That happens later on
> >> in the functions that were modified. ?The patch should not have had any
> >> affect on iso transfers. ?Unless I'm missing something?
> >
> > Hmm. What seems to happen is that for an isochronous transfer, the buffer
> > is split for each microframe. No?
> >
> 
> exactly. and each microframe has its own buffer length identifier.
> 
> the current behaviour breaks VMware, QEMU and virtualbox .. probably
> other things too.
> 
> 
> > So the total length may be in 'urb->actual_length', but the actual data in
> > the buffer may not be contiguous, because it's created from multiple
> > smaller frames, some of which might not be full length?
> >
> 
> yes, it's only contiguous for BULK.
> 
> > I dunno. That would explain the problem - actual_length is correct, but
> > the 'copy_to_user()' still doesn't copy all the data, because it's
> > fragmented.
> >
> 
> no you got it, but your patch does not work. The best way would be to
> revert it if someone wants to speed up BULK it should go down another
> path, leaving the old working implementation untouched.

Hm, so it's back to the original idea of just doing a kzalloc of the
initial buffer, that should solve the problem that Marcus found.

I'll go dig that back up and if you could test it, that would be most
appreciated.

thanks,

greg k-h

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Fri, Feb 26, 2010 at 09:17:37PM -0800, Greg KH wrote:
> On Sat, Feb 27, 2010 at 05:34:27AM +0100, Markus Rechberger wrote:
> > On Sat, Feb 27, 2010 at 5:29 AM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > >
> > > On Fri, 26 Feb 2010, Greg KH wrote:
> > >>
> > >> Yes, and that patch didn't touch the iso frames. ?That happens later on
> > >> in the functions that were modified. ?The patch should not have had any
> > >> affect on iso transfers. ?Unless I'm missing something?
> > >
> > > Hmm. What seems to happen is that for an isochronous transfer, the buffer
> > > is split for each microframe. No?
> > >
> > 
> > exactly. and each microframe has its own buffer length identifier.
> > 
> > the current behaviour breaks VMware, QEMU and virtualbox .. probably
> > other things too.
> > 
> > 
> > > So the total length may be in 'urb->actual_length', but the actual data in
> > > the buffer may not be contiguous, because it's created from multiple
> > > smaller frames, some of which might not be full length?
> > >
> > 
> > yes, it's only contiguous for BULK.
> > 
> > > I dunno. That would explain the problem - actual_length is correct, but
> > > the 'copy_to_user()' still doesn't copy all the data, because it's
> > > fragmented.
> > >
> > 
> > no you got it, but your patch does not work. The best way would be to
> > revert it if someone wants to speed up BULK it should go down another
> > path, leaving the old working implementation untouched.
> 
> Hm, so it's back to the original idea of just doing a kzalloc of the
> initial buffer, that should solve the problem that Marcus found.
> 
> I'll go dig that back up and if you could test it, that would be most
> appreciated.

Here, can you try this on top of everything?

thanks,

greg k-h


diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index a678186..252d3b4 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1168,7 +1168,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
 		return -ENOMEM;
 	}
 	if (uurb->buffer_length > 0) {
-		as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
+		as->urb->transfer_buffer = kzalloc(uurb->buffer_length,
 				GFP_KERNEL);
 		if (!as->urb->transfer_buffer) {
 			kfree(isopkt);
@@ -1312,9 +1312,9 @@ static int processcompl(struct async *as, void __user * __user *arg)
 	void __user *addr = as->userurb;
 	unsigned int i;
 
-	if (as->userbuffer && urb->actual_length)
+	if (as->userbuffer)
 		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
+				 urb->transfer_buffer_length))
 			goto err_out;
 	if (put_user(as->status, &userurb->status))
 		goto err_out;
@@ -1480,9 +1480,9 @@ static int processcompl_compat(struct async *as, void __user * __user *arg)
 	void __user *addr = as->userurb;
 	unsigned int i;
 
-	if (as->userbuffer && urb->actual_length)
+	if (as->userbuffer)
 		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
+				 urb->transfer_buffer_length))
 			return -EFAULT;
 	if (put_user(as->status, &userurb->status))
 		return -EFAULT;

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Sat, Feb 27, 2010 at 6:26 AM, Greg KH <greg@kroah.com> wrote:
> On Fri, Feb 26, 2010 at 09:17:37PM -0800, Greg KH wrote:
>> On Sat, Feb 27, 2010 at 05:34:27AM +0100, Markus Rechberger wrote:
>> > On Sat, Feb 27, 2010 at 5:29 AM, Linus Torvalds
>> > <torvalds@linux-foundation.org> wrote:
>> > >
>> > >
>> > > On Fri, 26 Feb 2010, Greg KH wrote:
>> > >>
>> > >> Yes, and that patch didn't touch the iso frames. ?That happens later on
>> > >> in the functions that were modified. ?The patch should not have had any
>> > >> affect on iso transfers. ?Unless I'm missing something?
>> > >
>> > > Hmm. What seems to happen is that for an isochronous transfer, the buffer
>> > > is split for each microframe. No?
>> > >
>> >
>> > exactly. and each microframe has its own buffer length identifier.
>> >
>> > the current behaviour breaks VMware, QEMU and virtualbox .. probably
>> > other things too.
>> >
>> >
>> > > So the total length may be in 'urb->actual_length', but the actual data in
>> > > the buffer may not be contiguous, because it's created from multiple
>> > > smaller frames, some of which might not be full length?
>> > >
>> >
>> > yes, it's only contiguous for BULK.
>> >
>> > > I dunno. That would explain the problem - actual_length is correct, but
>> > > the 'copy_to_user()' still doesn't copy all the data, because it's
>> > > fragmented.
>> > >
>> >
>> > no you got it, but your patch does not work. The best way would be to
>> > revert it if someone wants to speed up BULK it should go down another
>> > path, leaving the old working implementation untouched.
>>
>> Hm, so it's back to the original idea of just doing a kzalloc of the
>> initial buffer, that should solve the problem that Marcus found.
>>
>> I'll go dig that back up and if you could test it, that would be most
>> appreciated.
>
> Here, can you try this on top of everything?
>

just tested it, everything's back to normal again now!

thanks,
Markus

> thanks,
>
> greg k-h
>
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index a678186..252d3b4 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -1168,7 +1168,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb,
>                return -ENOMEM;
>        }
>        if (uurb->buffer_length > 0) {
> -               as->urb->transfer_buffer = kmalloc(uurb->buffer_length,
> +               as->urb->transfer_buffer = kzalloc(uurb->buffer_length,
>                                GFP_KERNEL);
>                if (!as->urb->transfer_buffer) {
>                        kfree(isopkt);
> @@ -1312,9 +1312,9 @@ static int processcompl(struct async *as, void __user * __user *arg)
>        void __user *addr = as->userurb;
>        unsigned int i;
>
> -       if (as->userbuffer && urb->actual_length)
> +       if (as->userbuffer)
>                if (copy_to_user(as->userbuffer, urb->transfer_buffer,
> -                                urb->actual_length))
> +                                urb->transfer_buffer_length))
>                        goto err_out;
>        if (put_user(as->status, &userurb->status))
>                goto err_out;
> @@ -1480,9 +1480,9 @@ static int processcompl_compat(struct async *as, void __user * __user *arg)
>        void __user *addr = as->userurb;
>        unsigned int i;
>
> -       if (as->userbuffer && urb->actual_length)
> +       if (as->userbuffer)
>                if (copy_to_user(as->userbuffer, urb->transfer_buffer,
> -                                urb->actual_length))
> +                                urb->transfer_buffer_length))
>                        return -EFAULT;
>        if (put_user(as->status, &userurb->status))
>                return -EFAULT;
>

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Sat, Feb 27, 2010 at 06:38:09AM +0100, Markus Rechberger wrote:
> On Sat, Feb 27, 2010 at 6:26 AM, Greg KH <greg@kroah.com> wrote:
> > On Fri, Feb 26, 2010 at 09:17:37PM -0800, Greg KH wrote:
> >> On Sat, Feb 27, 2010 at 05:34:27AM +0100, Markus Rechberger wrote:
> >> > On Sat, Feb 27, 2010 at 5:29 AM, Linus Torvalds
> >> > <torvalds@linux-foundation.org> wrote:
> >> > >
> >> > >
> >> > > On Fri, 26 Feb 2010, Greg KH wrote:
> >> > >>
> >> > >> Yes, and that patch didn't touch the iso frames. ?That happens later on
> >> > >> in the functions that were modified. ?The patch should not have had any
> >> > >> affect on iso transfers. ?Unless I'm missing something?
> >> > >
> >> > > Hmm. What seems to happen is that for an isochronous transfer, the buffer
> >> > > is split for each microframe. No?
> >> > >
> >> >
> >> > exactly. and each microframe has its own buffer length identifier.
> >> >
> >> > the current behaviour breaks VMware, QEMU and virtualbox .. probably
> >> > other things too.
> >> >
> >> >
> >> > > So the total length may be in 'urb->actual_length', but the actual data in
> >> > > the buffer may not be contiguous, because it's created from multiple
> >> > > smaller frames, some of which might not be full length?
> >> > >
> >> >
> >> > yes, it's only contiguous for BULK.
> >> >
> >> > > I dunno. That would explain the problem - actual_length is correct, but
> >> > > the 'copy_to_user()' still doesn't copy all the data, because it's
> >> > > fragmented.
> >> > >
> >> >
> >> > no you got it, but your patch does not work. The best way would be to
> >> > revert it if someone wants to speed up BULK it should go down another
> >> > path, leaving the old working implementation untouched.
> >>
> >> Hm, so it's back to the original idea of just doing a kzalloc of the
> >> initial buffer, that should solve the problem that Marcus found.
> >>
> >> I'll go dig that back up and if you could test it, that would be most
> >> appreciated.
> >
> > Here, can you try this on top of everything?
> >
> 
> just tested it, everything's back to normal again now!

Thanks for testing, I'll queue it up for Linus and then add it back to
.32-stable.

Linus, I know you didn't want to do a kzalloc, but it looks like this is
the easiest/simplest thing to do, unless you can think of something
else?

thanks,

greg k-h

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Sat, Feb 27, 2010 at 6:48 AM, Greg KH <greg@kroah.com> wrote:
> On Sat, Feb 27, 2010 at 06:38:09AM +0100, Markus Rechberger wrote:
>> On Sat, Feb 27, 2010 at 6:26 AM, Greg KH <greg@kroah.com> wrote:
>> > On Fri, Feb 26, 2010 at 09:17:37PM -0800, Greg KH wrote:
>> >> On Sat, Feb 27, 2010 at 05:34:27AM +0100, Markus Rechberger wrote:
>> >> > On Sat, Feb 27, 2010 at 5:29 AM, Linus Torvalds
>> >> > <torvalds@linux-foundation.org> wrote:
>> >> > >
>> >> > >
>> >> > > On Fri, 26 Feb 2010, Greg KH wrote:
>> >> > >>
>> >> > >> Yes, and that patch didn't touch the iso frames. ?That happens later on
>> >> > >> in the functions that were modified. ?The patch should not have had any
>> >> > >> affect on iso transfers. ?Unless I'm missing something?
>> >> > >
>> >> > > Hmm. What seems to happen is that for an isochronous transfer, the buffer
>> >> > > is split for each microframe. No?
>> >> > >
>> >> >
>> >> > exactly. and each microframe has its own buffer length identifier.
>> >> >
>> >> > the current behaviour breaks VMware, QEMU and virtualbox .. probably
>> >> > other things too.
>> >> >
>> >> >
>> >> > > So the total length may be in 'urb->actual_length', but the actual data in
>> >> > > the buffer may not be contiguous, because it's created from multiple
>> >> > > smaller frames, some of which might not be full length?
>> >> > >
>> >> >
>> >> > yes, it's only contiguous for BULK.
>> >> >
>> >> > > I dunno. That would explain the problem - actual_length is correct, but
>> >> > > the 'copy_to_user()' still doesn't copy all the data, because it's
>> >> > > fragmented.
>> >> > >
>> >> >
>> >> > no you got it, but your patch does not work. The best way would be to
>> >> > revert it if someone wants to speed up BULK it should go down another
>> >> > path, leaving the old working implementation untouched.
>> >>
>> >> Hm, so it's back to the original idea of just doing a kzalloc of the
>> >> initial buffer, that should solve the problem that Marcus found.
>> >>
>> >> I'll go dig that back up and if you could test it, that would be most
>> >> appreciated.
>> >
>> > Here, can you try this on top of everything?
>> >
>>
>> just tested it, everything's back to normal again now!
>
> Thanks for testing, I'll queue it up for Linus and then add it back to
> .32-stable.
>
> Linus, I know you didn't want to do a kzalloc, but it looks like this is
> the easiest/simplest thing to do, unless you can think of something
> else?
>

do you have any idea what to cherrypick in order to get rid of that
flickering with the intel GM45?
The flickering itself occures randomly, it was not there with the
original Ubuntu kernel (2.6.31 with alot patches)
This is pretty much a standard graphicchip nowadays....

Xorg.0.log:
(II) intel(0): Integrated Graphics Chipset: Intel(R) GM45
(--) intel(0): Chipset: "GM45"
$ lspci
00:02.0 VGA compatible controller: Intel Corporation Mobile 4 Series
Chipset Integrated Graphics Controller (rev 07)
01:00.0 VGA compatible controller: nVidia Corporation Device 0a74 (rev a2)

Markus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Pekka Enberg]

On Sat, Feb 27, 2010 at 1:00 PM, Markus Rechberger
<mrechberger@gmail.com> wrote:
>> Linus, I know you didn't want to do a kzalloc, but it looks like this is
>> the easiest/simplest thing to do, unless you can think of something
>> else?
>
> do you have any idea what to cherrypick in order to get rid of that
> flickering with the intel GM45?
> The flickering itself occures randomly, it was not there with the
> original Ubuntu kernel (2.6.31 with alot patches)
> This is pretty much a standard graphicchip nowadays....

Yeah, same here. I tried to bisect the damn thing but didn't find any
single commit. I think the problem was introduced in in the DRM merge
between 2.6.31 and 2.6.32-rc1. Some people suggested privately that it
might be related to frame buffer compression. Unfortunately I wasn't
able to revert that particular patch.

                        Pekka

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Pekka Enberg]

On Sat, Feb 27, 2010 at 2:15 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> On Sat, Feb 27, 2010 at 1:00 PM, Markus Rechberger
> <mrechberger@gmail.com> wrote:
>>> Linus, I know you didn't want to do a kzalloc, but it looks like this is
>>> the easiest/simplest thing to do, unless you can think of something
>>> else?
>>
>> do you have any idea what to cherrypick in order to get rid of that
>> flickering with the intel GM45?
>> The flickering itself occures randomly, it was not there with the
>> original Ubuntu kernel (2.6.31 with alot patches)
>> This is pretty much a standard graphicchip nowadays....
>
> Yeah, same here. I tried to bisect the damn thing but didn't find any
> single commit. I think the problem was introduced in in the DRM merge
> between 2.6.31 and 2.6.32-rc1. Some people suggested privately that it
> might be related to frame buffer compression. Unfortunately I wasn't
> able to revert that particular patch.

Btw, after I reinstalled Ubuntu 9.10 from scratch for unrelated
reasons, I haven't seen the flicker effect. Dunno if that's just a
coincidence or if userspace has something to do with the bug.

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Jesse Barnes]

On Sat, 27 Feb 2010 14:17:14 +0200
Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On Sat, Feb 27, 2010 at 2:15 PM, Pekka Enberg
> <penberg@cs.helsinki.fi> wrote:
> > On Sat, Feb 27, 2010 at 1:00 PM, Markus Rechberger
> > <mrechberger@gmail.com> wrote:
> >>> Linus, I know you didn't want to do a kzalloc, but it looks like
> >>> this is the easiest/simplest thing to do, unless you can think of
> >>> something else?
> >>
> >> do you have any idea what to cherrypick in order to get rid of that
> >> flickering with the intel GM45?
> >> The flickering itself occures randomly, it was not there with the
> >> original Ubuntu kernel (2.6.31 with alot patches)
> >> This is pretty much a standard graphicchip nowadays....
> >
> > Yeah, same here. I tried to bisect the damn thing but didn't find
> > any single commit. I think the problem was introduced in in the DRM
> > merge between 2.6.31 and 2.6.32-rc1. Some people suggested
> > privately that it might be related to frame buffer compression.
> > Unfortunately I wasn't able to revert that particular patch.
> 
> Btw, after I reinstalled Ubuntu 9.10 from scratch for unrelated
> reasons, I haven't seen the flicker effect. Dunno if that's just a
> coincidence or if userspace has something to do with the bug.

Some people reported ACPI related flicker due to lid detection taking a
long time and X periodically probing for it.  Maybe that's what you
were seeing?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Sat, Feb 27, 2010 at 5:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Sat, 27 Feb 2010 14:17:14 +0200
> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
>> On Sat, Feb 27, 2010 at 2:15 PM, Pekka Enberg
>> <penberg@cs.helsinki.fi> wrote:
>> > On Sat, Feb 27, 2010 at 1:00 PM, Markus Rechberger
>> > <mrechberger@gmail.com> wrote:
>> >>> Linus, I know you didn't want to do a kzalloc, but it looks like
>> >>> this is the easiest/simplest thing to do, unless you can think of
>> >>> something else?
>> >>
>> >> do you have any idea what to cherrypick in order to get rid of that
>> >> flickering with the intel GM45?
>> >> The flickering itself occures randomly, it was not there with the
>> >> original Ubuntu kernel (2.6.31 with alot patches)
>> >> This is pretty much a standard graphicchip nowadays....
>> >
>> > Yeah, same here. I tried to bisect the damn thing but didn't find
>> > any single commit. I think the problem was introduced in in the DRM
>> > merge between 2.6.31 and 2.6.32-rc1. Some people suggested
>> > privately that it might be related to frame buffer compression.
>> > Unfortunately I wasn't able to revert that particular patch.
>>
>> Btw, after I reinstalled Ubuntu 9.10 from scratch for unrelated
>> reasons, I haven't seen the flicker effect. Dunno if that's just a
>> coincidence or if userspace has something to do with the bug.
>
> Some people reported ACPI related flicker due to lid detection taking a
> long time and X periodically probing for it.  Maybe that's what you
> were seeing?
>

little bit offtopic there's another bug with the intel driver. Seems
like there's a memory pool which collects all the memory during the
runtime and especially makes the memory usage go up:
http://bugs.freedesktop.org/show_bug.cgi?id=26708
Maybe someone at Intel can have a closer look at that one...

another obvious bug seems to be offloading mpeg2 video to the GPU it
takes more CPU than decoding mpeg2 with software. Offloading is quite
important nowadays due HDTV. The only one which has working linux
support is NVidia with their closed source drivers.

Markus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Sat, Feb 27, 2010 at 7:08 PM, Markus Rechberger
<mrechberger@gmail.com> wrote:
> On Sat, Feb 27, 2010 at 5:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> On Sat, 27 Feb 2010 14:17:14 +0200
>> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>>
>>> On Sat, Feb 27, 2010 at 2:15 PM, Pekka Enberg
>>> <penberg@cs.helsinki.fi> wrote:
>>> > On Sat, Feb 27, 2010 at 1:00 PM, Markus Rechberger
>>> > <mrechberger@gmail.com> wrote:
>>> >>> Linus, I know you didn't want to do a kzalloc, but it looks like
>>> >>> this is the easiest/simplest thing to do, unless you can think of
>>> >>> something else?
>>> >>
>>> >> do you have any idea what to cherrypick in order to get rid of that
>>> >> flickering with the intel GM45?
>>> >> The flickering itself occures randomly, it was not there with the
>>> >> original Ubuntu kernel (2.6.31 with alot patches)
>>> >> This is pretty much a standard graphicchip nowadays....
>>> >
>>> > Yeah, same here. I tried to bisect the damn thing but didn't find
>>> > any single commit. I think the problem was introduced in in the DRM
>>> > merge between 2.6.31 and 2.6.32-rc1. Some people suggested
>>> > privately that it might be related to frame buffer compression.
>>> > Unfortunately I wasn't able to revert that particular patch.
>>>
>>> Btw, after I reinstalled Ubuntu 9.10 from scratch for unrelated
>>> reasons, I haven't seen the flicker effect. Dunno if that's just a
>>> coincidence or if userspace has something to do with the bug.
>>
>> Some people reported ACPI related flicker due to lid detection taking a
>> long time and X periodically probing for it.  Maybe that's what you
>> were seeing?
>>
>
> little bit offtopic there's another bug with the intel driver. Seems
> like there's a memory pool which collects all the memory during the
> runtime and especially makes the memory usage go up:
> http://bugs.freedesktop.org/show_bug.cgi?id=26708
> Maybe someone at Intel can have a closer look at that one...
>
> another obvious bug seems to be offloading mpeg2 video to the GPU it
> takes more CPU than decoding mpeg2 with software. Offloading is quite
> important nowadays due HDTV. The only one which has working linux
> support is NVidia with their closed source drivers.
>

Intel Graphic seems to be the pure chaos with Linux ....
I booted the old kernel again and did some debugging because the load
of udevd sometimes goes up.. the netlink interface seems to flood udev
with following message with ubuntu karmic 2.6.31
LINE: change@/devices/pci0000:00/0000:00:02.0/drm/card0
LINE: ACTION=change
LINE: DEVPATH=/devices/pci0000:00/0000:00:02.0/drm/card0
LINE: SUBSYSTEM=drm
LINE: HOTPLUG=1
LINE: MAJOR=226
LINE: MINOR=0
LINE: DEVNAME=dri/card0
LINE: DEVTYPE=drm_minor
LINE: SEQNUM=47819

obviosly this seems to be solved with the newer kernel, but there we
have that flickering issue and OpenGL support is messy

Aside of that some people sent me emails offlist that intel stopped
working correctly for them starting from 2.6.27 on. Is anyone from
Intel still working on the graphicdriver?

Some of our business customers are now switching to HDTV compatible
systems and Intel seems to be a nogo right now, NVidia Tegra seems to
have potential.

Markus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Alan Stern]

On Fri, 26 Feb 2010, Greg KH wrote:

> Hm, so it's back to the original idea of just doing a kzalloc of the
> initial buffer, that should solve the problem that Marcus found.
> 
> I'll go dig that back up and if you could test it, that would be most
> appreciated.

Here's a better solution.  In theory we could copy just the individual 
packets from within the transfer buffer, but that would probably take 
longer than simply copying the whole buffer.

(This was a little hasty; I haven't even compile-tested the patch. 
Some small fixes may be needed.)

Alan Stern


-----------------------------------------------------------------------

This patch fixes a bug in the way isochronous input data is returned
to userspace for usbfs transfers.  The entire buffer must be copied,
not just the first actual_length bytes, because the individual packets
will be discontiguous if any of them are short.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: stable <stable@kernel.org>

---

Index: usb-2.6/drivers/usb/core/devio.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/devio.c
+++ usb-2.6/drivers/usb/core/devio.c
@@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
 			free_async(as);
 			return -ENOMEM;
 		}
+		/* Isochronous input data may end up being discontiguous
+		 * if some of the packets are short.  Clear the buffer so
+		 * that the gaps don't leak kernel data to userspace.
+		 */
+		if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
+			memset(as->urb->transfer_buffer, 0,
+					uurb->buffer_length);
 	}
 	as->urb->dev = ps->dev;
 	as->urb->pipe = (uurb->type << 30) |
@@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
 	void __user *addr = as->userurb;
 	unsigned int i;
 
-	if (as->userbuffer && urb->actual_length)
-		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
+	if (as->userbuffer && urb->actual_length) {
+		if (urb->number_of_packets > 0)		/* Isochronous */
+			i = urb->transfer_buffer_length;
+		else					/* Non-Isoc */
+			i = urb->actual_length;
+		if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
 			goto err_out;
+	}
 	if (put_user(as->status, &userurb->status))
 		goto err_out;
 	if (put_user(urb->actual_length, &userurb->actual_length))

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Sat, Feb 27, 2010 at 12:20:35PM -0500, Alan Stern wrote:
> On Fri, 26 Feb 2010, Greg KH wrote:
> 
> > Hm, so it's back to the original idea of just doing a kzalloc of the
> > initial buffer, that should solve the problem that Marcus found.
> > 
> > I'll go dig that back up and if you could test it, that would be most
> > appreciated.
> 
> Here's a better solution.  In theory we could copy just the individual 
> packets from within the transfer buffer, but that would probably take 
> longer than simply copying the whole buffer.
> 
> (This was a little hasty; I haven't even compile-tested the patch. 
> Some small fixes may be needed.)

Markus, did you test this patch out?

thanks,

greg k-h

> -----------------------------------------------------------------------
> 
> This patch fixes a bug in the way isochronous input data is returned
> to userspace for usbfs transfers.  The entire buffer must be copied,
> not just the first actual_length bytes, because the individual packets
> will be discontiguous if any of them are short.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: stable <stable@kernel.org>
> 
> ---
> 
> Index: usb-2.6/drivers/usb/core/devio.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/devio.c
> +++ usb-2.6/drivers/usb/core/devio.c
> @@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
>  			free_async(as);
>  			return -ENOMEM;
>  		}
> +		/* Isochronous input data may end up being discontiguous
> +		 * if some of the packets are short.  Clear the buffer so
> +		 * that the gaps don't leak kernel data to userspace.
> +		 */
> +		if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
> +			memset(as->urb->transfer_buffer, 0,
> +					uurb->buffer_length);
>  	}
>  	as->urb->dev = ps->dev;
>  	as->urb->pipe = (uurb->type << 30) |
> @@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
>  	void __user *addr = as->userurb;
>  	unsigned int i;
>  
> -	if (as->userbuffer && urb->actual_length)
> -		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
> -				 urb->actual_length))
> +	if (as->userbuffer && urb->actual_length) {
> +		if (urb->number_of_packets > 0)		/* Isochronous */
> +			i = urb->transfer_buffer_length;
> +		else					/* Non-Isoc */
> +			i = urb->actual_length;
> +		if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
>  			goto err_out;
> +	}
>  	if (put_user(as->status, &userurb->status))
>  		goto err_out;
>  	if (put_user(urb->actual_length, &userurb->actual_length))

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Wed, Mar 3, 2010 at 1:09 AM, Greg KH <greg@kroah.com> wrote:
> On Sat, Feb 27, 2010 at 12:20:35PM -0500, Alan Stern wrote:
>> On Fri, 26 Feb 2010, Greg KH wrote:
>>
>> > Hm, so it's back to the original idea of just doing a kzalloc of the
>> > initial buffer, that should solve the problem that Marcus found.
>> >
>> > I'll go dig that back up and if you could test it, that would be most
>> > appreciated.
>>
>> Here's a better solution.  In theory we could copy just the individual
>> packets from within the transfer buffer, but that would probably take
>> longer than simply copying the whole buffer.
>>
>> (This was a little hasty; I haven't even compile-tested the patch.
>> Some small fixes may be needed.)
>
> Markus, did you test this patch out?
>

sorry just saw this now, will do tomorrow morning...

Markus

> thanks,
>
> greg k-h
>
>> -----------------------------------------------------------------------
>>
>> This patch fixes a bug in the way isochronous input data is returned
>> to userspace for usbfs transfers.  The entire buffer must be copied,
>> not just the first actual_length bytes, because the individual packets
>> will be discontiguous if any of them are short.
>>
>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>> CC: stable <stable@kernel.org>
>>
>> ---
>>
>> Index: usb-2.6/drivers/usb/core/devio.c
>> ===================================================================
>> --- usb-2.6.orig/drivers/usb/core/devio.c
>> +++ usb-2.6/drivers/usb/core/devio.c
>> @@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
>>                       free_async(as);
>>                       return -ENOMEM;
>>               }
>> +             /* Isochronous input data may end up being discontiguous
>> +              * if some of the packets are short.  Clear the buffer so
>> +              * that the gaps don't leak kernel data to userspace.
>> +              */
>> +             if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
>> +                     memset(as->urb->transfer_buffer, 0,
>> +                                     uurb->buffer_length);
>>       }
>>       as->urb->dev = ps->dev;
>>       as->urb->pipe = (uurb->type << 30) |
>> @@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
>>       void __user *addr = as->userurb;
>>       unsigned int i;
>>
>> -     if (as->userbuffer && urb->actual_length)
>> -             if (copy_to_user(as->userbuffer, urb->transfer_buffer,
>> -                              urb->actual_length))
>> +     if (as->userbuffer && urb->actual_length) {
>> +             if (urb->number_of_packets > 0)         /* Isochronous */
>> +                     i = urb->transfer_buffer_length;
>> +             else                                    /* Non-Isoc */
>> +                     i = urb->actual_length;
>> +             if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
>>                       goto err_out;
>> +     }
>>       if (put_user(as->status, &userurb->status))
>>               goto err_out;
>>       if (put_user(urb->actual_length, &userurb->actual_length))
>

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Markus Rechberger]

On Wed, Mar 3, 2010 at 1:09 AM, Greg KH <greg@kroah.com> wrote:
> On Sat, Feb 27, 2010 at 12:20:35PM -0500, Alan Stern wrote:
>> On Fri, 26 Feb 2010, Greg KH wrote:
>>
>> > Hm, so it's back to the original idea of just doing a kzalloc of the
>> > initial buffer, that should solve the problem that Marcus found.
>> >
>> > I'll go dig that back up and if you could test it, that would be most
>> > appreciated.
>>
>> Here's a better solution.  In theory we could copy just the individual
>> packets from within the transfer buffer, but that would probably take
>> longer than simply copying the whole buffer.
>>
>> (This was a little hasty; I haven't even compile-tested the patch.
>> Some small fixes may be needed.)
>
> Markus, did you test this patch out?
>

works properly with ISO, (bulk is not tested from my side). please
merge asap this also affects scanners (SANE).

Markus

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Greg KH]

On Sat, Mar 06, 2010 at 05:30:44PM +0100, Markus Rechberger wrote:
> On Wed, Mar 3, 2010 at 1:09 AM, Greg KH <greg@kroah.com> wrote:
> > On Sat, Feb 27, 2010 at 12:20:35PM -0500, Alan Stern wrote:
> >> On Fri, 26 Feb 2010, Greg KH wrote:
> >>
> >> > Hm, so it's back to the original idea of just doing a kzalloc of the
> >> > initial buffer, that should solve the problem that Marcus found.
> >> >
> >> > I'll go dig that back up and if you could test it, that would be most
> >> > appreciated.
> >>
> >> Here's a better solution. ?In theory we could copy just the individual
> >> packets from within the transfer buffer, but that would probably take
> >> longer than simply copying the whole buffer.
> >>
> >> (This was a little hasty; I haven't even compile-tested the patch.
> >> Some small fixes may be needed.)
> >
> > Markus, did you test this patch out?
> >
> 
> works properly with ISO, (bulk is not tested from my side). please
> merge asap this also affects scanners (SANE).

Great, Alan, care to resend this to me with a signed-off-by line so I
can get it queued up properly?

thanks,

greg k-h

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Alan Stern]

This patch (as1352) fixes a bug in the way isochronous input data is
returned to userspace for usbfs transfers.  The entire buffer must be
copied, not just the first actual_length bytes, because the individual
packets will be discontiguous if any of them are short.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: stable <stable@kernel.org>

---

On Sat, 6 Mar 2010, Greg KH wrote:

> Great, Alan, care to resend this to me with a signed-off-by line so I
> can get it queued up properly?

Here it is.



Index: usb-2.6/drivers/usb/core/devio.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/devio.c
+++ usb-2.6/drivers/usb/core/devio.c
@@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
 			free_async(as);
 			return -ENOMEM;
 		}
+		/* Isochronous input data may end up being discontiguous
+		 * if some of the packets are short.  Clear the buffer so
+		 * that the gaps don't leak kernel data to userspace.
+		 */
+		if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
+			memset(as->urb->transfer_buffer, 0,
+					uurb->buffer_length);
 	}
 	as->urb->dev = ps->dev;
 	as->urb->pipe = (uurb->type << 30) |
@@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
 	void __user *addr = as->userurb;
 	unsigned int i;
 
-	if (as->userbuffer && urb->actual_length)
-		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
+	if (as->userbuffer && urb->actual_length) {
+		if (urb->number_of_packets > 0)		/* Isochronous */
+			i = urb->transfer_buffer_length;
+		else					/* Non-Isoc */
+			i = urb->actual_length;
+		if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
 			goto err_out;
+	}
 	if (put_user(as->status, &userurb->status))
 		goto err_out;
 	if (put_user(urb->actual_length, &userurb->actual_length))

Re: 2.6.33 bugs (USBFS, Intel graphic)

[Linus Torvalds]

On Sat, 27 Feb 2010, Markus Rechberger wrote:
> 
> commit d4a4683ca054ed9917dfc9e3ff0f7ecf74ad90d6 upstream.
> 
> this patch breaks isochronous USBFS support, please revert that patch!
> 
> http://sundtek.de/images/tvtime-bildfehler.jpg
> 
> with the patch reverted:
> http://sundtek.de/images/tvtime-working.png

Hmm. That would seem to mean that either the app (tvtime) depended in some 
_really_ interesting way on some random data that was never even 
transferred from the device, or 'urb->actual_length' isn't actually 
reliable in some cases.

Does this patch (_instead_ of reverting things) change any behavior? Do 
you get that warning? It will zero-fill the remainder of the buffer.

(UNTESTED! It compiled for me, and looks ok, but whatever..)

		Linus
---
 drivers/usb/core/devio.c |   53 +++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index a678186..8afee02 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1305,6 +1305,47 @@ static int proc_unlinkurb(struct dev_state *ps, void __user *arg)
 	return 0;
 }
 
+/*
+ * Fixme! We don't have a good memclear_user(), so we do it with a
+ * stupid copy_to_user() loop from a zero buffer.
+ */
+static int memclear_user(void __user *dst, long len)
+{
+	while (len > 0) {
+		static const char zeroes[128];
+		unsigned long n = len;
+
+		if (n > sizeof(zeroes))
+			n = sizeof(zeroes);
+		if (copy_to_user(dst, zeroes, n))
+			return -EFAULT;
+		dst += n;
+		len -= n;
+	}
+	return 0;
+}
+
+static int copy_buffer_to_user(struct async *as, struct urb *urb)
+{
+	void __user *dst = as->userbuffer;
+	void *src;
+	unsigned long len, full;
+
+	if (!dst)
+		return 0;
+
+	len = urb->actual_length;
+	full = urb->transfer_buffer_length;
+	if (WARN_ONCE(len > full, "actual_length (%lu) > transfer_buffer_length (%lu)", len, full))
+		len = full;
+
+	src = urb->transfer_buffer;
+	if (copy_to_user(dst, src, len))
+		return -EFAULT;
+
+	return memclear_user(dst + len, full - len);
+}
+
 static int processcompl(struct async *as, void __user * __user *arg)
 {
 	struct urb *urb = as->urb;
@@ -1312,10 +1353,8 @@ static int processcompl(struct async *as, void __user * __user *arg)
 	void __user *addr = as->userurb;
 	unsigned int i;
 
-	if (as->userbuffer && urb->actual_length)
-		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
-			goto err_out;
+	if (copy_buffer_to_user(as, urb))
+		goto err_out;
 	if (put_user(as->status, &userurb->status))
 		goto err_out;
 	if (put_user(urb->actual_length, &userurb->actual_length))
@@ -1480,10 +1519,8 @@ static int processcompl_compat(struct async *as, void __user * __user *arg)
 	void __user *addr = as->userurb;
 	unsigned int i;
 
-	if (as->userbuffer && urb->actual_length)
-		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
-			return -EFAULT;
+	if (copy_buffer_to_user(as, urb))
+		return -EFAULT;
 	if (put_user(as->status, &userurb->status))
 		return -EFAULT;
 	if (put_user(urb->actual_length, &userurb->actual_length))