Re: [PATCH XEN v5 09/23] tools: Refactor hypercall calling wrappers into libxencall.

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-11-13 at 15:49 +0000, Andrew Cooper wrote:
> On 09/11/15 12:00, Ian Campbell wrote:
> > diff --git a/tools/libs/call/linux.c b/tools/libs/call/linux.c
> > new file mode 100644
> > index 0000000..906ca7e
> > --- /dev/null
> > +++ b/tools/libs/call/linux.c
> > @@ -0,0 +1,132 @@
> > +/*
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation;
> > + * version 2.1 of the License.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library; If not, see <http://www.gnu.org/li
> > censes/>.
> > + *
> > + * Split out from xc_linus_osdep.c:
> > + *
> > + * Copyright 2006 Sun Microsystems, Inc.  All rights reserved.
> > + */
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +
> > +#include <sys/mman.h>
> > +#include <sys/ioctl.h>
> > +
> > +#include "private.h"
> > +
> > +int osdep_xencall_open(xencall_handle *xcall)
> > +{
> > +    int flags, saved_errno;
> > +    int fd = open("/proc/xen/privcmd", O_RDWR);
> 
> I know this is a straight copy, but these new libraries really should
> start with the PVOps interface of /dev/xen/privcmd before falling back
> to the older classic-linux interfaces under /proc

Right, I'll pick this up for (almost, modulo resolving a conflict or two)
free when I rebase over Doug's patch to do this, which I'll be doing before
reposting.

> Ideally, once all these library improvements are done, there should be
> no need to mount xenfs in a Linux PVops domain.
> 
> > +
> > +    if ( fd == -1 )
> > +    {
> > +        PERROR("Could not obtain handle on privileged command
> > interface");
> > +        return -1;
> > +    }
> > +
> > +    /* Although we return the file handle as the 'xc handle' the API
> > +       does not specify / guarentee that this integer is in fact
> > +       a file handle. Thus we must take responsiblity to ensure
> > +       it doesn't propagate (ie leak) outside the process */
> 
> Again, I know this is a straight copy, but it is racy.  The open() call
> needs O_CLOEXEC.
> 
> There might be some issues here to do with older versions of libc.

Or indeed super old versions of Linux (O_CLOEXEC came with 2.6.23 according
to man).

I'm tempted to hedge and use O_CLOEXEC with /dev/xen/privcmd, and leave the
legacy /proc/xen/privcmd path doing it this way, after all we've managed to
get this far...

> > +    /* Do not copy the VMA to child process on fork. Avoid the page
> > being COW
> > +        on hypercall. */
> > +    rc = madvise(p, npages * PAGE_SIZE, MADV_DONTFORK);
> 
> This also seems racy, although it is unclear from the manpages how to
> avoid the race.

pthread_atfork() might be one approach, and libxencall already uses
pthreads (for locking the cache), so it wouldn't be a problem to add more
uses, I don't think.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel