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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Juergen Gross [mailto:jgross@xxxxxxxx]
> Sent: 13 September 2017 09:58
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: 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: Re: [Xen-devel] [PATCH v6 01/12] xen: correct
> gnttab_get_status_frames()
> 
> 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.

Ok. I suspect that was cut'n'paste from nr_grant_fames() but I see no 
particular harm in the increased scope since it's a read lock.

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx> 

> 
> 
> 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