Re: [PULL 2/2] vhost: replace rcu with mutex

[Michael S. Tsirkin]

On Wed, Jun 04, 2014 at 10:51:12PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 03, 2014 at 06:57:43AM -0700, Eric Dumazet wrote:
> > On Tue, 2014-06-03 at 14:48 +0200, Paolo Bonzini wrote:
> > > Il 02/06/2014 23:58, Eric Dumazet ha scritto:
> > > > This looks dubious
> > > >
> > > > What about using kfree_rcu() instead ?
> > > 
> > > It would lead to unbound allocation from userspace.
> > 
> > Look at how we did this in commit
> > c3059477fce2d956a0bb3e04357324780c5d8eeb
> > 
> > > 
> > > > translate_desc() still uses rcu_read_lock(), its not clear if the mutex
> > > > is really held.
> > > 
> > > Yes, vhost_get_vq_desc must be called with the vq mutex held.
> > > 
> > > The rcu_read_lock/unlock in translate_desc is unnecessary.
> > 
> > Yep, this is what I pointed out. This is not only necessary, but
> > confusing and might be incorrectly copy/pasted in the future.
> > 
> > This patch is a partial one and leaves confusion.
> > 
> > Some places uses the proper 
> > 
> > mp = rcu_dereference_protected(dev->memory,
> > 		 lockdep_is_held(&dev->mutex));
> > 
> > others use the now incorrect :
> > 
> > rcu_read_lock();
> > mp = rcu_dereference(dev->memory);
> > ...
> > 
> 
> I agree, working on a cleanup patch on top now.

OK I just posted two cleanups as patches on top that address this.
Eric, could you please confirm that you are fine with
cleanups being patches on top?
Bisect will be fine since this hack is ugly but technically correct.

Thanks a lot for pointing out the issues!

> -- 
> MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/