| Message ID | 20170913154651.2366-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Juergen Gross > Sent: 13 September 2017 08:47 > To: xen-devel@lists.xen.org > Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu > <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; > Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; > julien.grall@arm.com; jbeulich@suse.com; dgdegra@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? Paul > > While at it correct coding style: labels should be indented by one > space. > > Signed-off-by: Juergen Gross <jgross@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
On 13/09/17 18:01, Paul Durrant wrote: >> -----Original Message----- >> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of >> Juergen Gross >> Sent: 13 September 2017 08:47 >> To: xen-devel@lists.xen.org >> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu >> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; >> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson >> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; >> julien.grall@arm.com; jbeulich@suse.com; dgdegra@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. Juergen > > Paul > >> >> While at it correct coding style: labels should be indented by one >> space. >> >> Signed-off-by: Juergen Gross <jgross@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
> -----Original Message----- > From: Juergen Gross [mailto:jgross@suse.com] > Sent: 13 September 2017 09:58 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xen.org > Cc: sstabellini@kernel.org; Wei Liu <wei.liu2@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > (Xen.org) <tim@xen.org>; julien.grall@arm.com; jbeulich@suse.com; > dgdegra@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-bounces@lists.xen.org] On Behalf Of > >> Juergen Gross > >> Sent: 13 September 2017 08:47 > >> To: xen-devel@lists.xen.org > >> Cc: Juergen Gross <jgross@suse.com>; sstabellini@kernel.org; Wei Liu > >> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; > >> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson > >> <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; > >> julien.grall@arm.com; jbeulich@suse.com; dgdegra@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.durrant@citrix.com> > > > Juergen > > > > > Paul > > > >> > >> While at it correct coding style: labels should be indented by one > >> space. > >> > >> Signed-off-by: Juergen Gross <jgross@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
On Wed, Sep 13, 2017 at 05:46:40PM +0200, Juergen Gross wrote: > In gnttab_get_status_frames() all accesses to nr_status_frames should > be done with the grant table lock held. > > While at it correct coding style: labels should be indented by one > space. > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
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_status_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_status_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;
In gnttab_get_status_frames() all accesses to nr_status_frames should be done with the grant table lock held. While at it correct coding style: labels should be indented by one space. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/grant_table.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)