[libvirt] [Qemu-devel] [PATCH 1/2] add 'umask' option to -chardev

[Chun Yan Liu]

>>> On 9/2/2014 at 05:16 PM, in message <20140902091655.GB21282 at redhat.com>,
"Daniel P. Berrange" <berrange at redhat.com> wrote: 
> On Tue, Sep 02, 2014 at 03:08:51AM -0600, Chun Yan Liu wrote: 
> >  
> >  
> > >>> On 9/2/2014 at 04:54 PM, in message <20140902085434.GA21282 at redhat.com>, 
> > "Daniel P. Berrange" <berrange at redhat.com> wrote:  
> > > On Tue, Sep 02, 2014 at 03:40:42PM +0800, Chunyan Liu wrote:  
> > > > To use virtio-serial device, unix socket created for chardev with  
> > > > default umask(022) has insufficient permissions.  
> > > >   
> > > > e.g. start kvm guest with:  
> > > > -device virtio-serial \  
> > > > -chardev socket,path=/tmp/foo,server,nowait,id=foo \  
> > > > -device virtserialport,chardev=foo,name=org.fedoraproject.port.0  
> > > >   
> > > > Check permissions for the socket file that has been created in the host  
> > > > to enable communication through virtual serial ports in the guest:  
> > > > #ls -l /tmp/somefile.sock  
> > > > srwxr-xr-x 1 qemu qemu 0 21. Jul 14:19 /tmp/somefile.sock  
> > > >   
> > > > Other users in the qemu group (like real user, test engines, etc) cannot  
> > > > write to this socket.  
> > > >   
> > > > Problem reported here:  
> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=13078#c11  
> > > > https://bugzilla.novell.com/show_bug.cgi?id=888166  
> > > >   
> > > > This patch tries to add a 'umask' option to 'chardev', so that user  
> > > > can have chance to indicate a umask overwritting the default one (default  
>  
> > > > is 022), then create unix sockets with expected permissions.  
> > > >   
> > > > Signed-off-by: Chunyan Liu <cyliu at suse.com>  
> > > > ---  
> > > > This is patch for qemu.  
> > > >   
> > > >  qemu-char.c         |  3 +++  
> > > >  qemu-options.hx     |  9 +++++++--  
> > > >  util/qemu-sockets.c | 12 +++++++++++-  
> > > >  3 files changed, 21 insertions(+), 3 deletions(-)  
> > >   
> > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c  
> > > > index 5d38395..facf2c6 100644  
> > > > --- a/util/qemu-sockets.c  
> > > > +++ b/util/qemu-sockets.c  
> > > > @@ -680,7 +680,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)  
> > > >  {  
> > > >      struct sockaddr_un un;  
> > > >      const char *path = qemu_opt_get(opts, "path");  
> > > > -    int sock, fd;  
> > > > +    int newmask = qemu_opt_get_number(opts, "umask", 0);  
> > > > +    int sock, fd, oldmask;  
> > > >    
> > > >      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);  
> > > >      if (sock < 0) {  
> > > > @@ -708,10 +709,19 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)  
> > > >      }  
> > > >    
> > > >      unlink(un.sun_path);  
> > > > +    if (newmask) {  
> > > > +        oldmask = umask(newmask);  
> > > > +    }  
> > > >      if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) {  
> > > > +        if (newmask) {  
> > > > +            umask(oldmask);  
> > > > +        }  
> > > >          error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);  
> > > >          goto err;  
> > > >      }  
> > > > +    if (newmask) {  
> > > > +        umask(oldmask);  
> > > > +    }  
> > >   
> > > Setting  umask() is not thread-safe as it affects the entire process.  
> While  
> > > this is OK for chardevs which are cold-plugged at startup, once QEMU is  
> > > running it is not OK to alter umask during hotplug of devices.  
> > >   
> > > Wouldn't it be simpler for libvirt to simply set the umask to 002 when it  
> > > first launches QEMU, avoiding the need for trying todo this per device.  
> >  
> > I think that's OK. Only one thing: I'm not sure if permissions of any other 
> > file created in qemu will be changed due to this change, and if that is 
> > unexpected or not. 
>  
> Whether or not it causes problems today is only half the story. I'm more 
> concerned about the long term problems. The use of threads is increasing 
> in QEMU over time, so manipulating umask() is a time-bomb waiting to strike 
> at some point in the future when people have forgotten that this proposed 
> feature exists. IMHO umask() should simply never be used when multiple 
> threads are running, to avoid this long term risk entirely. 

I agree. Now suppose when it explicitly sets the mode S_IWGRP while creating
new file, group user writing permission is really expected, I think setting umask
to 002 to the whole qemu process should be OK.

>  
> Regards, 
> Daniel 
> --  
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/  
> :| 
> |: http://libvirt.org              -o-             http://virt-manager.org  
> :| 
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/  
> :| 
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc  
> :| 
>  
>  
>