Re: [Xen-devel] [PATCH] IOMMU: replace ASSERT()s checking for NULL

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Nov 07, 2016 at 10:30:37AM +0000, Andrew Cooper wrote:
> On 07/11/16 09:24, Jan Beulich wrote:
> > Avoid NULL derefs on non-debug builds.
> >
> > Coverity ID: 1055650
> >
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This introduces a bug:
> 
> >
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -165,7 +165,11 @@ static void pt_irq_time_out(void *data)
> >      spin_lock(&irq_map->dom->event_lock);
> >  
> >      dpci = domain_get_irq_dpci(irq_map->dom);
> > -    ASSERT(dpci);
> > +    if ( unlikely(!dpci) )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        return;


As this does not unlock the 'event_lock' spinlock.

But with the ASSERT_UNREACHABLE() we just spin around.

Perhaps a better option would be just to add an 'unlock'
label and then goto to it?

(And ditch the ASSERT altogether?)

> > +    }
> >      list_for_each_entry ( digl, &irq_map->digl_list, list )
> >      {
> >          unsigned int guest_gsi = hvm_pci_intx_gsi(digl->device, 
> > digl->intx);
> > @@ -793,7 +797,11 @@ void hvm_dpci_msi_eoi(struct domain *d,
> >  
> >  static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci 
> > *pirq_dpci)
> >  {
> > -    ASSERT(d->arch.hvm_domain.irq.dpci);
> > +    if ( unlikely(!d->arch.hvm_domain.irq.dpci) )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        return;
> > +    }
> >  
> >      spin_lock(&d->event_lock);
> >      if ( test_and_clear_bool(pirq_dpci->masked) )
> >
> >
> >
> 

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