[PATCH 0/2] x86/shadow: address two Coverity issues

[PATCH 0/2] x86/shadow: address two Coverity issues

[Jan Beulich]

It's not clear to me why the tool spotted them now and not before,
but this has got to be a subtle side effect of introducing the tiny
wrapper stubs. Anyway, the "fixes" (perhaps rather workarounds) are
pretty straightforward.

1: adjust some shadow_set_l<N>e() callers
2: adjust 2-level case of SHADOW_FOREACH_L2E()

Jan

[PATCH 1/2] x86/shadow: adjust some shadow_set_l<N>e() callers

[Jan Beulich]

Coverity dislikes sh_page_fault() storing the return value into a local
variable but then never using the value (and oddly enough spots this in
the 2- and 3-level cases, but not in the 4-level one). Instead of adding
yet another cast to void as replacement, take the opportunity and drop a
bunch of such casts at the same time - not using function return values
is a common thing to do. (It of course is an independent question
whether ignoring errors like this is a good idea.)

Coverity-ID: 1492856
Coverity-ID: 1492858
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1416,7 +1416,7 @@ void sh_unhook_32b_mappings(struct domai
     shadow_l2e_t *sl2e;
     SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, 0, d, {
         if ( !user_only || (sl2e->l2 & _PAGE_USER) )
-            (void) shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
+            shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
     });
 }
 
@@ -1428,7 +1428,7 @@ void sh_unhook_pae_mappings(struct domai
     shadow_l2e_t *sl2e;
     SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, 0, d, {
         if ( !user_only || (sl2e->l2 & _PAGE_USER) )
-            (void) shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
+            shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
     });
 }
 
@@ -1439,7 +1439,7 @@ void sh_unhook_64b_mappings(struct domai
     shadow_l4e_t *sl4e;
     SHADOW_FOREACH_L4E(sl4mfn, sl4e, 0, 0, d, {
         if ( !user_only || (sl4e->l4 & _PAGE_USER) )
-            (void) shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
+            shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
     });
 }
 
