Re: [Xen-devel] [PATCH v6 01/12] xen: correct gnttab_get_status_frames()

[Juergen Gross <jgross@xxxxxxxx>]

On 13/09/17 18:01, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
>> Juergen Gross
>> Sent: 13 September 2017 08:47
>> To: xen-devel@xxxxxxxxxxxxx
>> Cc: Juergen Gross <jgross@xxxxxxxx>; sstabellini@xxxxxxxxxx; Wei Liu
>> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
>> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
>> <Ian.Jackson@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>;
>> julien.grall@xxxxxxx; jbeulich@xxxxxxxx; dgdegra@xxxxxxxxxxxxx
>> Subject: [Xen-devel] [PATCH v6 01/12] xen: correct
>> gnttab_get_status_frames()
>>
>> In gnttab_get_status_frames() all accesses to nr_status_frames should
>> be done with the grant table lock held.
> 
> Is this true? The value can only increase so what does the increase lock 
> scope actually protect against?

The comment above nr_status_frames() says so. Either the comment or the
code is wrong.


Juergen

> 
>   Paul
> 
>>
>> While at it correct coding style: labels should be indented by one
>> space.
>>
>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>> ---
>>  xen/common/grant_table.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index c3895e6201..00ff075bd9 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -2866,19 +2866,19 @@
>> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
>> s_frames_t) uop,
>>
>>      gt = d->grant_table;
>>
>> +    op.status = GNTST_okay;
>> +
>> +    grant_read_lock(gt);
>> +
>>      if ( unlikely(op.nr_frames > nr_status_frames(gt)) )
>>      {
>>          gdprintk(XENLOG_INFO, "Guest requested addresses for %d grant
>> status "
>>                   "frames, but only %d are available.\n",
>>                   op.nr_frames, nr_status_frames(gt));
>>          op.status = GNTST_general_error;
>> -        goto out2;
>> +        goto unlock;
>>      }
>>
>> -    op.status = GNTST_okay;
>> -
>> -    grant_read_lock(gt);
>> -
>>      for ( i = 0; i < op.nr_frames; i++ )
>>      {
>>          gmfn = gnttab_status_gmfn(d, gt, i);
>> @@ -2886,10 +2886,11 @@
>> gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_statu
>> s_frames_t) uop,
>>              op.status = GNTST_bad_virt_addr;
>>      }
>>
>> + unlock:
>>      grant_read_unlock(gt);
>> -out2:
>> + out2:
>>      rcu_unlock_domain(d);
>> -out1:
>> + out1:
>>      if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
>>          return -EFAULT;
>>
>> --
>> 2.12.3
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel


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