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

2011-01-20 Thread Stefan Hajnoczi
On Tue, Jan 18, 2011 at 01:54:16PM +0530, M. Mohan Kumar wrote:
 After creating a file object, its permission and ownership details are updated
 as per client's request for both passthrough and none security model. But with
 chrooted environment its not required for passthrough security model. Move all
 post file creation changes to none security model
 
 Signed-off-by: M. Mohan Kumar mo...@in.ibm.com
 ---
  hw/9pfs/virtio-9p-local.c |   19 ++-
  1 files changed, 6 insertions(+), 13 deletions(-)
 
 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index 08fd67f..d2e32e2 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -208,21 +208,14 @@ static int local_set_xattr(const char *path, FsCred 
 *credp)
  return 0;
  }
  
 -static int local_post_create_passthrough(FsContext *fs_ctx, const char *path,
 +static int local_post_create_none(FsContext *fs_ctx, const char *path,
  FsCred *credp)
  {
 +int retval;
  if (chmod(rpath(fs_ctx, path), credp-fc_mode  0)  0) {
  return -1;
  }
 -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.

Can multiple virtio-9p requests execute at a time?  chmod() and lchown()
after creation is a race condition if other requests can execute
concurrently.

Stefan



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

2011-01-20 Thread M. Mohan Kumar
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


M. Mohan Kumar



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

2011-01-20 Thread Daniel P. Berrange
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 :-)

Regards,
Daniel



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

2011-01-20 Thread Venkateswararao Jujjuri (JV)
On 1/20/2011 12:59 AM, Stefan Hajnoczi wrote:
 On Tue, Jan 18, 2011 at 01:54:16PM +0530, M. Mohan Kumar wrote:
 After creating a file object, its permission and ownership details are 
 updated
 as per client's request for both passthrough and none security model. But 
 with
 chrooted environment its not required for passthrough security model. Move 
 all
 post file creation changes to none security model

 Signed-off-by: M. Mohan Kumar mo...@in.ibm.com
 ---
  hw/9pfs/virtio-9p-local.c |   19 ++-
  1 files changed, 6 insertions(+), 13 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index 08fd67f..d2e32e2 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -208,21 +208,14 @@ static int local_set_xattr(const char *path, FsCred 
 *credp)
  return 0;
  }
  
 -static int local_post_create_passthrough(FsContext *fs_ctx, const char 
 *path,
 +static int local_post_create_none(FsContext *fs_ctx, const char *path,
  FsCred *credp)
  {
 +int retval;
  if (chmod(rpath(fs_ctx, path), credp-fc_mode  0)  0) {
  return -1;
  }
 -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.
 
 Can multiple virtio-9p requests execute at a time?  chmod() and lchown()
 after creation is a race condition if other requests can execute
 concurrently.

If some level of serialization is needed it will be done at the client/guest
inode level.
Are you worried about filesystem semantics? or do you see some corruption if 
they
get executed in parallel?

JV

 
 Stefan
 





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

2011-01-20 Thread Stefan Hajnoczi
On Thu, Jan 20, 2011 at 2:48 PM, Daniel P. Berrange berra...@redhat.com 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



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

2011-01-20 Thread Stefan Hajnoczi
On Thu, Jan 20, 2011 at 9:15 PM, Venkateswararao Jujjuri (JV)
jv...@linux.vnet.ibm.com wrote:
 On 1/20/2011 12:59 AM, Stefan Hajnoczi wrote:
 On Tue, Jan 18, 2011 at 01:54:16PM +0530, M. Mohan Kumar wrote:
 After creating a file object, its permission and ownership details are 
 updated
 as per client's request for both passthrough and none security model. But 
 with
 chrooted environment its not required for passthrough security model. Move 
 all
 post file creation changes to none security model

 Signed-off-by: M. Mohan Kumar mo...@in.ibm.com
 ---
  hw/9pfs/virtio-9p-local.c |   19 ++-
  1 files changed, 6 insertions(+), 13 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index 08fd67f..d2e32e2 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -208,21 +208,14 @@ static int local_set_xattr(const char *path, FsCred 
 *credp)
      return 0;
  }

 -static int local_post_create_passthrough(FsContext *fs_ctx, const char 
 *path,
 +static int local_post_create_none(FsContext *fs_ctx, const char *path,
          FsCred *credp)
  {
 +    int retval;
      if (chmod(rpath(fs_ctx, path), credp-fc_mode  0)  0) {
          return -1;
      }
 -    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.

 Can multiple virtio-9p requests execute at a time?  chmod() and lchown()
 after creation is a race condition if other requests can execute
 concurrently.

 If some level of serialization is needed it will be done at the client/guest
 inode level.
 Are you worried about filesystem semantics? or do you see some corruption if 
 they
 get executed in parallel?

My main concern is unreliable results due to the race conditions
between creation and the fixups that are performed afterwards.

Is virtio-9p only useful for single guest exclusive access?  I thought
both guest and host could access files at the same time?  What about
multiple VMs sharing a directory?  These scenarios can only work if
operations are made atomic.

Stefan



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

2011-01-20 Thread Venkateswararao Jujjuri (JV)
On 1/20/2011 1:45 PM, Stefan Hajnoczi wrote:
 On Thu, Jan 20, 2011 at 9:15 PM, Venkateswararao Jujjuri (JV)
 jv...@linux.vnet.ibm.com wrote:
 On 1/20/2011 12:59 AM, Stefan Hajnoczi wrote:
 On Tue, Jan 18, 2011 at 01:54:16PM +0530, M. Mohan Kumar wrote:
 After creating a file object, its permission and ownership details are 
 updated
 as per client's request for both passthrough and none security model. But 
 with
 chrooted environment its not required for passthrough security model. Move 
 all
 post file creation changes to none security model

 Signed-off-by: M. Mohan Kumar mo...@in.ibm.com
 ---
  hw/9pfs/virtio-9p-local.c |   19 ++-
  1 files changed, 6 insertions(+), 13 deletions(-)

 diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
 index 08fd67f..d2e32e2 100644
 --- a/hw/9pfs/virtio-9p-local.c
 +++ b/hw/9pfs/virtio-9p-local.c
 @@ -208,21 +208,14 @@ static int local_set_xattr(const char *path, FsCred 
 *credp)
  return 0;
  }

 -static int local_post_create_passthrough(FsContext *fs_ctx, const char 
 *path,
 +static int local_post_create_none(FsContext *fs_ctx, const char *path,
  FsCred *credp)
  {
 +int retval;
  if (chmod(rpath(fs_ctx, path), credp-fc_mode  0)  0) {
  return -1;
  }
 -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.

 Can multiple virtio-9p requests execute at a time?  chmod() and lchown()
 after creation is a race condition if other requests can execute
 concurrently.

 If some level of serialization is needed it will be done at the client/guest
 inode level.
 Are you worried about filesystem semantics? or do you see some corruption if 
 they
 get executed in parallel?
 
 My main concern is unreliable results due to the race conditions
 between creation and the fixups that are performed afterwards.
 
 Is virtio-9p only useful for single guest exclusive access?  I thought
 both guest and host could access files at the same time?  What about
 multiple VMs sharing a directory?  These scenarios can only work if
 operations are made atomic.

For now, there is only one exploiter for the filesystem. The Guest/client.

In the future it could be different and we 'may' support multiple 
exploiters/users.
Note that we have two security models
1. Passthrough 2. Mapped. (3. None -  can be ignored as it is intended for
developer)

Mapped model is advised when you have only one exploiter;
Passthrough model is for more practical application/uses and it can be
used for multiple exploiters (say guests).

In passthrough model we don't do chmod() lchmod() after creating files.

Thanks,
JV
 
 Stefan