Re: [PATCH] dm_op: Add xendevicemodel_modified_memory_bulk.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.03.17 at 15:44, <jennifer.herbert@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -119,56 +119,96 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
>  }
>  
>  static int modified_memory(struct domain *d,
> -                           struct xen_dm_op_modified_memory *data)
> +                           struct xen_dm_op_modified_memory *header,
> +                           xen_dm_op_buf_t* buf)
>  {
> -    xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
> -    unsigned int iter = 0;
> -    int rc = 0;
> +    /* Process maximum of 255 pfns before checking for continuation */
> +    const uint32_t max_pfns = 0xff;
>  
> -    if ( (data->first_pfn > last_pfn) ||
> -         (last_pfn > domain_get_maximum_gpfn(d)) )
> -        return -EINVAL;
> +    xen_pfn_t last_pfn;
> +    int rc = 0;
> +    uint32_t offset = header->offset;
> +    unsigned long pfn;
> +    unsigned long max_pfn;
> +    unsigned max_nr;
> +    uint32_t rem_pfns = max_pfns;

Please avoid fixed width types when you don't really need them. Also
"unsigned int" please instead of plain "unsigned".

>      if ( !paging_mode_log_dirty(d) )
>          return 0;
>  
> -    while ( iter < data->nr )
> +    if ( (buf->size / sizeof(struct xen_dm_op_modified_memory_extent)) <
> +        header->nr_extents )

Indentation.

> +        return -EINVAL;
> +
> +    while ( offset < header->nr_extents )
>      {
> -        unsigned long pfn = data->first_pfn + iter;
> -        struct page_info *page;
> +        struct xen_dm_op_modified_memory_extent extent;
>  
> -        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> -        if ( page )
> +        if ( copy_from_guest_offset(&extent, buf->h, offset, 1) )
> +            return -EFAULT;
> +
> +        last_pfn = extent.first_pfn + extent.nr - 1;
> +
> +        if ( last_pfn > domain_get_maximum_gpfn(d) )

Where did the original "(data->first_pfn > last_pfn)" go?

> +            return -EINVAL;
> +
> +        if ( extent.nr > rem_pfns )
> +           max_nr = rem_pfns;
> +        else
>          {
> -            mfn_t gmfn = _mfn(page_to_mfn(page));
> -
> -            paging_mark_dirty(d, gmfn);
> -            /*
> -             * These are most probably not page tables any more
> -             * don't take a long time and don't die either.
> -             */
> -            sh_remove_shadows(d, gmfn, 1, 0);
> -            put_page(page);
> +            max_nr = extent.nr;
> +            offset++;
>          }
>  
> -        iter++;
> +        rem_pfns -= max_nr;
> +        max_pfn = extent.first_pfn + max_nr;
>  
> -        /*
> -         * Check for continuation every 256th iteration and if the
> -         * iteration is not the last.
> -         */
> -        if ( (iter < data->nr) && ((iter & 0xff) == 0) &&
> -             hypercall_preempt_check() )
> +        pfn = extent.first_pfn;
> +        while ( pfn < max_pfn )

The max_ prefixes chosen are somewhat problematic with a loop
condition like this: "max" commonly means the maximum valid one
rather than the first following item. Perhaps "end_pfn" and just
"nr"?

>          {
> -            data->first_pfn += iter;
> -            data->nr -= iter;
> +            struct page_info *page;
> +            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> +
> +            if ( page )
> +            {
> +                mfn_t gmfn = _mfn(page_to_mfn(page));
> +
> +                paging_mark_dirty(d, gmfn);
> +                /*
> +                 * These are most probably not page tables any more
> +                 * don't take a long time and don't die either.
> +                 */
> +                sh_remove_shadows(d, gmfn, 1, 0);
> +                put_page(page);
> +            }
> +            pfn++;
> +        }
>  
> -            rc = -ERESTART;
> -            break;
> +        if ( max_nr < extent.nr )
> +        {
> +            extent.first_pfn += max_nr;
> +            extent.nr-= max_nr;
> +            if ( copy_to_guest_offset(buf->h, offset, &extent, 1) )
> +                return -EFAULT;

Is this copying back needed when not returning -ERESTART below?
I do realize that with the current code structure it's needed for the
copy_from above to read back the value, but even if this doesn't
look to be a double fetch vulnerability I think it would be better if
the value propagation would occur without going through guest
memory.

>          }
> -    }
>  
> -    return rc;
> +        /*
> +         * Check for continuation every 256th pfn and if the
> +         * pfn is not the last.
> +         */
> +
> +        if ( (rem_pfns == 0) && (offset <= header->nr_extents) )

DYM < instead of <= here?

> +        {
> +            if ( hypercall_preempt_check() )
> +            {
> +                header->offset = offset;
> +                rc = -ERESTART;
> +                break;
> +            }
> +            rem_pfns += max_pfns;

rem_pfns is zero prior to this anyway: Please use = instead of += .

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -237,13 +237,19 @@ struct xen_dm_op_set_pci_link_route {
>   * XEN_DMOP_modified_memory: Notify that a set of pages were modified by
>   *                           an emulator.
>   *
> - * NOTE: In the event of a continuation, the @first_pfn is set to the
> - *       value of the pfn of the remaining set of pages and @nr reduced
> - *       to the size of the remaining set.
> + * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
> + * nr_extent entries.
> + *
> + * On continuation, @offset is set to the next extent to be processed.
>   */
>  #define XEN_DMOP_modified_memory 11
>  
>  struct xen_dm_op_modified_memory {
> +    uint32_t nr_extents; /* IN - number of extents. */
> +    uint32_t offset;     /* Caller must set to 0. */

I think you should try to get away without this extra field: For the
purpose here, incrementing the handle value together with
decrementing nr_extents should be sufficient (just like done with
various other batchable hypercalls). And if the field was to stay,
"offset" is a bad name imo, as this leaves open whether this is
byte- or extent-granular. "cur_extent" perhaps?

Jan

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