| Message ID | d9def9db1003240921g5c023ccdp7fb94eb1f872b1a9@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Markus, On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger <mrechberger@gmail.com> wrote: > Hi, > > on IRC I was recommended to submit this information to the mailinglist > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527 The bug itself doesn't list much information. It basically says "usbfs is broken." We need some proof of what is going wrong, preferably something that can be tested when a fix is available. > 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.) The general procedure for fixes to reach Ubuntu is to have them sent upstream and be included in the mainline linux, and then to have the fix read the -stable tree. We then pull patches from the -stable tree for inclusion in Ubuntu. If you can show that a specific patch has been found to fix an issue in Ubuntu and it has been accepted upstream by the maintainers of the kernel subsystem this impacts, we may be able to accept the patch as a pre-stable fix, in anticipation that it will be accepted into the -stable tree in the future. What is the origin of this patch? Is it from upstream somewhere? A commit hash from linux-2.6 or some other tree would be very helpful. > 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)) > > It would be nice if this patch could go into the ubuntu lucid kernel > as soon as possible. > > Thanks, > Markus -- Chase
On Wed, Mar 24, 2010 at 5:34 PM, Chase Douglas <chase.douglas@canonical.com> wrote: > Hi Markus, > > On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger > <mrechberger@gmail.com> wrote: >> Hi, >> >> on IRC I was recommended to submit this information to the mailinglist >> >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527 > > The bug itself doesn't list much information. It basically says "usbfs > is broken." We need some proof of what is going wrong, preferably > something that can be tested when a fix is available. > >> 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.) > > The general procedure for fixes to reach Ubuntu is to have them sent > upstream and be included in the mainline linux, and then to have the > fix read the -stable tree. We then pull patches from the -stable tree > for inclusion in Ubuntu. > > If you can show that a specific patch has been found to fix an issue > in Ubuntu and it has been accepted upstream by the maintainers of the > kernel subsystem this impacts, we may be able to accept the patch as a > pre-stable fix, in anticipation that it will be accepted into the > -stable tree in the future. > > What is the origin of this patch? Is it from upstream somewhere? A > commit hash from linux-2.6 or some other tree would be very helpful. Hi, sorry I'm a little bit busy the patch is already upstream but not in ubuntu right now I tested Ubuntu Lucid yesterday and it's affected by that bug http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7152b592593b9d48b33f8997b1dfd6df9143f7ec Markus > >> 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)) >> >> It would be nice if this patch could go into the ubuntu lucid kernel >> as soon as possible. >> >> Thanks, >> Markus > > -- Chase >
Chase Douglas wrote: > Hi Markus, > > On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger > <mrechberger@gmail.com> wrote: >> Hi, >> >> on IRC I was recommended to submit this information to the mailinglist >> >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527 > > The bug itself doesn't list much information. It basically says "usbfs > is broken." We need some proof of what is going wrong, preferably > something that can be tested when a fix is available. > >> 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.) > > The general procedure for fixes to reach Ubuntu is to have them sent > upstream and be included in the mainline linux, and then to have the > fix read the -stable tree. We then pull patches from the -stable tree > for inclusion in Ubuntu. > > If you can show that a specific patch has been found to fix an issue > in Ubuntu and it has been accepted upstream by the maintainers of the > kernel subsystem this impacts, we may be able to accept the patch as a > pre-stable fix, in anticipation that it will be accepted into the > -stable tree in the future. > > What is the origin of this patch? Is it from upstream somewhere? A > commit hash from linux-2.6 or some other tree would be very helpful. > >> 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)) >> >> It would be nice if this patch could go into the ubuntu lucid kernel >> as soon as possible. >> >> Thanks, >> Markus > > -- Chase > Well it seems to come from Alan Stern and be cc'ed to stable. So it should/would come from there. But I have not seen it in the queue, yet. But it is in upstream: commit 7152b592593b9d48b33f8997b1dfd6df9143f7ec Author: Alan Stern <stern@rowland.harvard.edu> Date: Sat Mar 6 15:04:03 2010 -0500 USB: fix usbfs regression
On Wed, Mar 24, 2010 at 12:39 PM, Stefan Bader <stefan.bader@canonical.com> wrote: > Chase Douglas wrote: >> Hi Markus, >> >> On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger >> <mrechberger@gmail.com> wrote: >>> Hi, >>> >>> on IRC I was recommended to submit this information to the mailinglist >>> >>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527 >> >> The bug itself doesn't list much information. It basically says "usbfs >> is broken." We need some proof of what is going wrong, preferably >> something that can be tested when a fix is available. >> >>> 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.) >> >> The general procedure for fixes to reach Ubuntu is to have them sent >> upstream and be included in the mainline linux, and then to have the >> fix read the -stable tree. We then pull patches from the -stable tree >> for inclusion in Ubuntu. >> >> If you can show that a specific patch has been found to fix an issue >> in Ubuntu and it has been accepted upstream by the maintainers of the >> kernel subsystem this impacts, we may be able to accept the patch as a >> pre-stable fix, in anticipation that it will be accepted into the >> -stable tree in the future. >> >> What is the origin of this patch? Is it from upstream somewhere? A >> commit hash from linux-2.6 or some other tree would be very helpful. >> >>> 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)) >>> >>> It would be nice if this patch could go into the ubuntu lucid kernel >>> as soon as possible. >>> >>> Thanks, >>> Markus >> >> -- Chase >> > Well it seems to come from Alan Stern and be cc'ed to stable. So it should/would > come from there. But I have not seen it in the queue, yet. But it is in upstream: > > commit 7152b592593b9d48b33f8997b1dfd6df9143f7ec > Author: Alan Stern <stern@rowland.harvard.edu> > Date: Sat Mar 6 15:04:03 2010 -0500 > > USB: fix usbfs regression Ahh, I didn't realize Alan is the maintainer of the code. I'll leave it up to you, Stefan, to determine how to handle this. -- Chase
One addition: It might be that we are talking about USB_DEVICEFS. And that is not active anymore. Stefan
Stefan Bader wrote: > One addition: > It might be that we are talking about USB_DEVICEFS. And that is not active anymore. > > Stefan > Ok, upon feedback from Markus, he does not need the usb filesystem being mounted.
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))
Hi, on IRC I was recommended to submit this information to the mailinglist https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527 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> --- It would be nice if this patch could go into the ubuntu lucid kernel as soon as possible. Thanks, Markus