| Message ID | 1295339056-25396-1-git-send-email-mohan@in.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <mohan@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 & 07777) < 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
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
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
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 <mohan@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 & 07777) < 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 >
On Thu, Jan 20, 2011 at 2:48 PM, Daniel P. Berrange <berrange@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
On Thu, Jan 20, 2011 at 9:15 PM, Venkateswararao Jujjuri (JV) <jvrao@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 <mohan@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 & 07777) < 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
On 1/20/2011 1:45 PM, Stefan Hajnoczi wrote: > On Thu, Jan 20, 2011 at 9:15 PM, Venkateswararao Jujjuri (JV) > <jvrao@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 <mohan@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 & 07777) < 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
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 & 07777) < 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; } @@ -363,7 +356,7 @@ static int local_mknod(FsContext *fs_ctx, const char *path, FsCred *credp) if (err == -1) { return err; } - err = local_post_create_passthrough(fs_ctx, path, credp); + err = local_post_create_none(fs_ctx, path, credp); if (err == -1) { serrno = errno; goto err_end; @@ -405,7 +398,7 @@ static int local_mkdir(FsContext *fs_ctx, const char *path, FsCred *credp) if (err == -1) { return err; } - err = local_post_create_passthrough(fs_ctx, path, credp); + err = local_post_create_none(fs_ctx, path, credp); if (err == -1) { serrno = errno; goto err_end; @@ -480,7 +473,7 @@ static int local_open2(FsContext *fs_ctx, const char *path, int flags, if (fd == -1) { return fd; } - err = local_post_create_passthrough(fs_ctx, path, credp); + err = local_post_create_none(fs_ctx, path, credp); if (err == -1) { serrno = errno; goto err_end;
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 <mohan@in.ibm.com> --- hw/9pfs/virtio-9p-local.c | 19 ++++++------------- 1 files changed, 6 insertions(+), 13 deletions(-)