Re: [Qemu-devel] [PATCH v2 for-xen-4.5] xen_disk: fix unmapping of persistent grants

["Roger Pau Monné" <roger.pau@citrix.com>]

El 13/11/14 a les 16.36, Stefano Stabellini ha escrit:
> On Thu, 13 Nov 2014, Roger Pau Monne wrote:
>> @@ -421,7 +451,17 @@ static int ioreq_map(struct ioreq *ioreq)
>>              }
>>          }
>>      }
>> -    if (ioreq->blkdev->feature_persistent) {
>> +    if (ioreq->blkdev->feature_persistent && new_maps &&
>> +        (!batch_maps || (ioreq->blkdev->persistent_gnt_count + new_maps <=
>> +        ioreq->blkdev->max_grants))) {
> 
> Do we really need this change? If so, could you please write something
> about it in the commit message?

Ack.

> 
>> +        /*
>> +         * If we are using persistent grants and batch mappings only
>> +         * add the new maps to the list of persistent grants if the whole
>> +         * area can be persistently mapped.
>> +         */
> 
> Is this the reason for the change above?

Yes, this is the reason. Maybe it's not clear enough. If we are using
batch maps we have to map all the region persistently, we cannot cherry
pick single grants and make them persistent.

> 
>> +        if (batch_maps && new_maps) {
>> +            add_persistent_region(ioreq->blkdev, ioreq->pages, new_maps);
>> +        }
>>
>>          while ((ioreq->blkdev->persistent_gnt_count < ioreq->blkdev->max_grants)
>>                && new_maps) {
>>              /* Go through the list of newly mapped grants and add as many
>> @@ -437,6 +477,7 @@ static int ioreq_map(struct ioreq *ioreq)
>>                  grant->page = ioreq->pages + (new_maps) * XC_PAGE_SIZE;
>>              } else {
>>                  grant->page = ioreq->page[new_maps];
>> +                add_persistent_region(ioreq->blkdev, ioreq->page[new_maps], 1);
> 
> Basically in the !batch_maps case we are duplicating persistent_gnts
> into persistent_regions. Couldn't we just avoid calling
> add_persistent_region at all in that case and rely on the old
> destroy_grant function? In fact I think we could just pass NULL as
> GDestroyNotify function when creating persistent_gnts if batch_maps and
> the old unmodified destroy_grant if !batch_maps.

Not exactly, we still need the GDestroyNotify function in the batch_maps
case, but I could just pass g_free directly. Let me send v3 with this
reworked.

> 
>>              }
>>              grant->blkdev = ioreq->blkdev;
>>              xen_be_printf(&ioreq->blkdev->xendev, 3,
>> @@ -447,6 +488,7 @@ static int ioreq_map(struct ioreq *ioreq)
>>                            grant);
>>              ioreq->blkdev->persistent_gnt_count++;
>>          }
>> +        assert(!batch_maps || new_maps == 0);
> 
> Shouldn't new_maps be == 0 even in the !batch_maps case?

In theory yes, because we can persistently map all the grants that can
be on the ring. But a malicious/badly coded blkfront could try to
exploit this by always passing different grants, in which case we would
not be able to map them all persistently.

Roger.