Re: [Xen-devel] [PATCH RFC 2/4] xen: grant_table: implement grant_table_soft_reset()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.06.15 at 15:35, <vkuznets@xxxxxxxxxx> wrote:
> When soft reset is being performed we need to replace all actively
> granted pages with empty pages to prevent possible future memory
> corruption as the newly started kernel won't be aware of these
> granted pages.
> 
> We make the tot_pages < max_pages assumption here: previously granted pages
> need to belong to someone and we don't want to implement possible DoS by
> reassigning them to the grantee/anonymous domain/xen/.. (the malicious guest
> will be able to consume all host's memory).

How is that going to look in practice? I.e. won't this cause frequent
failures?

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3120,6 +3120,68 @@ gnttab_release_mappings(
>      }
>  }
>  
> +int grant_table_soft_reset(struct domain *d)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    struct active_grant_entry *act;
> +    grant_ref_t ref;
> +    unsigned long mfn_new;
> +    struct page_info *page, *new_page;
> +    int ret = 0;
> +
> +    spin_lock(&gt->lock);
> +
> +    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
> +    {
> +        act = &active_entry(gt, ref);
> +        if ( !act->pin )
> +            continue;
> +
> +        page = mfn_to_page(act->frame);
> +        if ( !page || !get_page(page, d) )

Under what conditions would mfn_to_page() return NULL? I.e. don't
you rather need to check mfn_valid(act->frame) if you want some
sort of guarantee that the pointer you obtain can be dereferenced?
(Quite likely this could be an ASSERT() instead of an if(), as is done
elsewhere.)

> +        {
> +            printk(XENLOG_G_ERR "Dom%d's grant frame %lx (gfn %lx) is 
> invalid",
> +                   d->domain_id, act->gfn, act->frame);

Either you mean "mfn" in the format string, or you got ->gfn and
->frame swapped here.

> +            ret = -EINVAL;
> +            break;

If this happens after a successful replacement of another granted
page, what is the expected disposition of the domain? I.e. is it
correct to just return failure to the caller, rather than killing the
domain (which is now in an inconsistent state)?

> +        }
> +
> +        /*
> +         * Here we assume the domain has tot_pages < max_pages. Previously
> +         * granted page will still belong to us until it is unmapped from
> +         * grantee.
> +         */
> +        new_page = alloc_domheap_page(d, 0);
> +        if ( !new_page )
> +        {
> +            printk(XENLOG_G_ERR "Failed to alloc a page to replace"
> +                   " Dom%d's grant frame %lx\n", d->domain_id, act->frame);
> +            ret = -ENOMEM;
> +            put_page(page);
> +            break;
> +        }
> +        mfn_new = page_to_mfn(new_page);
> +        guest_remove_page(d, act->gfn);
> +
> +        if ( guest_physmap_add_page(d, act->gfn, mfn_new, 0) )
> +        {
> +            printk(XENLOG_G_ERR "Failed to replace Dom%d's grant frame %lx"
> +                   " with new MFN %lx\n", d->domain_id, act->frame, mfn_new);
> +            ret = -EFAULT;
> +        }
> +
> +        gnttab_flush_tlb(d);

Is this needed (a) at all or (b) on each iteration, considering that you
don't remove from the grantee or de-allocate the old page?

> +        put_page(new_page);
> +        put_page(page);

Do you really need to hold onto the page until here?

> +        if (ret)

Missing spaces inside the parentheses.

> +            break;
> +    }
> +
> +    spin_unlock(&gt->lock);
> +    return ret;

Blank line before final return statement please.

Jan


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