@@ -1969,7 +1969,7 @@ static void sh_prefetch(struct vcpu *v,
 
         /* Propagate the entry.  */
         l1e_propagate_from_guest(v, gl1e, gmfn, &sl1e, ft_prefetch, p2mt);
-        (void) shadow_set_l1e(d, ptr_sl1e + i, sl1e, p2mt, sl1mfn);
+        shadow_set_l1e(d, ptr_sl1e + i, sl1e, p2mt, sl1mfn);
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
         if ( snpl1p != NULL )
@@ -2534,7 +2534,7 @@ static int sh_page_fault(struct vcpu *v,
 
     /* Calculate the shadow entry and write it */
     l1e_propagate_from_guest(v, gw.l1e, gmfn, &sl1e, ft, p2mt);
-    r = shadow_set_l1e(d, ptr_sl1e, sl1e, p2mt, sl1mfn);
+    shadow_set_l1e(d, ptr_sl1e, sl1e, p2mt, sl1mfn);
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
     if ( mfn_valid(gw.l1mfn)
@@ -3014,8 +3014,7 @@ static bool sh_invlpg(struct vcpu *v, un
                 shadow_l1e_t *sl1;
                 sl1 = sh_linear_l1_table(v) + shadow_l1_linear_offset(linear);
                 /* Remove the shadow entry that maps this VA */
-                (void) shadow_set_l1e(d, sl1, shadow_l1e_empty(),
-                                      p2m_invalid, sl1mfn);
+                shadow_set_l1e(d, sl1, shadow_l1e_empty(), p2m_invalid, sl1mfn);
             }
             paging_unlock(d);
             /* Need the invlpg, to pick up the disappeareance of the sl1e */
@@ -3608,7 +3607,8 @@ int sh_rm_write_access_from_l1(struct do
              && (mfn_x(shadow_l1e_get_mfn(*sl1e)) == mfn_x(readonly_mfn)) )
         {
             shadow_l1e_t ro_sl1e = shadow_l1e_remove_flags(*sl1e, _PAGE_RW);
-            (void) shadow_set_l1e(d, sl1e, ro_sl1e, p2m_ram_rw, sl1mfn);
+
+            shadow_set_l1e(d, sl1e, ro_sl1e, p2m_ram_rw, sl1mfn);
 #if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC
             /* Remember the last shadow that we shot a writeable mapping in */
             if ( curr->domain == d )
@@ -3637,8 +3637,7 @@ int sh_rm_mappings_from_l1(struct domain
         if ( (flags & _PAGE_PRESENT)
              && (mfn_x(shadow_l1e_get_mfn(*sl1e)) == mfn_x(target_mfn)) )
         {
-            (void) shadow_set_l1e(d, sl1e, shadow_l1e_empty(),
-                                  p2m_invalid, sl1mfn);
+            shadow_set_l1e(d, sl1e, shadow_l1e_empty(), p2m_invalid, sl1mfn);
             if ( sh_check_page_has_no_refs(mfn_to_page(target_mfn)) )
                 /* This breaks us cleanly out of the FOREACH macro */
                 done = 1;
@@ -3656,20 +3655,20 @@ void sh_clear_shadow_entry(struct domain
     switch ( mfn_to_page(smfn)->u.sh.type )
     {
     case SH_type_l1_shadow:
-        (void) shadow_set_l1e(d, ep, shadow_l1e_empty(), p2m_invalid, smfn);
+        shadow_set_l1e(d, ep, shadow_l1e_empty(), p2m_invalid, smfn);
         break;
     case SH_type_l2_shadow:
 #if GUEST_PAGING_LEVELS >= 4
     case SH_type_l2h_shadow:
 #endif
-        (void) shadow_set_l2e(d, ep, shadow_l2e_empty(), smfn);
+        shadow_set_l2e(d, ep, shadow_l2e_empty(), smfn);
         break;
 #if GUEST_PAGING_LEVELS >= 4
     case SH_type_l3_shadow:
-        (void) shadow_set_l3e(d, ep, shadow_l3e_empty(), smfn);
+        shadow_set_l3e(d, ep, shadow_l3e_empty(), smfn);
         break;
     case SH_type_l4_shadow:
-        (void) shadow_set_l4e(d, ep, shadow_l4e_empty(), smfn);
+        shadow_set_l4e(d, ep, shadow_l4e_empty(), smfn);
         break;
 #endif
     default: BUG(); /* Called with the wrong kind of shadow. */
@@ -3689,7 +3688,7 @@ int sh_remove_l1_shadow(struct domain *d
         if ( (flags & _PAGE_PRESENT)
              && (mfn_x(shadow_l2e_get_mfn(*sl2e)) == mfn_x(sl1mfn)) )
         {
-            (void) shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
+            shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
             if ( mfn_to_page(sl1mfn)->u.sh.type == 0 )
                 /* This breaks us cleanly out of the FOREACH macro */
                 done = 1;
@@ -3712,7 +3711,7 @@ int sh_remove_l2_shadow(struct domain *d
         if ( (flags & _PAGE_PRESENT)
              && (mfn_x(shadow_l3e_get_mfn(*sl3e)) == mfn_x(sl2mfn)) )
         {
-            (void) shadow_set_l3e(d, sl3e, shadow_l3e_empty(), sl3mfn);
+            shadow_set_l3e(d, sl3e, shadow_l3e_empty(), sl3mfn);
             if ( mfn_to_page(sl2mfn)->u.sh.type == 0 )
                 /* This breaks us cleanly out of the FOREACH macro */
                 done = 1;
@@ -3734,7 +3733,7 @@ int sh_remove_l3_shadow(struct domain *d
         if ( (flags & _PAGE_PRESENT)
              && (mfn_x(shadow_l4e_get_mfn(*sl4e)) == mfn_x(sl3mfn)) )
         {
-            (void) shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
+            shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
             if ( mfn_to_page(sl3mfn)->u.sh.type == 0 )
                 /* This breaks us cleanly out of the FOREACH macro */
                 done = 1;

Re: [PATCH 1/2] x86/shadow: adjust some shadow_set_l<N>e() callers

[Andrew Cooper]

On 13/10/2021 16:37, Jan Beulich wrote:
> Coverity dislikes sh_page_fault() storing the return value into a local
> variable but then never using the value (and oddly enough spots this in
> the 2- and 3-level cases, but not in the 4-level one). Instead of adding
> yet another cast to void as replacement, take the opportunity and drop a
> bunch of such casts at the same time - not using function return values
> is a common thing to do. (It of course is an independent question
> whether ignoring errors like this is a good idea.)
>
> Coverity-ID: 1492856
> Coverity-ID: 1492858
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH 1/2] x86/shadow: adjust some shadow_set_l<N>e() callers

[Stefano Stabellini]

This patch broke gitlab-ci:

https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/1684530258

In file included from guest_4.c:2:
./multi.c:2159:9: error: unused variable 'r' [-Werror,-Wunused-variable]
    int r;
        ^
1 error generated.
make[5]: *** [/builds/xen-project/people/sstabellini/xen/xen/Rules.mk:197: guest_4.o] Error 1


To be sure I got the right commit, I confirmed that by reverting the
commit gitlab-ci passes again:

https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/389383466


On Wed, 13 Oct 2021, Jan Beulich wrote:
> Coverity dislikes sh_page_fault() storing the return value into a local
> variable but then never using the value (and oddly enough spots this in
> the 2- and 3-level cases, but not in the 4-level one). Instead of adding
> yet another cast to void as replacement, take the opportunity and drop a
> bunch of such casts at the same time - not using function return values
> is a common thing to do. (It of course is an independent question
> whether ignoring errors like this is a good idea.)
> 
> Coverity-ID: 1492856
> Coverity-ID: 1492858
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -1416,7 +1416,7 @@ void sh_unhook_32b_mappings(struct domai
>      shadow_l2e_t *sl2e;
>      SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, 0, d, {
>          if ( !user_only || (sl2e->l2 & _PAGE_USER) )
> -            (void) shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
> +            shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
>      });
>  }
>  
> @@ -1428,7 +1428,7 @@ void sh_unhook_pae_mappings(struct domai
>      shadow_l2e_t *sl2e;
>      SHADOW_FOREACH_L2E(sl2mfn, sl2e, 0, 0, d, {
>          if ( !user_only || (sl2e->l2 & _PAGE_USER) )
> -            (void) shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
> +            shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
>      });
>  }
>  
> @@ -1439,7 +1439,7 @@ void sh_unhook_64b_mappings(struct domai
>      shadow_l4e_t *sl4e;
>      SHADOW_FOREACH_L4E(sl4mfn, sl4e, 0, 0, d, {
>          if ( !user_only || (sl4e->l4 & _PAGE_USER) )
> -            (void) shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
> +            shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
>      });
>  }
>  
> @@ -1969,7 +1969,7 @@ static void sh_prefetch(struct vcpu *v,
>  
>          /* Propagate the entry.  */
>          l1e_propagate_from_guest(v, gl1e, gmfn, &sl1e, ft_prefetch, p2mt);
> -        (void) shadow_set_l1e(d, ptr_sl1e + i, sl1e, p2mt, sl1mfn);
> +        shadow_set_l1e(d, ptr_sl1e + i, sl1e, p2mt, sl1mfn);
>  
>  #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
>          if ( snpl1p != NULL )
> @@ -2534,7 +2534,7 @@ static int sh_page_fault(struct vcpu *v,
>  
>      /* Calculate the shadow entry and write it */
>      l1e_propagate_from_guest(v, gw.l1e, gmfn, &sl1e, ft, p2mt);
> -    r = shadow_set_l1e(d, ptr_sl1e, sl1e, p2mt, sl1mfn);
> +    shadow_set_l1e(d, ptr_sl1e, sl1e, p2mt, sl1mfn);
>  
>  #if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
>      if ( mfn_valid(gw.l1mfn)
> @@ -3014,8 +3014,7 @@ static bool sh_invlpg(struct vcpu *v, un
>                  shadow_l1e_t *sl1;
>                  sl1 = sh_linear_l1_table(v) + shadow_l1_linear_offset(linear);
>                  /* Remove the shadow entry that maps this VA */
> -                (void) shadow_set_l1e(d, sl1, shadow_l1e_empty(),
> -                                      p2m_invalid, sl1mfn);
> +                shadow_set_l1e(d, sl1, shadow_l1e_empty(), p2m_invalid, sl1mfn);
>              }
>              paging_unlock(d);
>              /* Need the invlpg, to pick up the disappeareance of the sl1e */
> @@ -3608,7 +3607,8 @@ int sh_rm_write_access_from_l1(struct do
>               && (mfn_x(shadow_l1e_get_mfn(*sl1e)) == mfn_x(readonly_mfn)) )
>          {
>              shadow_l1e_t ro_sl1e = shadow_l1e_remove_flags(*sl1e, _PAGE_RW);
> -            (void) shadow_set_l1e(d, sl1e, ro_sl1e, p2m_ram_rw, sl1mfn);
> +
> +            shadow_set_l1e(d, sl1e, ro_sl1e, p2m_ram_rw, sl1mfn);
>  #if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC
>              /* Remember the last shadow that we shot a writeable mapping in */
>              if ( curr->domain == d )
> @@ -3637,8 +3637,7 @@ int sh_rm_mappings_from_l1(struct domain
>          if ( (flags & _PAGE_PRESENT)
>               && (mfn_x(shadow_l1e_get_mfn(*sl1e)) == mfn_x(target_mfn)) )
>          {
> -            (void) shadow_set_l1e(d, sl1e, shadow_l1e_empty(),
> -                                  p2m_invalid, sl1mfn);
> +            shadow_set_l1e(d, sl1e, shadow_l1e_empty(), p2m_invalid, sl1mfn);
>              if ( sh_check_page_has_no_refs(mfn_to_page(target_mfn)) )
>                  /* This breaks us cleanly out of the FOREACH macro */
>                  done = 1;
> @@ -3656,20 +3655,20 @@ void sh_clear_shadow_entry(struct domain
>      switch ( mfn_to_page(smfn)->u.sh.type )
>      {
>      case SH_type_l1_shadow:
> -        (void) shadow_set_l1e(d, ep, shadow_l1e_empty(), p2m_invalid, smfn);
> +        shadow_set_l1e(d, ep, shadow_l1e_empty(), p2m_invalid, smfn);
>          break;
>      case SH_type_l2_shadow:
>  #if GUEST_PAGING_LEVELS >= 4
>      case SH_type_l2h_shadow:
>  #endif
> -        (void) shadow_set_l2e(d, ep, shadow_l2e_empty(), smfn);
> +        shadow_set_l2e(d, ep, shadow_l2e_empty(), smfn);
>          break;
>  #if GUEST_PAGING_LEVELS >= 4
>      case SH_type_l3_shadow:
> -        (void) shadow_set_l3e(d, ep, shadow_l3e_empty(), smfn);
> +        shadow_set_l3e(d, ep, shadow_l3e_empty(), smfn);
>          break;
>      case SH_type_l4_shadow:
> -        (void) shadow_set_l4e(d, ep, shadow_l4e_empty(), smfn);
> +        shadow_set_l4e(d, ep, shadow_l4e_empty(), smfn);
>          break;
>  #endif
>      default: BUG(); /* Called with the wrong kind of shadow. */
> @@ -3689,7 +3688,7 @@ int sh_remove_l1_shadow(struct domain *d
>          if ( (flags & _PAGE_PRESENT)
>               && (mfn_x(shadow_l2e_get_mfn(*sl2e)) == mfn_x(sl1mfn)) )
>          {
> -            (void) shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
> +            shadow_set_l2e(d, sl2e, shadow_l2e_empty(), sl2mfn);
>              if ( mfn_to_page(sl1mfn)->u.sh.type == 0 )
>                  /* This breaks us cleanly out of the FOREACH macro */
>                  done = 1;
> @@ -3712,7 +3711,7 @@ int sh_remove_l2_shadow(struct domain *d
>          if ( (flags & _PAGE_PRESENT)
>               && (mfn_x(shadow_l3e_get_mfn(*sl3e)) == mfn_x(sl2mfn)) )
>          {
> -            (void) shadow_set_l3e(d, sl3e, shadow_l3e_empty(), sl3mfn);
> +            shadow_set_l3e(d, sl3e, shadow_l3e_empty(), sl3mfn);
>              if ( mfn_to_page(sl2mfn)->u.sh.type == 0 )
>                  /* This breaks us cleanly out of the FOREACH macro */
>                  done = 1;
> @@ -3734,7 +3733,7 @@ int sh_remove_l3_shadow(struct domain *d
>          if ( (flags & _PAGE_PRESENT)
>               && (mfn_x(shadow_l4e_get_mfn(*sl4e)) == mfn_x(sl3mfn)) )
>          {
> -            (void) shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
> +            shadow_set_l4e(d, sl4e, shadow_l4e_empty(), sl4mfn);
>              if ( mfn_to_page(sl3mfn)->u.sh.type == 0 )
>                  /* This breaks us cleanly out of the FOREACH macro */
>                  done = 1;
> 
> 

Re: [PATCH 1/2] x86/shadow: adjust some shadow_set_l<N>e() callers

[Jan Beulich]

On 16.10.2021 00:55, Stefano Stabellini wrote:
> This patch broke gitlab-ci:
> 
> https://gitlab.com/xen-project/people/sstabellini/xen/-/jobs/1684530258
> 
> In file included from guest_4.c:2:
> ./multi.c:2159:9: error: unused variable 'r' [-Werror,-Wunused-variable]
>     int r;
>         ^
> 1 error generated.
> make[5]: *** [/builds/xen-project/people/sstabellini/xen/xen/Rules.mk:197: guest_4.o] Error 1

Oh, indeed, that variable is now only when CONFIG_HVM. Fix sent.

Jan

[PATCH 2/2] x86/shadow: adjust 2-level case of SHADOW_FOREACH_L2E()

[Jan Beulich]

Coverity apparently takes issue with the assignment inside an if(), but
then only in two of the cases (sh_destroy_l2_shadow() and
sh_unhook_32b_mappings()). As it's pretty simple to break out of the
outer loop without the need for a local helper variable, adjust the code
that way.

While there, with the other "unused value" reports also in mind, further
drop a dead assignment from SHADOW_FOREACH_L1E().

Coverity-ID: 1492857
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Looking over other SHADOW_FOREACH_L<N>E() invocations wrt their uses of
"done", I find the audit ones particularly odd: The respective variables
get set only from AUDIT_FAIL() and AUDIT_FAIL_MIN(), but in both cases
after invoking BUG(), i.e. in an unreachable position.

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -794,8 +794,7 @@ do {
                          ({ (__done = _done); }), _code);               \
     _sl1mfn = sh_next_page(_sl1mfn);                                    \
     if ( !__done )                                                      \
-        _SHADOW_FOREACH_L1E(_sl1mfn, _sl1e, _gl1p,                      \
-                             ({ (__done = _done); }), _code);           \
+        _SHADOW_FOREACH_L1E(_sl1mfn, _sl1e, _gl1p, _done, _code);       \
 } while (0)
 #else /* Everything else; l1 shadows are only one page */
 #define SHADOW_FOREACH_L1E(_sl1mfn, _sl1e, _gl1p, _done, _code)         \
@@ -808,10 +807,10 @@ do {
 /* 32-bit l2 on PAE/64: four pages, touch every second entry */
 #define SHADOW_FOREACH_L2E(_sl2mfn, _sl2e, _gl2p, _done, _dom, _code)     \
 do {                                                                      \
-    int _i, _j, __done = 0;                                               \
+    int _i, _j;                                                           \
     ASSERT(shadow_mode_external(_dom));                                   \
     ASSERT(mfn_to_page(_sl2mfn)->u.sh.type == SH_type_l2_32_shadow);      \
-    for ( _j = 0; _j < 4 && !__done; _j++ )                               \
+    for ( _j = 0; _j < 4; _j++ )                                          \
     {                                                                     \
         shadow_l2e_t *_sp = map_domain_page(_sl2mfn);                     \
         for ( _i = 0; _i < SHADOW_L2_PAGETABLE_ENTRIES; _i += 2 )         \
@@ -819,11 +818,12 @@ do {
             (_sl2e) = _sp + _i;                                           \
             if ( shadow_l2e_get_flags(*(_sl2e)) & _PAGE_PRESENT )         \
                 {_code}                                                   \
-            if ( (__done = (_done)) ) break;                              \
+            if ( _done ) break;                                           \
             increment_ptr_to_guest_entry(_gl2p);                          \
         }                                                                 \
         unmap_domain_page(_sp);                                           \
         if ( _j < 3 ) _sl2mfn = sh_next_page(_sl2mfn);                    \
+        if ( _i < SHADOW_L2_PAGETABLE_ENTRIES ) break;                    \
     }                                                                     \
 } while (0)
 

Re: [PATCH 2/2] x86/shadow: adjust 2-level case of SHADOW_FOREACH_L2E()

[Andrew Cooper]

On 13/10/2021 16:38, Jan Beulich wrote:
> Coverity apparently takes issue with the assignment inside an if(), but
> then only in two of the cases (sh_destroy_l2_shadow() and
> sh_unhook_32b_mappings()). As it's pretty simple to break out of the
> outer loop without the need for a local helper variable, adjust the code
> that way.
>
> While there, with the other "unused value" reports also in mind, further
> drop a dead assignment from SHADOW_FOREACH_L1E().
>
> Coverity-ID: 1492857
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Looking over other SHADOW_FOREACH_L<N>E() invocations wrt their uses of
> "done", I find the audit ones particularly odd: The respective variables
> get set only from AUDIT_FAIL() and AUDIT_FAIL_MIN(), but in both cases
> after invoking BUG(), i.e. in an unreachable position.

Sounds like there is dead code we can drop.

This logic is all horrible.  I think this is an improvement, so
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 0/2] x86/shadow: address two Coverity issues

[Andrew Cooper]

On 13/10/2021 16:36, Jan Beulich wrote:
> It's not clear to me why the tool spotted them now and not before,

Several reasons.

The Coverity backend is a software product just like everything else. 
IIRC, it releases quarterly.

"If something's free, then you are the product".  The value of offering
free scanning of open source codebases comes from us (the free users)
integrating a massive corpus of code into Coverity's system, upon which
they can evaluate the effectiveness of new heuristics.


Second, and far more likely in this case, "x86/mm: avoid building
multiple .o from a single .c file".  Coverity has always choked on that
in Xen, because it's intermediate database is keyed on source file with
latest takes precedent, so we were only seeing the 4-level case previously.


And to also answer your question from patch 1 here, there are upper time
and complexity bounds on all analysis, because scanning is an
exponential problem with the size of the source file.  I don't know
exactly where the cutoffs are, and I fear that some of our larger files
never have later functions looked at.

~Andrew

Re: [PATCH 0/2] x86/shadow: address two Coverity issues

[Jan Beulich]

On 13.10.2021 18:10, Andrew Cooper wrote:
> On 13/10/2021 16:36, Jan Beulich wrote:
>> It's not clear to me why the tool spotted them now and not before,
> 
> Several reasons.
> 
> The Coverity backend is a software product just like everything else. 
> IIRC, it releases quarterly.
> 
> "If something's free, then you are the product".  The value of offering
> free scanning of open source codebases comes from us (the free users)
> integrating a massive corpus of code into Coverity's system, upon which
> they can evaluate the effectiveness of new heuristics.
> 
> 
> Second, and far more likely in this case, "x86/mm: avoid building
> multiple .o from a single .c file".  Coverity has always choked on that
> in Xen, because it's intermediate database is keyed on source file with
> latest takes precedent, so we were only seeing the 4-level case previously.
> 
> 
> And to also answer your question from patch 1 here, there are upper time
> and complexity bounds on all analysis, because scanning is an
> exponential problem with the size of the source file.  I don't know
> exactly where the cutoffs are, and I fear that some of our larger files
> never have later functions looked at.

Thanks for the explanations. I have to admit that I would find it helpful
if the tool distinguished new issues it found just because code previously
wasn't scanned from ones that were truly introduced anew. For patch 1 here
this might mean that the report was previously put off when reported
against the 4-level case; I think it shouldn't have been ignored, but
opinions might diverge and hence there might be a reason why patch 1
isn't wanted then. Patch 2, otoh, doesn't have a 4-level equivalent so is
likely to be wanted. Unfortunately your reply didn't include an ack, nak,
or at least a vague indication towards either, so I don't really know what
(if anything) it means towards the actual patches.

Jan