Re: [PATCH 3/5] fs: remove ki_nbytes

[Al Viro <viro@ZenIV.linux.org.uk>]

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, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org.  For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>