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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
