[PATCH] IOMMU: replace ASSERT()s checking for NULL

[PATCH] IOMMU: replace ASSERT()s checking for NULL

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

Avoid NULL derefs on non-debug builds.

Coverity ID: 1055650

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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;
+    }
     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) )




[-- Attachment #2: IOMMU-dpci-assert.patch --]
[-- Type: text/plain, Size: 1054 bytes --]

IOMMU: replace ASSERT()s checking for NULL

Avoid NULL derefs on non-debug builds.

Coverity ID: 1055650

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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;
+    }
     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) )

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> On 07.11.16 at 10:24, <JBeulich@suse.com> wrote:
> --- 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;
> +    }
>      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;
> +    }

I wonder, btw, whether we shouldn't ease these by making a macro
along the lines of

#define ASSERT_BAIL(cond, retval...) do { \
    if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \
} while (0)

Opinions?

Jan


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

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

[Andrew Cooper]

On 07/11/16 09:53, Jan Beulich wrote:
>>>> On 07.11.16 at 10:24, <JBeulich@suse.com> wrote:
>> --- 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;
>> +    }
>>      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;
>> +    }
> I wonder, btw, whether we shouldn't ease these by making a macro
> along the lines of
>
> #define ASSERT_BAIL(cond, retval...) do { \
>     if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \
> } while (0)
>
> Opinions?

On the one hand, this is becoming a common pattern.  On the other, I
really dislike hiding control flow in a macro like this.

It might be ok if named ASSERT_UNREACHABLE_RETURN() to both highlight
that it is an ASSERT_UNREACHABLE() rather than an ASSERT() of the
condition passed.  Perhaps  ASSERT_UNREACHABLE_RETURN_IF() to avoid
mixing up the types of assertion?

~Andrew

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

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

[Jan Beulich]

>>> On 07.11.16 at 11:43, <andrew.cooper3@citrix.com> wrote:
> On 07/11/16 09:53, Jan Beulich wrote:
>>>>> On 07.11.16 at 10:24, <JBeulich@suse.com> wrote:
>>> --- 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;
>>> +    }
>>>      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;
>>> +    }
>> I wonder, btw, whether we shouldn't ease these by making a macro
>> along the lines of
>>
>> #define ASSERT_BAIL(cond, retval...) do { \
>>     if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); return retval; } \
>> } while (0)
>>
>> Opinions?
> 
> On the one hand, this is becoming a common pattern.  On the other, I
> really dislike hiding control flow in a macro like this.
> 
> It might be ok if named ASSERT_UNREACHABLE_RETURN() to both highlight
> that it is an ASSERT_UNREACHABLE() rather than an ASSERT() of the
> condition passed.  Perhaps  ASSERT_UNREACHABLE_RETURN_IF() to avoid
> mixing up the types of assertion?

That would end up being (taking one of the examples above)

static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
    ASSERT_UNREACHABLE_RETURN_IF(d->arch.hvm_domain.irq.dpci);
    ...

or

static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci)
{
    ASSERT_UNREACHABLE_RETURN_IF(!d->arch.hvm_domain.irq.dpci);
    ...

No matter which way I take it, I find it confusing: Is the condition
meant to be ASSERT()-like or if()-like? If hiding control flow keywords
in a macro looks bad to you, how about

#define ASSERT_BAIL(cond, stmt...) do { \
    if ( unlikely(!(cond)) ) { ASSERT_UNREACHABLE(); stmt; } \
} while (0)

i.e. forcing the "return" to be visible at the macro invocation site
(and at once allowing e.g. using "break" or "goto" there too)? It
might even be worthwhile calling it ASSERT_CLEANUP() or some
such, making it more logical to use even non-control-flow
statements there (like setting a variable to a certain value).

Jan


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

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

[Andrew Cooper]

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@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- 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;
> +    }
>      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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Wei Liu]

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@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

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

[Konrad Rzeszutek Wilk]

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@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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@lists.xen.org
https://lists.xen.org/xen-devel