> -----Original Message-----
> From: Juergen Gross [mailto:jgr...@suse.com]
> Sent: 13 September 2017 09:58
> To: Paul Durrant <paul.durr...@citrix.com>; xen-devel@lists.xen.org
> Cc: sstabell...@kernel.org; Wei Liu <wei.l...@citrix.com>; George Dunlap
> <george.dun...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; julien.gr...@arm.com; jbeul...@suse.com;
> dgde...@tycho.nsa.gov
> 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-boun...@lists.xen.org] On Behalf Of
> >> Juergen Gross
> >> Sent: 13 September 2017 08:47
> >> To: xen-devel@lists.xen.org
> >> Cc: Juergen Gross <jgr...@suse.com>; sstabell...@kernel.org; Wei Liu
> >> <wei.l...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> >> Andrew Cooper <andrew.coop...@citrix.com>; Ian Jackson
> >> <ian.jack...@citrix.com>; Tim (Xen.org) <t...@xen.org>;
> >> julien.gr...@arm.com; jbeul...@suse.com; dgde...@tycho.nsa.gov
> >> 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.durr...@citrix.com>
>
>
> Juergen
>
> >
> > Paul
> >
> >>
> >> While at it correct coding style: labels should be indented by one
> >> space.
> >>
> >> Signed-off-by: Juergen Gross <jgr...@suse.com>
> >> ---
> >> 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@lists.xen.org
> >> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
