Xen devel - Aug 2013 - [RFC] x86/hpet: Correct -ENOMEM actions in hpet_fsb_cap_lookup()

These changes are entirely from inspection, discovered while investigating
another problem.

* Don''t leak the previously allocated cpumasks
* Don''t leave num_hpets_used > 0.  It would fool
hpet_broadcast_init() into
  believing that broadcast mode had been set up, despite having freed the
  underlying datastructure (and subsequenly result in a NULL pointer fault).
* Unconditionally decallocate hpet_events.  hpet_broadcast_init() will then
  try to allocate a single hpet_event_channel instead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---

This patch is RFC as I didn''t actually encounter the problem, nor can
think of
an easy way of actually testing the correctness of the codepath.  Chances are
that if -ENOMEM occurs here, Xen is not actually going to complete booting.
---
 xen/arch/x86/hpet.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 7e0d332..bad2d68 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -415,11 +415,12 @@ static void __init hpet_fsb_cap_lookup(void)
 
         if ( !zalloc_cpumask_var(&ch->cpumask) )
         {
-            if ( !num_hpets_used )
-            {
-                xfree(hpet_events);
-                hpet_events = NULL;
-            }
+            /* Out of mem.  Clean up and bail. */
+            for ( i = 0; i < num_hpets_used; ++i )
+                free_cpumask_var(hpet_events[i].cpumask);
+            xfree(hpet_events);
+            hpet_events = NULL;
+            num_hpets_used = 0;
             break;
         }
 
-- 
1.7.10.4

>>> On 28.08.13 at 18:55, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> These changes are entirely from inspection, discovered while investigating
> another problem.
> 
> * Don''t leak the previously allocated cpumasks
> * Don''t leave num_hpets_used > 0.  It would fool
hpet_broadcast_init() into
>   believing that broadcast mode had been set up, despite having freed the
>   underlying datastructure (and subsequenly result in a NULL pointer
fault).
> * Unconditionally decallocate hpet_events.  hpet_broadcast_init() will then
>   try to allocate a single hpet_event_channel instead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> 
> ---
> 
> This patch is RFC as I didn''t actually encounter the problem, nor
can think of
> an easy way of actually testing the correctness of the codepath.  Chances
are
> that if -ENOMEM occurs here, Xen is not actually going to complete booting.
And you''re turning a success case (just using fewer than the
available channels) into an error one - it was intentionally coded
this way, and only if there''s a problem with that logic I''d
consider
a patch valid.

Jan
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -415,11 +415,12 @@ static void __init hpet_fsb_cap_lookup(void)
>  
>          if ( !zalloc_cpumask_var(&ch->cpumask) )
>          {
> -            if ( !num_hpets_used )
> -            {
> -                xfree(hpet_events);
> -                hpet_events = NULL;
> -            }
> +            /* Out of mem.  Clean up and bail. */
> +            for ( i = 0; i < num_hpets_used; ++i )
> +                free_cpumask_var(hpet_events[i].cpumask);
> +            xfree(hpet_events);
> +            hpet_events = NULL;
> +            num_hpets_used = 0;
>              break;
>          }
>  
> -- 
> 1.7.10.4

On 29/08/13 08:12, Jan Beulich wrote:
>>
>> This patch is RFC as I didn''t actually encounter the problem,
nor can think of
>> an easy way of actually testing the correctness of the codepath. 
Chances are
>> that if -ENOMEM occurs here, Xen is not actually going to complete
booting.
> And you''re turning a success case (just using fewer than the
> available channels) into an error one - it was intentionally coded
> this way, and only if there''s a problem with that logic
I''d consider
> a patch valid.
>
> Jan
>
Ah - I had not appreciated that possibility, which does make sense.

~Andrew