On Wed, Feb 04, 2015 at 01:17:30PM -0500, Alan Stern wrote: > On Wed, 4 Feb 2015, Al Viro wrote: > > > [USB folks Cc'd] > > Incidentally, Al, have you seen this email? > > http://marc.info/?l=linux-usb&m=142295011402339&w=2 > > I encouraged the writer to send in a patch but so far there has been no > reply. Yecchhh... Anything that changes ->f_op *after* return from ->open() is doing a nasty, nasty thing. What's to guarantee that any checks for NULL fields will stay valid, etc.? FWIW, in all the tree there are only 4 places where that would be happening; * i810_map_buffer() screwing around with having vm_mmap() done, only it wants its own thing called as ->mmap() (and a bit of extra data stashed for it). Racy as hell (if another thread calls mmap() on the same file, you'll get a nasty surprise). Driver's too old and brittle to touch, according to drm folks... * TTY hangup logics. Nasty (and might be broken around ->fasync()), but it's a very special case. * snd_card_disconnect(). Analogue of TTY hangup, actually; both are trying to do a form of revoke(). * this one. Note that you are not guaranteed that ep_config() won't be called more than once - two threads might race in write(2), with the loser getting through mutex_lock_interruptible(&data->lock); in ep_config() only after the winner has already gotten through write(), switched ->f_op, returned to userland and started doing read()/write()/etc. If nothing else, the contents of data->desc and data->hs_desc can be buggered by arbitrary data, no matter how bogus, right as the first thread is doing IO. > > [Context for USB people: The difference in question is what ep_read() does > > when it is called on write endpoint that isn't isochronous; > > You're talking about drivers/usb/gadget/legacy/inode.c, right? Yes. > > it halts the > > sucker and fails with EBADMSG, while ep_aio_read() handles all write endpoints > > as isochronous ones - fails with EINVAL; FWIW, I agree that it's probably > > a bug] > > It's not a bug; it's by design. That's how you halt an endpoint in > gadgetfs -- by doing a synchronous I/O call in the "wrong" direction. Yes, but you have readv() on single-element vector behave different from read(), which is surprising, to put it mildly. > > I plan to pull the fix for use-after-free in the beginning of that queue > > (in an easy to backport form) and then have ep_aio_read/ep_aio_write > > start doing the halt-related bits as in ep_read/ep_write. With that it's > > trivial to convert that sucker along the same lines as function/f_fs.c. > > I don't think there's any need to make the async routines do the > halt-related stuff. After all, it's silly for users to call an async > I/O routine to perform a synchronous action like halting an endpoint. Um... readv() is also going through ->aio_read(). I can tie that to sync vs. async, though - is_sync_kiocb() will do just that, if you are OK with having readv() act the same as read() in that respect. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
