Re: [Qemu-devel] [V3 PATCH 7/8] virtio-9p: Move file post creation changes to none security model
On 1/20/2011 1:45 PM, Stefan Hajnoczi wrote:
> On Thu, Jan 20, 2011 at 9:15 PM, Venkateswararao Jujjuri (JV)
> 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
---
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
Re: [Qemu-devel] [V3 PATCH 7/8] virtio-9p: Move file post creation changes to none security model
On Thu, Jan 20, 2011 at 9:15 PM, Venkateswararao Jujjuri (JV)
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
>>> ---
>>> 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
On Thu, Jan 20, 2011 at 2:48 PM, Daniel P. Berrange 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
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
>> ---
>> 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
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
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
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
> ---
> 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
