Re: [Xen-devel] [PATCH for-4.5] call xc_domain_irq_permission before xc_domain_irq_permission

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 2 Dec 2014, Jan Beulich wrote:
> >>> On 01.12.14 at 20:22, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > xc_physdev_unmap_pirq might revoke the permission to map the irq from
> > the domain causing the following xc_domain_irq_permission call to fail
> > and return error (domain_pirq_to_irq returns 0).
> 
> Apart from the patch title being rather confusing, isn't the root
> problem then xc_physdev_unmap_pirq() removes permissions? At
> least after the strict splitting of permission granting and mapping for
> MMIO (commit 0561e1f0) it would seem that if the underlying
> hypercall here behaves similarly to the original behavior there, it
> ought to be fixed in an analogous way.

The big difference is that 0561e1f0 makes XEN_DOMCTL_memory_mapping
stricker, while you are suggesting to make xc_physdev_unmap_pirq laxer.

Are you sure that is a good idea? What if a third party caller of
xc_physdev_unmap_pirq relies on that behaviour?


 
> > Call xc_domain_irq_permission first to prevent this from happening
> > (xc_physdev_unmap_pirq calls irq_deny_access, that doesn't fail if the
> > permission is already removed).
> > 
> > This is a simple bug fix and I think should go in 4.5.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 316643c..904d255 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -1288,14 +1290,14 @@ skip1:
> >              goto out;
> >          }
> >          if ((fscanf(f, "%u", &irq) == 1) && irq) {
> > -            rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
> > -            if (rc < 0) {
> > -                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, 
> > "xc_physdev_unmap_pirq irq=%d", irq);
> > -            }
> >              rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> >              if (rc < 0) {
> >                  LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, 
> > "xc_domain_irq_permission irq=%d", irq);
> >              }
> > +            rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
> > +            if (rc < 0) {
> > +                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, 
> > "xc_physdev_unmap_pirq irq=%d", irq);
> > +            }
> >          }
> >          fclose(f);
> >      }
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx 
> > http://lists.xen.org/xen-devel 
> 
> 
> 

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