Re: [PATCH XEN v6 25/32] tools/libs/gnttab: Extensive updates to API documentation.

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

On Wed, 2015-12-09 at 13:50 +0000, Andrew Cooper wrote:
> On 09/12/15 13:00, Ian Campbell wrote:
> > On Wed, 2015-12-09 at 12:41 +0000, Wei Liu wrote:
> > > > + */
> > > > +
> > > > +/*
> > > >   * Grant Table Interface (making use of grants from other domains)
> > > >   */
> > > >  
> > > >  typedef struct xengntdev_handle xengnttab_handle;
> > > >  
> > > >  /*
> > > > - * Note:
> > > > - * After fork a child process must not use any opened xc gnttab
> > > > - * handle inherited from their parent. They must open a new handle
> > > > if
> > > > - * they want to interact with xc.
> > > > + * Returns a handle onto the grant table driver.  Logs errors.
> > > > + *
> > > > + * Note: After fork a child process must not use any opened gnttab
> > > > + * handle inherited from their parent, more access any grant
> > > > mapped
> > > > + * areas associated with that handle.
> > > > + *
> > > And this could use the same cloexec trick as you do in other patch
> > > for
> > > privcmd.
> > I think this statement "do not use the handle" still stands regardless
> > of
> > the underlying fd being cloexec'd.
> > 
> > However, this did make me think about a comment elsewhere, specifically
> > the
> > various instances of:
> >  * This is the only function which may be safely called on a
> >  * xen<foo>_handle in a child after a fork.
> > for several xen<foo>_close() functions.
> > 
> > This is not really true if the fd is cloexec, since then _close() will
> > either fail (EBADF) or, worse, close some other freshly opened file
> > descriptor.
> > 
> > There seems to be two choices here, either require all osdep backends
> > to
> > make their fds O_CLOEXEC (which might involve tolerating a racy
> > fcntl+FD_CLOEXEC pattern after open on some platforms) or don't set
> > O_CLOEXEC ever and declare that the application is responsible for
> > closing
> > after fork, and for taking care of the corner cases themselves in
> > multithreaded applications.
> > 
> > The former seems friendlier to me, even if some platforms need to use
> > FD_CLOEXEC.
> > 
> > Hrm, maybe I can extend the atfork trick to cover the open+fcntl bits.
> > Hopefully there is no issue with using pthreads from each of the
> > affected
> > libraries.
> > 
> > So, I think the advice in the comment would then be:
> > 
> >     If you fork and then exec then you must not (and need not) call
> > _close()
> >     or any other function on the handle.
> > 
> >     If you fork but do not exec then it is permissible to call
> > _close(), but
> >     it is not permissible to call any other function on the handle.
> > 
> > Need to think about that wording.
> 
> This is risky.  What if $FOO_open() allocated more resource than just
> the CLOEXEC fd?

Any allocated memory (the other main class of such resource) will be
abolished by an exec, hence the distinction made above between fork and
fork+exec.

What other sorts of resources are you worried about?

> An _open() call must be matched with a _close() call.

This is not safe after an exec though, since the fd will be closed, or
perhaps even reopened already (although the requirement could be to call
_close before doing such things as opening any fds, at which point close
could silently handle EBADF).

> In the case of fork but no exec, there should be a _close() call in both
> the parent and child, to free other resources.

In the parent I assume you mean "at some point (or call exit())" rather
than in some way associated with the forking, because the parent is
entitled to keep using the handle.

Ian.

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