Re: [Qemu-devel] [V3 PATCH 7/8] virtio-9p: Move file post creation changes to none security model

[Stefan Hajnoczi]

On Thu, Jan 20, 2011 at 2:48 PM, Daniel P. Berrange <address@hidden> wrote:
> On Thu, Jan 20, 2011 at 08:11:27PM +0530, M. Mohan Kumar wrote:
>> On Thursday 20 January 2011 2:29:54 pm Stefan Hajnoczi wrote:
>> > On Tue, Jan 18, 2011 at 01:54:16PM +0530, M. Mohan Kumar wrote:
>>
>> > > -    if (lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid) < 0) {
>> > > -        /*
>> > > -         * If we fail to change ownership and if we are
>> > > -         * using security model none. Ignore the error
>> > > -         */
>> > > -        if (fs_ctx->fs_sm != SM_NONE) {
>> > > -            return -1;
>> > > -        }
>> > > -    }
>> > > +    retval = lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
>> > >
>> > >      return 0;
>> > >
>> > >  }
>> >
>> > retval is unused.
>> >
>>
>> That was used to disable the warning message "error: ignoring return value of
>> ‘lchown’, declared with attribute warn_unused_result"
>>
>> Otherwise I have to use
>> if (lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid)) {
>>       ;
>> }
>>
>> > Can multiple virtio-9p requests execute at a time?  chmod() and lchown()
>> > after creation is a race condition if other requests can execute
>> > concurrently.
>> >
>>
>> We can't implement file creation with requested user credentials and 
>> permission
>> bits in the none security model atomically. Its expected behaviour only
>
> Well you could do the nasty trick of forking a child process
> and doing setuid/gid in that and then creating the file before
> letting the parent continue.
>
>  if ((pid = fork()) == 0) {
>     setuid(fc_uid);
>     setgid(fc_gid);
>     fd =open("foo", O_CREAT);
>     close(fd);
>  } else {
>     waitpid(pid);
>  }
>
> This kind of approach is in fact required if you want to
> be able to create files with a special uid/gid on a root
> squashing NFS server, because otherwise your QEMU running
> as root will have its files squashed to 'nobody' when initially
> created, and lchown will fail with EPERM.  You might decide
> that root squashing NFS is too painful to care about supporting
> though :-)

I was thinking about this approach and it's similar to the chroot
helper process, but this time you have a helper process that does
umask/setgid/setuid as necessary.  Performance will be bad but there's
really no way around this.

Either implement something that works 90% of the time only but runs a
bit faster or implement something that works all the time but runs
slow.  It's not a nice trade-off.

Stefan