From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [BUGFIX][PATCH 3/4] hvm_save_one: return correct data. Date: Mon, 16 Dec 2013 12:51:13 -0500 Message-ID: <52AF3D91.6000809@terremark.com> References: <1386809777-12898-1-git-send-email-dslutz@terremark.com> <1386809777-12898-4-git-send-email-dslutz@terremark.com> <52AB25B4020000780010D0B0@nat28.tlf.novell.com> <52ACF7CE.9030904@terremark.com> <52ADDE15.8010408@citrix.com> <52AEC522020000780010D7BE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------090206060402060701040208" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VscLe-0005CD-BI for xen-devel@lists.xenproject.org; Mon, 16 Dec 2013 17:52:54 +0000 In-Reply-To: <52AEC522020000780010D7BE@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Andrew Cooper , Don Slutz Cc: xen-devel , Keir Fraser , Ian Jackson , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org --------------090206060402060701040208 Content-Type: multipart/alternative; boundary="------------070109020108060901030204" --------------070109020108060901030204 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 12/16/13 03:17, Jan Beulich wrote: >>>> On 15.12.13 at 17:51, Andrew Cooper wrote: >> On 15/12/2013 00:29, Don Slutz wrote: >>> I think I have corrected all coding errors (please check again). And >>> done all requested changes. I did add the reviewed by (not sure if I >>> should since this changes a large part of the patch, but they are all >>> what Jan said). >>> >>> I have unit tested it and it appears to work the same as the previous >>> version (as expected). >>> >>> Here is the new version, also attached. >>> >>> From e0e8f5246ba492b153884cea93bfe753f1b0782e Mon Sep 17 00:00:00 2001 >>> From: Don Slutz >>> Date: Tue, 12 Nov 2013 08:22:53 -0500 >>> Subject: [PATCH v2 3/4] hvm_save_one: return correct data. >>> >>> It is possible that hvm_sr_handlers[typecode].save does not use all >>> the provided room. In that case, using: >>> >>> instance * hvm_sr_handlers[typecode].size >>> >>> does not select the correct instance. Add code to search for the >>> correct instance. >>> >>> Signed-off-by: Don Slutz >>> Reviewed-by: Jan Beulich >> but this fairs no better at selecting the correct subset in the case >> that less data than hvm_sr_handlers[typecode].size is written by >> hvm_sr_handlers[typecode].save. > Oh, yes, indeed. > >> It always increments by 'size' bytes, and will only copy the data back >> if the bytes under desc->instance happen to match the instance we are >> looking for. >> >> The only solution I can see is that for the per-vcpu records, the save >> functions get refactored to take an instance ID, and only save their >> specific instance. > I don't see why you shouldn't be able to look at the descriptor > instead - that one does have the correct size, doesn't it? > > Jan > Attached is v3 of this. It is basically a merge of patch #3 and patch #4 with cleanups. This is what I said in: http://lists.xen.org/archives/html/xen-devel/2013-12/msg02216.html and Andrew replied in: http://lists.xen.org/archives/html/xen-devel/2013-12/msg02217.html and the RFC is: http://lists.xen.org/archives/html/xen-devel/2013-12/msg02223.html to which: http://lists.xen.org/archives/html/xen-devel/2013-12/msg02270.html (from this): IMHO this is obviously not 4.4 material at this stage. Apart from anything else we've been managing to release with these short comings for many years. Indeed. -George I feel that the attached bugfix patch is simple enough to make it into 4.4 and also be back ported to stable branches. -Don Slutz --------------070109020108060901030204 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 12/16/13 03:17, Jan Beulich wrote:
On 15.12.13 at 17:51, Andrew Cooper <andrew.cooper3@citrix.com> wrote:

On 15/12/2013 00:29, Don Slutz wrote:

I think I have corrected all coding errors (please check again). And
done all requested changes.  I did add the reviewed by (not sure if I
should since this changes a large part of the patch, but they are all
what Jan said).

