Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720928 > unrolled thread

Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce Al Viro <viro@ZenIV.linux.org.uk> - 2017-08-27 23:10 +0200
    Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce Paul Mackerras <paulus@ozlabs.org> - 2017-08-28 06:50 +0200
      Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce Al Viro <viro@ZenIV.linux.org.uk> - 2017-08-28 07:30 +0200
        Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce Paul Mackerras <paulus@ozlabs.org> - 2017-08-28 08:10 +0200
        Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce Michael Ellerman <mpe@ellerman.id.au> - 2017-08-28 13:40 +0200

#1720928 — Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce

On Wed, Aug 23, 2017 at 04:06:24PM +1000, Paul Mackerras wrote:

> It seems to me that it would be better to do the anon_inode_getfd()
> call before the kvm_get_kvm() call, and go to the fail label if it
> fails.

And what happens if another thread does close() on the (guessed) fd?

[toc] | [next] | [standalone]

#1721013

On Sun, Aug 27, 2017 at 10:02:20PM +0100, Al Viro wrote:
> On Wed, Aug 23, 2017 at 04:06:24PM +1000, Paul Mackerras wrote:
> 
> > It seems to me that it would be better to do the anon_inode_getfd()
> > call before the kvm_get_kvm() call, and go to the fail label if it
> > fails.
> 
> And what happens if another thread does close() on the (guessed) fd?

Chaos ensues, but mostly because we don't have proper mutual exclusion
on the modifications to the list.  I'll add a mutex_lock/unlock to
kvm_spapr_tce_release() and move the anon_inode_getfd() call inside
the mutex.

It looks like the other possible uses of the fd (mmap, and passing it
as a parameter to the KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl on a KVM
device fd) are safe.

Thanks,
Paul.

[toc] | [prev] | [next] | [standalone]

#1721031

On Mon, Aug 28, 2017 at 02:38:37PM +1000, Paul Mackerras wrote:
> On Sun, Aug 27, 2017 at 10:02:20PM +0100, Al Viro wrote:
> > On Wed, Aug 23, 2017 at 04:06:24PM +1000, Paul Mackerras wrote:
> > 
> > > It seems to me that it would be better to do the anon_inode_getfd()
> > > call before the kvm_get_kvm() call, and go to the fail label if it
> > > fails.
> > 
> > And what happens if another thread does close() on the (guessed) fd?
> 
> Chaos ensues, but mostly because we don't have proper mutual exclusion
> on the modifications to the list.  I'll add a mutex_lock/unlock to
> kvm_spapr_tce_release() and move the anon_inode_getfd() call inside
> the mutex.
> 
> It looks like the other possible uses of the fd (mmap, and passing it
> as a parameter to the KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl on a KVM
> device fd) are safe.

Frankly, it's a lot saner to have "no failure points past anon_inode_getfd()"
policy...

[toc] | [prev] | [next] | [standalone]

#1721040

On Mon, Aug 28, 2017 at 06:28:08AM +0100, Al Viro wrote:
> On Mon, Aug 28, 2017 at 02:38:37PM +1000, Paul Mackerras wrote:
> > On Sun, Aug 27, 2017 at 10:02:20PM +0100, Al Viro wrote:
> > > On Wed, Aug 23, 2017 at 04:06:24PM +1000, Paul Mackerras wrote:
> > > 
> > > > It seems to me that it would be better to do the anon_inode_getfd()
> > > > call before the kvm_get_kvm() call, and go to the fail label if it
> > > > fails.
> > > 
> > > And what happens if another thread does close() on the (guessed) fd?
> > 
> > Chaos ensues, but mostly because we don't have proper mutual exclusion
> > on the modifications to the list.  I'll add a mutex_lock/unlock to
> > kvm_spapr_tce_release() and move the anon_inode_getfd() call inside
> > the mutex.
> > 
> > It looks like the other possible uses of the fd (mmap, and passing it
> > as a parameter to the KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl on a KVM
> > device fd) are safe.
> 
> Frankly, it's a lot saner to have "no failure points past anon_inode_getfd()"
> policy...

Right.  In my latest patch, there are no failure points past
anon_inode_getfd().

Paul.

[toc] | [prev] | [next] | [standalone]

#1721544

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Mon, Aug 28, 2017 at 02:38:37PM +1000, Paul Mackerras wrote:
>> On Sun, Aug 27, 2017 at 10:02:20PM +0100, Al Viro wrote:
>> > On Wed, Aug 23, 2017 at 04:06:24PM +1000, Paul Mackerras wrote:
>> > 
>> > > It seems to me that it would be better to do the anon_inode_getfd()
>> > > call before the kvm_get_kvm() call, and go to the fail label if it
>> > > fails.
>> > 
>> > And what happens if another thread does close() on the (guessed) fd?
>> 
>> Chaos ensues, but mostly because we don't have proper mutual exclusion
>> on the modifications to the list.  I'll add a mutex_lock/unlock to
>> kvm_spapr_tce_release() and move the anon_inode_getfd() call inside
>> the mutex.
>> 
>> It looks like the other possible uses of the fd (mmap, and passing it
>> as a parameter to the KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl on a KVM
>> device fd) are safe.
>
> Frankly, it's a lot saner to have "no failure points past anon_inode_getfd()"
> policy...

Actually I thought that was a hard rule. But I don't see it documented
or mentioned anywhere so I'm not sure now why I thought that.

cheers

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web