Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jan 21, 2015 at 10:57:05PM +0000, Andrew Cooper wrote:
> On 21/01/2015 21:39, Andrew Cooper wrote:
> > On 21/01/2015 21:27, Konrad Rzeszutek Wilk wrote:
> >> As I was looking at some of the XSA I realized that the
> >> call-chain of:
> >>
> >>  domain_relinquish_resources
> >>    ->vcpu_destroy_pagetables
> >>      -> put_page_and_type_preemptible
> >>         -> __put_page_type
> >> 		returns -EINTR
> >>
> >> which means we end up at:
> >>  618         rc = domain_relinquish_resources(d);                                    
> >>  619         if ( rc != 0 )                                                          
> >>  620         {                                                                       
> >>  621             if ( rc == -ERESTART )                                              
> >>  622                 rc = -EAGAIN;                                                   
> >>  623             break;             <=== with rc=-EINTR
> >>  624         }                                         
> >>
> >> And return -EINTR to user-space - which loop in 
> >> 'xc_domain_destroy' is only looking for:
> >>
> >>  112 int xc_domain_destroy(xc_interface *xch,                                        
> >>  113                       uint32_t domid)                                           
> >>  114 {                                                                               
> >>  115     int ret;                                                                    
> >>  116     DECLARE_DOMCTL;                                                             
> >>  117     domctl.cmd = XEN_DOMCTL_destroydomain;                                      
> >>  118     domctl.domain = (domid_t)domid;                                             
> >>  119     do {                                                                        
> >>  120         ret = do_domctl(xch, &domctl);                                          
> >>  121     } while ( ret && (errno == EAGAIN) );                                       
> >>  122     return ret;                                                                 
> >>  123 }                                
> >>
> >> which to my reading looks like we would exit out and leave
> >> an DOMDYING_dying domain. Looking at the code it seems possible
> >> to continue on if the user does 'xl destroy <X>' guest again,
> >> but I was wondering if:
> >>
> >>  a). Should the toolstack (libxl or libxc) have the code to
> >>      handle -EINTR?
> >>
> >>  b). Or should the hypervisor convert the -EINTR to -ERESTART
> >>      (or -EAGAIN) - which most of the code (see users of
> >>      get_page_type_preemptible) do right now?
> > Good spot.
> >
> > Other areas of similar code condense EINTR into ERESTART.  I think in
> > this case it is Xen's job to turn -EINTR into -EAGAIN as this hypercall
> > specifically has preemptibility  built into its normal use.
> >
> > I wonder if there are other similar hypercall paths which need to catch
> > EINTR as well as ERESTART?

I did not see them.
> >
> > ~Andrew
> 
> Thinking about this, it occurs to me that, along with parameter
> clobbering in debug builds, we should assert that internal error codes
> never escape to guests.

Are there any other ones besides ERESTART/EINTR ?
> 
> It also occurs to me that the PV hypercall paths would both be far more
> simple (particularly the register clobbering bits) if they were written
> in C like their HVM counterparts, rather than ASM.  I will see whether I
> can find some copious free time to see about making this happen.
> 
> ~Andrew