I have unit tested it and it appears to work the same as the previous
version (as expected).

Here is the new version, also attached.

>>From e0e8f5246ba492b153884cea93bfe753f1b0782e Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Tue, 12 Nov 2013 08:22:53 -0500
Subject: [PATCH v2 3/4] hvm_save_one: return correct data.

It is possible that hvm_sr_handlers[typecode].save does not use all
the provided room.  In that case, using:

   instance * hvm_sr_handlers[typecode].size

does not select the correct instance.  Add code to search for the
correct instance.

Signed-off-by: Don Slutz <dslutz@verizon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

but this fairs no better at selecting the correct subset in the case
that less data than hvm_sr_handlers[typecode].size is written by
hvm_sr_handlers[typecode].save.

Oh, yes, indeed.


It always increments by 'size' bytes, and will only copy the data back
if the bytes under desc->instance happen to match the instance we are
looking for.

The only solution I can see is that for the per-vcpu records, the save
functions get refactored to take an instance ID, and only save their
specific instance.

I don't see why you shouldn't be able to look at the descriptor
instead - that one does have the correct size, doesn't it?

Jan
Attached is v3 of this.  It is basically a merge of patch #3 and patch #4 with cleanups.

This is what I said in:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02216.html

and Andrew replied in:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02217.html

and the RFC is:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02223.html

to which:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02270.html

(from this):
IMHO this is obviously not 4.4 material at this stage. Apart from
anything else we've been managing to release with these short comings
for many years.
Indeed. -George
I feel that the attached bugfix patch is simple enough to make it into 4.4 and also be back ported to stable branches.

   -Don Slutz





--------------070109020108060901030204-- --------------090206060402060701040208 Content-Type: text/x-patch; name="0001-hvm_save_one-return-correct-data.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-hvm_save_one-return-correct-data.patch" >>From 07897bd0d4a680df03421c0eab96cfa41de2d9f6 Mon Sep 17 00:00:00 2001 From: Don Slutz Date: Tue, 12 Nov 2013 08:22:53 -0500 Subject: [BUGFIX][PATCH v3 1/1] hvm_save_one: return correct data. It is possible that hvm_sr_handlers[typecode].save does not use all the provided room. Also it can use variable sized records. In both cases, using: instance * hvm_sr_handlers[typecode].size does not select the correct instance. Add code to search for the correct instance. Signed-off-by: Don Slutz --- changes v2 to v3: merge in patch #4. changes v1 to v2: fix coding style and coding issues. xen/common/hvm/save.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c index de76ada..a7e0edc 100644 --- a/xen/common/hvm/save.c +++ b/xen/common/hvm/save.c @@ -98,9 +98,6 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance, else sz = hvm_sr_handlers[typecode].size; - if ( (instance + 1) * hvm_sr_handlers[typecode].size > sz ) - return -EINVAL; - ctxt.size = sz; ctxt.data = xmalloc_bytes(sz); if ( !ctxt.data ) @@ -112,13 +109,26 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance, d->domain_id, typecode); rv = -EFAULT; } - else if ( copy_to_guest(handle, - ctxt.data - + (instance * hvm_sr_handlers[typecode].size) - + sizeof (struct hvm_save_descriptor), - hvm_sr_handlers[typecode].size - - sizeof (struct hvm_save_descriptor)) ) - rv = -EFAULT; + else + { + uint32_t off; + struct hvm_save_descriptor *desc; + + rv = -EBADSLT; + for ( off = 0; off < ctxt.cur; off += desc->length ) + { + desc = (void *)ctxt.data + off; + /* Move past header */ + off += sizeof(*desc); + if ( instance == desc->instance ) + { + rv = 0; + if ( copy_to_guest(handle, ctxt.data + off, desc->length) ) + rv = -EFAULT; + break; + } + } + } xfree(ctxt.data); return rv; -- 1.8.4 --------------090206060402060701040208 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------090206060402060701040208--