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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux