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); ...
Il 03/06/2014 15:57, Eric Dumazet ha scritto:> 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 > c3059477fce2d956a0bb3e04357324780c5d8eebThat would make VHOST_SET_MEMORY as slow as before (even though once every few times).>>> 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.I agree. Paolo
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. -- MST
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