Re: gadgetfs broken since 7f7f25e8
Am 11.03.2015 um 11:29 schrieb Alexander Holler: Am 07.03.2015 um 12:23 schrieb Alexander Holler: In detail it's only usable once (3.19). When unmounting it throws one or two warnings because of too much puts (kernel/module.c:963, see below). The result is that a subsequent mount afterwards fails because the old instance is still busy (Resource temporarily unavailable). I haven't looked deeper into that problem up to now. Regards, Alexander Holler [ 151.425966] gadgetfs: USB Gadget filesystem, version 24 Aug 2004 [ 151.479616] gadgetfs: bound to musb-hdrc driver [ 151.781095] gadgetfs: connected [ 151.785871] gadgetfs: disconnected [ 151.864187] gadgetfs: connected [ 151.873258] gadgetfs: configuration #1 [ 151.962240] musb_g_ep0_irq 804: SETUP packet len 0 != 8 ? [ 155.065272] input: USB Keyboard as /devices/platform/ocp/4740.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.0/0003:04D9:1603.0003/input/input2 [ 155.119746] hid-generic 0003:04D9:1603.0003: input,hidraw0: USB HID v1.10 Keyboard [ USB Keyboard] on usb-musb-hdrc.1.auto-1/input0 [ 155.138194] input: USB Keyboard as /devices/platform/ocp/4740.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.1/0003:04D9:1603.0004/input/input3 [ 155.189783] hid-generic 0003:04D9:1603.0004: input,hidraw1: USB HID v1.10 Device [ USB Keyboard] on usb-musb-hdrc.1.auto-1/input1 [ 155.190002] gadgetfs: disconnected [ 155.190352] [ cut here ] [ 155.190389] WARNING: CPU: 0 PID: 1290 at kernel/module.c:963 module_put+0x54/0x5c() [ 155.190396] Modules linked in: gadgetfs usb_f_ecm usb_f_rndis u_ether libcomposite configfs ipv6 bnep bluetooth rfkill hid_generic tda998x tilcdc drm_kms_helper ti_cpsw ptp drm usbhid pps_core davinci_cpdma omap_mailbox cppi41 [last unloaded: g_ether] [ 155.190459] CPU: 0 PID: 1290 Comm: usb-mitm Not tainted 3.19.0-bbb-00080-g5e25c2a #246 [ 155.190466] Hardware name: Generic AM33XX (Flattened Device Tree) [ 155.190505] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 155.190527] [] (show_stack) from [] (warn_slowpath_common+0x80/0xa8) [ 155.190542] [] (warn_slowpath_common) from [] (warn_slowpath_null+0x18/0x20) [ 155.190556] [] (warn_slowpath_null) from [] (module_put+0x54/0x5c) [ 155.190581] [] (module_put) from [] (deactivate_locked_super+0x4c/0x64) [ 155.190598] [] (deactivate_locked_super) from [] (cleanup_mnt+0x4c/0x6c) [ 155.190614] [] (cleanup_mnt) from [] (task_work_run+0x8c/0xa4) [ 155.190628] [] (task_work_run) from [] (do_work_pending+0x98/0xac) [ 155.190642] [] (do_work_pending) from [] (work_pending+0xc/0x20) [ 155.190649] ---[ end trace c3d57fca714062a3 ]--- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ That, BTW. is too a result of changing f_op dynamically. They will be changed from having no owner to having gadgetfs as owner. Which fails since commit e513cc1c (3.19). Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
Am 07.03.2015 um 12:23 schrieb Alexander Holler: In detail it's only usable once (3.19). When unmounting it throws one or two warnings because of too much puts (kernel/module.c:963, see below). The result is that a subsequent mount afterwards fails because the old instance is still busy (Resource temporarily unavailable). I haven't looked deeper into that problem up to now. Regards, Alexander Holler [ 151.425966] gadgetfs: USB Gadget filesystem, version 24 Aug 2004 [ 151.479616] gadgetfs: bound to musb-hdrc driver [ 151.781095] gadgetfs: connected [ 151.785871] gadgetfs: disconnected [ 151.864187] gadgetfs: connected [ 151.873258] gadgetfs: configuration #1 [ 151.962240] musb_g_ep0_irq 804: SETUP packet len 0 != 8 ? [ 155.065272] input: USB Keyboard as /devices/platform/ocp/4740.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.0/0003:04D9:1603.0003/input/input2 [ 155.119746] hid-generic 0003:04D9:1603.0003: input,hidraw0: USB HID v1.10 Keyboard [ USB Keyboard] on usb-musb-hdrc.1.auto-1/input0 [ 155.138194] input: USB Keyboard as /devices/platform/ocp/4740.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.1/0003:04D9:1603.0004/input/input3 [ 155.189783] hid-generic 0003:04D9:1603.0004: input,hidraw1: USB HID v1.10 Device [ USB Keyboard] on usb-musb-hdrc.1.auto-1/input1 [ 155.190002] gadgetfs: disconnected [ 155.190352] [ cut here ] [ 155.190389] WARNING: CPU: 0 PID: 1290 at kernel/module.c:963 module_put+0x54/0x5c() [ 155.190396] Modules linked in: gadgetfs usb_f_ecm usb_f_rndis u_ether libcomposite configfs ipv6 bnep bluetooth rfkill hid_generic tda998x tilcdc drm_kms_helper ti_cpsw ptp drm usbhid pps_core davinci_cpdma omap_mailbox cppi41 [last unloaded: g_ether] [ 155.190459] CPU: 0 PID: 1290 Comm: usb-mitm Not tainted 3.19.0-bbb-00080-g5e25c2a #246 [ 155.190466] Hardware name: Generic AM33XX (Flattened Device Tree) [ 155.190505] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 155.190527] [] (show_stack) from [] (warn_slowpath_common+0x80/0xa8) [ 155.190542] [] (warn_slowpath_common) from [] (warn_slowpath_null+0x18/0x20) [ 155.190556] [] (warn_slowpath_null) from [] (module_put+0x54/0x5c) [ 155.190581] [] (module_put) from [] (deactivate_locked_super+0x4c/0x64) [ 155.190598] [] (deactivate_locked_super) from [] (cleanup_mnt+0x4c/0x6c) [ 155.190614] [] (cleanup_mnt) from [] (task_work_run+0x8c/0xa4) [ 155.190628] [] (task_work_run) from [] (do_work_pending+0x98/0xac) [ 155.190642] [] (do_work_pending) from [] (work_pending+0xc/0x20) [ 155.190649] ---[ end trace c3d57fca714062a3 ]--- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ That, BTW. is too a result of changing f_op dynamically. They will be changed from having no owner to having gadgetfs as owner. Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sun, Mar 08, 2015 at 02:35:25PM -0400, Alan Stern wrote: > On Sun, 8 Mar 2015, Al Viro wrote: > > > On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote: > > > On Sat, 7 Mar 2015, Alexander Holler wrote: > > > > > > > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > > > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > > > > > > > >> check to see what those values actually were. It's easy enough to > > > > >> fix > > > > >> up, though; revised patch below. > > > > > > > > > > Thanks, in contrast to the patch from Al Viro that one applies. > > > > > > > > And as I've just tested it, it isn't complete. ep_config_operations will > > > > be switched to ep_io_operations and suffers under the same problem of > > > > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 > > > > (3.16). > > > > > > Of course. I stated in the email accompanying the original version of > > > this patch that it contained some corrections that Al's patch left out. > > > You have to use the two of them together to fix all the issues. > > > > FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your > > s-o-b on the ep0 part? See 2b13438 in vfs.git... > > Certainly. > > Signed-off-by: Alan Stern Just to make sure people know I'm okay with you taking the patch: Acked-by: Felipe Balbi -- balbi signature.asc Description: Digital signature
Re: gadgetfs broken since 7f7f25e8
On Sun, Mar 08, 2015 at 02:35:25PM -0400, Alan Stern wrote: > > FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your > > s-o-b on the ep0 part? See 2b13438 in vfs.git... > > Certainly. > > Signed-off-by: Alan Stern Amended and pushed... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sun, 8 Mar 2015, Al Viro wrote: > On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote: > > On Sat, 7 Mar 2015, Alexander Holler wrote: > > > > > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > > > > > >> check to see what those values actually were. It's easy enough to fix > > > >> up, though; revised patch below. > > > > > > > > Thanks, in contrast to the patch from Al Viro that one applies. > > > > > > And as I've just tested it, it isn't complete. ep_config_operations will > > > be switched to ep_io_operations and suffers under the same problem of > > > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). > > > > Of course. I stated in the email accompanying the original version of > > this patch that it contained some corrections that Al's patch left out. > > You have to use the two of them together to fix all the issues. > > FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your > s-o-b on the ep0 part? See 2b13438 in vfs.git... Certainly. Signed-off-by: Alan Stern Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sat, Mar 07, 2015 at 04:08:49PM -0500, Alan Stern wrote: > On Sat, 7 Mar 2015, Alexander Holler wrote: > > > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > > > >> check to see what those values actually were. It's easy enough to fix > > >> up, though; revised patch below. > > > > > > Thanks, in contrast to the patch from Al Viro that one applies. > > > > And as I've just tested it, it isn't complete. ep_config_operations will > > be switched to ep_io_operations and suffers under the same problem of > > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). > > Of course. I stated in the email accompanying the original version of > this patch that it contained some corrections that Al's patch left out. > You have to use the two of them together to fix all the issues. FWIW, I've pushed those two fixes in vfs.git#gadget; could I have your s-o-b on the ep0 part? See 2b13438 in vfs.git... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sat, 7 Mar 2015, Alexander Holler wrote: > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > >> check to see what those values actually were. It's easy enough to fix > >> up, though; revised patch below. > > > > Thanks, in contrast to the patch from Al Viro that one applies. > > And as I've just tested it, it isn't complete. ep_config_operations will > be switched to ep_io_operations and suffers under the same problem of > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). Of course. I stated in the email accompanying the original version of this patch that it contained some corrections that Al's patch left out. You have to use the two of them together to fix all the issues. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
Am 07.03.2015 um 21:51 schrieb Al Viro: > On Sat, Mar 07, 2015 at 09:03:51PM +0100, Alexander Holler wrote: >> Am 07.03.2015 um 12:23 schrieb Alexander Holler: >>> Am 04.03.2015 um 16:31 schrieb Alan Stern: >>> check to see what those values actually were. It's easy enough to fix up, though; revised patch below. >>> >>> Thanks, in contrast to the patch from Al Viro that one applies. > > Translation: it applies to mainline as well as to vfs.git#gadget + > ep_config_operations patch it's incremental to. > >> And as I've just tested it, it isn't complete. ep_config_operations will >> be switched to ep_io_operations and suffers under the same problem of >> not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). > > Incremental patch does not deal with the problem handled by the patch > it is incremental to, film at 11... > > FWIW, patches in vfs.git#gadget are fixing fairly obvious bugs - leaks and > use-after-free. > Feel free to test it yourself. Thansk for all the fish. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Sat, Mar 07, 2015 at 09:03:51PM +0100, Alexander Holler wrote: > Am 07.03.2015 um 12:23 schrieb Alexander Holler: > > Am 04.03.2015 um 16:31 schrieb Alan Stern: > > > >> check to see what those values actually were. It's easy enough to fix > >> up, though; revised patch below. > > > > Thanks, in contrast to the patch from Al Viro that one applies. Translation: it applies to mainline as well as to vfs.git#gadget + ep_config_operations patch it's incremental to. > And as I've just tested it, it isn't complete. ep_config_operations will > be switched to ep_io_operations and suffers under the same problem of > not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). Incremental patch does not deal with the problem handled by the patch it is incremental to, film at 11... FWIW, patches in vfs.git#gadget are fixing fairly obvious bugs - leaks and use-after-free. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
Am 07.03.2015 um 12:23 schrieb Alexander Holler: > Am 04.03.2015 um 16:31 schrieb Alan Stern: > >> check to see what those values actually were. It's easy enough to fix >> up, though; revised patch below. > > Thanks, in contrast to the patch from Al Viro that one applies. And as I've just tested it, it isn't complete. ep_config_operations will be switched to ep_io_operations and suffers under the same problem of not having initially (aio_)read/(aio_)write since commit 7f7f25e8 (3.16). Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
Am 04.03.2015 um 16:31 schrieb Alan Stern: check to see what those values actually were. It's easy enough to fix up, though; revised patch below. Thanks, in contrast to the patch from Al Viro that one applies. I wonder if the patches in the (vfs-)tree he has used as base might fix some more problems of gadgetfs I've discovered since I can use it. In detail it's only usable once (3.19). When unmounting it throws one or two warnings because of too much puts (kernel/module.c:963, see below). The result is that a subsequent mount afterwards fails because the old instance is still busy (Resource temporarily unavailable). I haven't looked deeper into that problem up to now. Regards, Alexander Holler [ 151.425966] gadgetfs: USB Gadget filesystem, version 24 Aug 2004 [ 151.479616] gadgetfs: bound to musb-hdrc driver [ 151.781095] gadgetfs: connected [ 151.785871] gadgetfs: disconnected [ 151.864187] gadgetfs: connected [ 151.873258] gadgetfs: configuration #1 [ 151.962240] musb_g_ep0_irq 804: SETUP packet len 0 != 8 ? [ 155.065272] input: USB Keyboard as /devices/platform/ocp/4740.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.0/0003:04D9:1603.0003/input/input2 [ 155.119746] hid-generic 0003:04D9:1603.0003: input,hidraw0: USB HID v1.10 Keyboard [ USB Keyboard] on usb-musb-hdrc.1.auto-1/input0 [ 155.138194] input: USB Keyboard as /devices/platform/ocp/4740.usb/47401c00.usb/musb-hdrc.1.auto/usb2/2-1/2-1:1.1/0003:04D9:1603.0004/input/input3 [ 155.189783] hid-generic 0003:04D9:1603.0004: input,hidraw1: USB HID v1.10 Device [ USB Keyboard] on usb-musb-hdrc.1.auto-1/input1 [ 155.190002] gadgetfs: disconnected [ 155.190352] [ cut here ] [ 155.190389] WARNING: CPU: 0 PID: 1290 at kernel/module.c:963 module_put+0x54/0x5c() [ 155.190396] Modules linked in: gadgetfs usb_f_ecm usb_f_rndis u_ether libcomposite configfs ipv6 bnep bluetooth rfkill hid_generic tda998x tilcdc drm_kms_helper ti_cpsw ptp drm usbhid pps_core davinci_cpdma omap_mailbox cppi41 [last unloaded: g_ether] [ 155.190459] CPU: 0 PID: 1290 Comm: usb-mitm Not tainted 3.19.0-bbb-00080-g5e25c2a #246 [ 155.190466] Hardware name: Generic AM33XX (Flattened Device Tree) [ 155.190505] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 155.190527] [] (show_stack) from [] (warn_slowpath_common+0x80/0xa8) [ 155.190542] [] (warn_slowpath_common) from [] (warn_slowpath_null+0x18/0x20) [ 155.190556] [] (warn_slowpath_null) from [] (module_put+0x54/0x5c) [ 155.190581] [] (module_put) from [] (deactivate_locked_super+0x4c/0x64) [ 155.190598] [] (deactivate_locked_super) from [] (cleanup_mnt+0x4c/0x6c) [ 155.190614] [] (cleanup_mnt) from [] (task_work_run+0x8c/0xa4) [ 155.190628] [] (task_work_run) from [] (do_work_pending+0x98/0xac) [ 155.190642] [] (do_work_pending) from [] (work_pending+0xc/0x20) [ 155.190649] ---[ end trace c3d57fca714062a3 ]--- -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Tue, 3 Mar 2015, Al Viro wrote: > On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote: > > On Tue, 3 Mar 2015, Al Viro wrote: > > > > > Looking at that thing again... why do they need to be dummy? After all, > > > those methods start with get_ready_ep(), which will fail unless we have > > > ->state == STATE_EP_ENABLED. So they'd be failing just fine until that > > > first write() anyway. Let's do the following: > > > > In addition to the changes you made, it looks like you will need the > > following or something similar (also untested). I'm not sure if this > > is race-free, but it's better than before. > > Right, ep0 has the same kind of problem... > > > > @@ -1240,6 +1241,10 @@ static int > > ep0_fasync (int f, struct file *fd, int on) > > { > > struct dev_data *dev = fd->private_data; > > + > > + if (dev->state <= STATE_DEV_OPENED) > > + return -ENODEV; > > + > > Er... What is protecting dev->state here? Matter of fact, what's the > point of that check at all? Right now you have .fasync = ep0_fasync > both in ep0_io_operations and in dev_init_operations, so your delta > changes the existing semantics... That was a mistake; it shouldn't have been in the patch. I wrote this rather hastily without doing a careful check at the end. > On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote: > > @@ -1279,6 +1284,9 @@ ep0_poll (struct file *fd, poll_table *w > > struct dev_data *dev = fd->private_data; > > int mask = 0; > > > > + if (dev->state <= STATE_DEV_OPENED) > > + return -ENODEV; > > + > > poll_wait(fd, &dev->wait, wait); > > > > spin_lock_irq (&dev->lock); > > That, BTW, is wrong. ->poll() does *not* return negatives - to imitate > "we don't have ->poll()" we need it to return DEFAULT_POLLMASK. Yes, this should return whatever the default value is when the ->poll method pointer isn't set. Likewise for the ->read method. I didn't check to see what those values actually were. It's easy enough to fix up, though; revised patch below. Alan Stern Index: usb-3.19/drivers/usb/gadget/legacy/inode.c === --- usb-3.19.orig/drivers/usb/gadget/legacy/inode.c +++ usb-3.19/drivers/usb/gadget/legacy/inode.c @@ -987,6 +987,10 @@ ep0_read (struct file *fd, char __user * enum ep0_state state; spin_lock_irq (&dev->lock); + if (dev->state <= STATE_DEV_OPENED) { + retval = -EINVAL; + goto done; + } /* report fd mode change before acting on it */ if (dev->setup_abort) { @@ -1185,8 +1189,6 @@ ep0_write (struct file *fd, const char _ struct dev_data *dev = fd->private_data; ssize_t retval = -ESRCH; - spin_lock_irq (&dev->lock); - /* report fd mode change before acting on it */ if (dev->setup_abort) { dev->setup_abort = 0; @@ -1232,7 +1234,6 @@ ep0_write (struct file *fd, const char _ } else DBG (dev, "fail %s, state %d\n", __func__, dev->state); - spin_unlock_irq (&dev->lock); return retval; } @@ -1279,6 +1280,9 @@ ep0_poll (struct file *fd, poll_table *w struct dev_data *dev = fd->private_data; int mask = 0; + if (dev->state <= STATE_DEV_OPENED) + return DEFAULT_POLLMASK; + poll_wait(fd, &dev->wait, wait); spin_lock_irq (&dev->lock); @@ -1314,19 +1318,6 @@ static long dev_ioctl (struct file *fd, return ret; } -/* used after device configuration */ -static const struct file_operations ep0_io_operations = { - .owner =THIS_MODULE, - .llseek = no_llseek, - - .read = ep0_read, - .write =ep0_write, - .fasync = ep0_fasync, - .poll = ep0_poll, - .unlocked_ioctl = dev_ioctl, - .release = dev_release, -}; - /*--*/ /* The in-kernel gadget driver handles most ep0 issues, in particular @@ -1850,6 +1841,14 @@ dev_config (struct file *fd, const char u32 tag; char*kbuf; + spin_lock_irq(&dev->lock); + if (dev->state > STATE_DEV_OPENED) { + value = ep0_write(fd, buf, len, ptr); + spin_unlock_irq(&dev->lock); + return value; + } + spin_unlock_irq(&dev->lock); + if (len < (USB_DT_CONFIG_SIZE + USB_DT_DEVICE_SIZE + 4)) return -EINVAL; @@ -1923,7 +1922,6 @@ dev_config (struct file *fd, const char * on, they can work ... except in cleanup paths that * kick in after the ep0 descriptor is closed. */ - fd->f_op = &ep0_io_operations; value = l
Re: gadgetfs broken since 7f7f25e8
On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote: > @@ -1279,6 +1284,9 @@ ep0_poll (struct file *fd, poll_table *w > struct dev_data *dev = fd->private_data; > int mask = 0; > > + if (dev->state <= STATE_DEV_OPENED) > + return -ENODEV; > + > poll_wait(fd, &dev->wait, wait); > > spin_lock_irq (&dev->lock); That, BTW, is wrong. ->poll() does *not* return negatives - to imitate "we don't have ->poll()" we need it to return DEFAULT_POLLMASK. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Tue, Mar 03, 2015 at 10:47:14AM -0500, Alan Stern wrote: > On Tue, 3 Mar 2015, Al Viro wrote: > > > Looking at that thing again... why do they need to be dummy? After all, > > those methods start with get_ready_ep(), which will fail unless we have > > ->state == STATE_EP_ENABLED. So they'd be failing just fine until that > > first write() anyway. Let's do the following: > > In addition to the changes you made, it looks like you will need the > following or something similar (also untested). I'm not sure if this > is race-free, but it's better than before. Right, ep0 has the same kind of problem... > @@ -1240,6 +1241,10 @@ static int > ep0_fasync (int f, struct file *fd, int on) > { > struct dev_data *dev = fd->private_data; > + > + if (dev->state <= STATE_DEV_OPENED) > + return -ENODEV; > + Er... What is protecting dev->state here? Matter of fact, what's the point of that check at all? Right now you have .fasync = ep0_fasync both in ep0_io_operations and in dev_init_operations, so your delta changes the existing semantics... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Tue, 3 Mar 2015, Al Viro wrote: > Looking at that thing again... why do they need to be dummy? After all, > those methods start with get_ready_ep(), which will fail unless we have > ->state == STATE_EP_ENABLED. So they'd be failing just fine until that > first write() anyway. Let's do the following: In addition to the changes you made, it looks like you will need the following or something similar (also untested). I'm not sure if this is race-free, but it's better than before. Alan Stern Index: usb-3.19/drivers/usb/gadget/legacy/inode.c === --- usb-3.19.orig/drivers/usb/gadget/legacy/inode.c +++ usb-3.19/drivers/usb/gadget/legacy/inode.c @@ -987,6 +987,10 @@ ep0_read (struct file *fd, char __user * enum ep0_state state; spin_lock_irq (&dev->lock); + if (dev->state <= STATE_DEV_OPENED) { + retval = -ENODEV; + goto done; + } /* report fd mode change before acting on it */ if (dev->setup_abort) { @@ -1185,8 +1189,6 @@ ep0_write (struct file *fd, const char _ struct dev_data *dev = fd->private_data; ssize_t retval = -ESRCH; - spin_lock_irq (&dev->lock); - /* report fd mode change before acting on it */ if (dev->setup_abort) { dev->setup_abort = 0; @@ -1232,7 +1234,6 @@ ep0_write (struct file *fd, const char _ } else DBG (dev, "fail %s, state %d\n", __func__, dev->state); - spin_unlock_irq (&dev->lock); return retval; } @@ -1240,6 +1241,10 @@ static int ep0_fasync (int f, struct file *fd, int on) { struct dev_data *dev = fd->private_data; + + if (dev->state <= STATE_DEV_OPENED) + return -ENODEV; + // caller must F_SETOWN before signal delivery happens VDEBUG (dev, "%s %s\n", __func__, on ? "on" : "off"); return fasync_helper (f, fd, on, &dev->fasync); @@ -1279,6 +1284,9 @@ ep0_poll (struct file *fd, poll_table *w struct dev_data *dev = fd->private_data; int mask = 0; + if (dev->state <= STATE_DEV_OPENED) + return -ENODEV; + poll_wait(fd, &dev->wait, wait); spin_lock_irq (&dev->lock); @@ -1314,19 +1322,6 @@ static long dev_ioctl (struct file *fd, return ret; } -/* used after device configuration */ -static const struct file_operations ep0_io_operations = { - .owner =THIS_MODULE, - .llseek = no_llseek, - - .read = ep0_read, - .write =ep0_write, - .fasync = ep0_fasync, - .poll = ep0_poll, - .unlocked_ioctl = dev_ioctl, - .release = dev_release, -}; - /*--*/ /* The in-kernel gadget driver handles most ep0 issues, in particular @@ -1850,6 +1845,14 @@ dev_config (struct file *fd, const char u32 tag; char*kbuf; + spin_lock_irq(&dev->lock); + if (dev->state > STATE_DEV_OPENED) { + value = ep0_write(fd, buf, len, ptr); + spin_unlock_irq(&dev->lock); + return value; + } + spin_unlock_irq(&dev->lock); + if (len < (USB_DT_CONFIG_SIZE + USB_DT_DEVICE_SIZE + 4)) return -EINVAL; @@ -1923,7 +1926,6 @@ dev_config (struct file *fd, const char * on, they can work ... except in cleanup paths that * kick in after the ep0 descriptor is closed. */ - fd->f_op = &ep0_io_operations; value = len; } return value; @@ -1954,12 +1956,14 @@ dev_open (struct inode *inode, struct fi return value; } -static const struct file_operations dev_init_operations = { +static const struct file_operations ep0_operations = { .llseek = no_llseek, .open = dev_open, + .read = ep0_read, .write =dev_config, .fasync = ep0_fasync, + .poll = ep0_poll, .unlocked_ioctl = dev_ioctl, .release = dev_release, }; @@ -2075,7 +2079,7 @@ gadgetfs_fill_super (struct super_block goto Enomem; dev->sb = sb; - dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &dev_init_operations); + dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations); if (!dev->dentry) { put_dev(dev); goto Enomem; -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Mon, Mar 02, 2015 at 02:02:56PM +0100, Alexander Holler wrote: > >I exactly did what you've assumed, I've just fixed f_mode but not the > >already existing races which I haven't introduced. So I was right in not > >sending a patch as would have been blamed for not rewriting everything > >as so often. > > Another quick solution would be to just add some dummy ops for > read/write to those file_operations which are missing it which are > only returning -EINVAL or some other error which might make sense. Looking at that thing again... why do they need to be dummy? After all, those methods start with get_ready_ep(), which will fail unless we have ->state == STATE_EP_ENABLED. So they'd be failing just fine until that first write() anyway. Let's do the following: * get_ready_ep() gets a new argument - true when called from ep_write_iter(), false otherwise. * make it quiet when it finds STATE_EP_READY (no printk, that is; the case won't be impossible after that change). * when that new argument is true, treat STATE_EP_READY the same way as STATE_EP_ENABLED (i.e. return zero and do not unlock). * in ep_write_iter(), after success of get_ready_ep() turn if (!usb_endpoint_dir_in(&epdata->desc)) { into if (epdata->state == STATE_EP_ENABLED && !usb_endpoint_dir_in(&epdata->desc)) { - that logics only applies after config. * have ep_config() take kernel-side buffer (i.e. use memcpy() instead of copy_from_user() in there) and in the "let's call ep_io or ep_aio" (again, in ep_write_iter()) add "... or ep_config() in case it's not configured yet" IOW, how about the following (completely untested) patch on top of vfs.git#gadget? Comments? diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c index b825edc..c0e2532 100644 --- a/drivers/usb/gadget/legacy/inode.c +++ b/drivers/usb/gadget/legacy/inode.c @@ -74,6 +74,8 @@ MODULE_DESCRIPTION (DRIVER_DESC); MODULE_AUTHOR ("David Brownell"); MODULE_LICENSE ("GPL"); +static int ep_open(struct inode *, struct file *); + /*--*/ @@ -283,14 +285,15 @@ static void epio_complete (struct usb_ep *ep, struct usb_request *req) * still need dev->lock to use epdata->ep. */ static int -get_ready_ep (unsigned f_flags, struct ep_data *epdata) +get_ready_ep (unsigned f_flags, struct ep_data *epdata, bool is_write) { int val; if (f_flags & O_NONBLOCK) { if (!mutex_trylock(&epdata->lock)) goto nonblock; - if (epdata->state != STATE_EP_ENABLED) { + if (epdata->state != STATE_EP_ENABLED && + (!is_write || epdata->state != STATE_EP_READY)) { mutex_unlock(&epdata->lock); nonblock: val = -EAGAIN; @@ -305,18 +308,20 @@ nonblock: switch (epdata->state) { case STATE_EP_ENABLED: + return 0; + case STATE_EP_READY:/* not configured yet */ + if (is_write) + return 0; + // FALLTHRU + case STATE_EP_UNBOUND: /* clean disconnect */ break; // case STATE_EP_DISABLED: /* "can't happen" */ - // case STATE_EP_READY: /* "can't happen" */ default:/* error! */ pr_debug ("%s: ep %p not available, state %d\n", shortname, epdata, epdata->state); - // FALLTHROUGH - case STATE_EP_UNBOUND: /* clean disconnect */ - val = -ENODEV; - mutex_unlock(&epdata->lock); } - return val; + mutex_unlock(&epdata->lock); + return -ENODEV; } static ssize_t @@ -390,7 +395,7 @@ static long ep_ioctl(struct file *fd, unsigned code, unsigned long value) struct ep_data *data = fd->private_data; int status; - if ((status = get_ready_ep (fd->f_flags, data)) < 0) + if ((status = get_ready_ep (fd->f_flags, data, false)) < 0) return status; spin_lock_irq (&data->dev->lock); @@ -572,7 +577,7 @@ ep_read_iter(struct kiocb *iocb, struct iov_iter *to) ssize_t value; char *buf; - if ((value = get_ready_ep(file->f_flags, epdata)) < 0) + if ((value = get_ready_ep(file->f_flags, epdata, false)) < 0) return value; /* halt any endpoint by doing a "wrong direction" i/o call */ @@ -620,20 +625,25 @@ fail: return value; } +static ssize_t ep_config(struct ep_data *, const char *, size_t); + static ssize_t ep_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct file *file = iocb->ki_filp; struct ep_data *epdata = file->private_data; size_t len = iov_iter_count(from); +
Re: gadgetfs broken since 7f7f25e8
Am 02.03.2015 um 14:02 schrieb Alexander Holler: Am 02.03.2015 um 12:39 schrieb Alexander Holler: Am 02.03.2015 um 11:20 schrieb Al Viro: On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote: On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler wrote: Hello. Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke dynamic changing of file_operations->[read|write]. At least gadgetfs is a victim. Just for your amusement and as an example: This bug lead to me to examine and search bugs in the userland piece I've tried to use and ended up in around === aholler@laptopahbt ~/Source/USBProxy.git/src $ PAGER= git diff 7d2506648e3404bf7070bae6ab8da4a702ed093c --stat doc/gadgetfs_kernel_above_3.15.patch | 50 +++ src/Plugins/Hosts/GadgetFS_helpers.c | 4 ++-- src/Plugins/Hosts/HostProxy_GadgetFS.cpp | 12 src/debian/header-check.c| 1 - src/lib/CMakeLists.txt | 2 -- src/lib/ConfigParser.cpp | 9 +++-- src/lib/ConfigParser.h | 2 +- src/lib/FDInfo.c | 2 +- src/lib/HaltSignal.c | 54 --- src/lib/HaltSignal.h | 19 -- src/lib/Injector.cpp | 23 +- src/lib/Injector.h | 11 +++ src/lib/Manager.cpp | 122 +++--- src/lib/Manager.h| 15 +++--- src/lib/PluginManager.cpp| 47 +--- src/lib/Proxy.h | 12 src/lib/RelayReader.cpp | 39 - src/lib/RelayReader.h| 9 ++--- src/lib/RelayWriter.cpp | 69 src/lib/RelayWriter.h| 8 +--- src/tools/usb-mitm.cpp | 2 -- 21 files changed, 223 insertions(+), 289 deletions(-) === without counting at least a dozen patches I did on that userland piece before those which are counted in the above stat. All in order to find the bug. So, you can see, I've already spend some hours before I've dived into the kernel to search for the bug. Of course, the problem in the kernel is innocent for all the problems I've found in userland which lead me to the assumption that the -EINVAL returned from a read() after a poll() is because of some problem in userspace (like memory or stack corruption). Just in case someone thinks I'm lazy because I don't want to rewrite gadgetfs and deal with kernel maintainers. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
Am 02.03.2015 um 12:39 schrieb Alexander Holler: Am 02.03.2015 um 11:20 schrieb Al Viro: On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote: On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler wrote: Hello. Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke dynamic changing of file_operations->[read|write]. At least gadgetfs is a victim. Feel free to ask me off-list for a patch as I don't want to end up in annoying discussions on Linux kernel lists anymore. Alexander Holler CC'ing Al. I know. FWIW, gadgetfs is one of the very few places that tried to pull that crap off and it had always been seriously racy. I've posted a partial analysis about a month ago (<20150204190645.gj29...@zeniv.linux.org.uk>). If Alexander (or anybody else) has a patch that really fixes that thing, I would certainly like to see it. If not, I'll try to cook something, but I'm not very familiar with that code. I really hope that this patch isn't "modify ->f_mode to match ->f_op change" - that's too racy. We'll obviously need to fix the userland-visible breakage in that one, but that's not the way to go... I exactly did what you've assumed, I've just fixed f_mode but not the already existing races which I haven't introduced. So I was right in not sending a patch as would have been blamed for not rewriting everything as so often. Another quick solution would be to just add some dummy ops for read/write to those file_operations which are missing it which are only returning -EINVAL or some other error which might make sense. But that still won't fix any existing race occuring while changing the the ops. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
Am 02.03.2015 um 11:20 schrieb Al Viro: On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote: On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler wrote: Hello. Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke dynamic changing of file_operations->[read|write]. At least gadgetfs is a victim. Feel free to ask me off-list for a patch as I don't want to end up in annoying discussions on Linux kernel lists anymore. Alexander Holler CC'ing Al. I know. FWIW, gadgetfs is one of the very few places that tried to pull that crap off and it had always been seriously racy. I've posted a partial analysis about a month ago (<20150204190645.gj29...@zeniv.linux.org.uk>). If Alexander (or anybody else) has a patch that really fixes that thing, I would certainly like to see it. If not, I'll try to cook something, but I'm not very familiar with that code. I really hope that this patch isn't "modify ->f_mode to match ->f_op change" - that's too racy. We'll obviously need to fix the userland-visible breakage in that one, but that's not the way to go... I exactly did what you've assumed, I've just fixed f_mode but not the already existing races which I haven't introduced. So I was right in not sending a patch as would have been blamed for not rewriting everything as so often. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Mon, Mar 02, 2015 at 10:13:27AM +0100, Richard Weinberger wrote: > On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler wrote: > > Hello. > > > > Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke > > dynamic changing of file_operations->[read|write]. > > > > At least gadgetfs is a victim. > > > > Feel free to ask me off-list for a patch as I don't want to end up in > > annoying discussions on Linux kernel lists anymore. > > > > Alexander Holler > > CC'ing Al. I know. FWIW, gadgetfs is one of the very few places that tried to pull that crap off and it had always been seriously racy. I've posted a partial analysis about a month ago (<20150204190645.gj29...@zeniv.linux.org.uk>). If Alexander (or anybody else) has a patch that really fixes that thing, I would certainly like to see it. If not, I'll try to cook something, but I'm not very familiar with that code. I really hope that this patch isn't "modify ->f_mode to match ->f_op change" - that's too racy. We'll obviously need to fix the userland-visible breakage in that one, but that's not the way to go... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: gadgetfs broken since 7f7f25e8
On Mon, Mar 2, 2015 at 9:28 AM, Alexander Holler wrote: > Hello. > > Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke > dynamic changing of file_operations->[read|write]. > > At least gadgetfs is a victim. > > Feel free to ask me off-list for a patch as I don't want to end up in > annoying discussions on Linux kernel lists anymore. > > Alexander Holler CC'ing Al. -- Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
gadgetfs broken since 7f7f25e8
Hello. Commit 7f7f25e82d54870df24d415a7007fbd327da027b (introduced with 3.16) broke dynamic changing of file_operations->[read|write]. At least gadgetfs is a victim. Feel free to ask me off-list for a patch as I don't want to end up in annoying discussions on Linux kernel lists anymore. Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html