linux-kernel - Re: [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.64.0802281245580.7660@alien.or.mcafeemobile.com>
Date:	Thu, 28 Feb 2008 12:52:11 -0800 (PST)
From:	Davide Libenzi <davidel@...ilserver.org>
To:	Roland Dreier <rdreier@...co.com>
cc:	Christoph Hellwig <hch@....de>, linux-fsdevel@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Avi Kivity <avi@...ranet.com>, kvm-devel@...ts.sourceforge.net,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@....linux.org.uk>
Subject: Re: [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from
 anon_inode_getfd()

On Thu, 28 Feb 2008, Roland Dreier wrote:

>  > If we let the caller call fd_install(), then it may be messed up WRT 
>  > cleanup (fd, file, inode).
> 
> Yes, that is a tiny bit tricky (need to call put_unused_fd() if you
> don't install the fd).
> 
>  > How about removing the inode pointer handout altogether, and *doing* 
>  > fd_install() inside anon_inode_getfd() like:
>  > 
>  > 	if (pfile != NULL) {
>  > 		get_file(file);
>  > 		*pfile = file;
>  > 	}
>  > 	fd_install(fd, file);
>  > 
>  > In this way, if the caller want the file* back, he gets the reference 
>  > bumped before fd_install().
> 
> I think that may be a bit cleaner than Al's approach, but it still
> leaves the same trap that create_vcpu_fd() falls into.  The current
> code is:
> 
> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
> {
> 	int fd, r;
> 	struct inode *inode;
> 	struct file *file;
> 
> 	r = anon_inode_getfd(&fd, &inode, &file,
> 			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
> 	if (r)
> 		return r;
> 	atomic_inc(&vcpu->kvm->filp->f_count);
> 	return fd;
> }
> 
> and with your proposal, the natural way to write that becomes:
> 
> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
> {
> 	int fd, r;
> 
> 	r = anon_inode_getfd(&fd, NULL,
> 			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
> 	if (r)
> 		return r;
> 	atomic_inc(&vcpu->kvm->filp->f_count);
> 	return fd;
> }

I don't know KVM code, but can't the "private_data" setup be completed 
before calling anon_inode_getfd()?
Or ...

static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
	int fd, r;

	get_file(vcpu->kvm->filp);
	r = anon_inode_getfd(&fd, NULL,
			     "kvm-vcpu", &kvm_vcpu_fops, vcpu);
	if (r) {
		fput(vcpu->kvm->filp);
		return r;
	}
	return fd;
}

Hmm...?



- Davide


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