On Wed, 4 Feb 2015, Al Viro wrote: > 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. Well, this one certainly can be fixed to avoid altering ->f_op, at the cost of adding an extra check at the start of each I/O operation. > > > 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(). Why does readv() do this but not read()? Wouldn't it make more sense to have all the read* calls use the same internal interface? > 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. I don't really care one way or the other. In fact, it doesn't matter if the same behavior applies to all the async calls as well as the sync calls -- I just doubt that anybody will ever use them. Alan Stern -- 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
