Re: [Xen-devel] [PATCH v2 2/2] x86/p2m: force return value checking of p2m_set_entry()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 12/20/2017 09:35 AM, Jan Beulich wrote:
> As XSAs 246 and 247 have shown, not doing so is rather dangerous.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1550,9 +1550,11 @@ void p2m_mem_paging_populate(struct doma
>          if ( p2mt == p2m_ram_paging_out )
>              req.u.mem_paging.flags |= MEM_PAGING_EVICT_FAIL;
>  
> -        p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_paging_in, a);
> +        rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_paging_in, 
> a);
>      }
>      gfn_unlock(p2m, gfn, 0);
> +    if ( rc < 0 )
> +        return;

On the failure path we call vm_event_claim_slot(), but don't release it.
 Looks like maybe we need an out_cancel that calls vm_event_cancel_slot()?

I was going to say something about |= MEM_PAGING_EVICT_FAIL, but it
looks like that only gets used on success.

>      /* Pause domain if request came from guest and gfn has paging type */
>      if ( p2m_is_paging(p2mt) && v->domain == d )
> @@ -1700,10 +1702,12 @@ void p2m_mem_paging_resume(struct domain
>           */
>          if ( mfn_valid(mfn) && (p2mt == p2m_ram_paging_in) )
>          {
> -            p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
> -                          paging_mode_log_dirty(d) ? p2m_ram_logdirty :
> -                          p2m_ram_rw, a);
> -            set_gpfn_from_mfn(mfn_x(mfn), gfn_x(gfn));
> +            int rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
> +                                   paging_mode_log_dirty(d) ? 
> p2m_ram_logdirty :
> +                                   p2m_ram_rw, a);
> +
> +            if ( !rc )
> +                set_gpfn_from_mfn(mfn_x(mfn), gfn_x(gfn));
>          }
>          gfn_unlock(p2m, gfn, 0);
>      }
> @@ -2463,9 +2467,9 @@ static void p2m_reset_altp2m(struct p2m_
>      p2m->max_remapped_gfn = 0;
>  }
>  
> -void p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
> -                                 mfn_t mfn, unsigned int page_order,
> -                                 p2m_type_t p2mt, p2m_access_t p2ma)
> +int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
> +                                mfn_t mfn, unsigned int page_order,
> +                                p2m_type_t p2mt, p2m_access_t p2ma)
>  {
>      struct p2m_domain *p2m;
>      p2m_access_t a;
> @@ -2474,9 +2478,10 @@ void p2m_altp2m_propagate_change(struct
>      unsigned int i;
>      unsigned int reset_count = 0;
>      unsigned int last_reset_idx = ~0;
> +    int ret = 0;
>  
>      if ( !altp2m_active(d) )
> -        return;
> +        return 0;
>  
>      altp2m_list_lock(d);
>  
> @@ -2515,17 +2520,25 @@ void p2m_altp2m_propagate_change(struct
>                      p2m_unlock(p2m);
>                  }
>  
> -                goto out;
> +                ret = 0;
> +                break;
>              }
>          }
>          else if ( !mfn_eq(m, INVALID_MFN) )
> -            p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
> +        {
> +            int rc = p2m_set_entry(p2m, gfn, mfn, page_order, p2mt, p2ma);
> +
> +            /* Best effort: Don't bail on error. */
> +            if ( !ret )
> +                ret = rc;
> +        }

Hmm, this isn't great -- add this to the long list of functions that
say, "Something went wrong -- maybe nothing was done, maybe it was half
done; good luck."

Wasn't there a patch floating around that would crash a domain if
p2m_set_entry() failed to break down a superpage?  I can't seem to find
that now.

Anyway I think apart from the "leak" mentioned above, the rest of the
patch is fine; the situation isn't great but you're not really making it
worse.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel