From patchwork Fri Oct 21 14:37:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Halil Pasic X-Patchwork-Id: 9389367 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 51426607D0 for ; Fri, 21 Oct 2016 14:38:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 425C22A26B for ; Fri, 21 Oct 2016 14:38:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 35D7B2A26D; Fri, 21 Oct 2016 14:38:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id B73942A26B for ; Fri, 21 Oct 2016 14:38:32 +0000 (UTC) Received: from localhost ([::1]:32780 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxaxs-0000mG-1E for patchwork-qemu-devel@patchwork.kernel.org; Fri, 21 Oct 2016 10:38:32 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxaxL-0000k3-SK for qemu-devel@nongnu.org; Fri, 21 Oct 2016 10:38:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxaxH-0006QJ-T7 for qemu-devel@nongnu.org; Fri, 21 Oct 2016 10:37:59 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:36821) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxaxH-0006Pm-LW for qemu-devel@nongnu.org; Fri, 21 Oct 2016 10:37:55 -0400 Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u9LEYOt6133983 for ; Fri, 21 Oct 2016 10:37:54 -0400 Received: from e06smtp07.uk.ibm.com (e06smtp07.uk.ibm.com [195.75.94.103]) by mx0a-001b2d01.pphosted.com with ESMTP id 267er62skv-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 21 Oct 2016 10:37:54 -0400 Received: from localhost by e06smtp07.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 21 Oct 2016 15:37:52 +0100 Received: from d06dlp02.portsmouth.uk.ibm.com (9.149.20.14) by e06smtp07.uk.ibm.com (192.168.101.137) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 21 Oct 2016 15:37:50 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id E72B0219004D for ; Fri, 21 Oct 2016 15:37:05 +0100 (BST) Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u9LEbnVb24510630 for ; Fri, 21 Oct 2016 14:37:49 GMT Received: from d06av06.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u9LEbm7H023195 for ; Fri, 21 Oct 2016 10:37:49 -0400 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u9LEbl8Z023096 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO); Fri, 21 Oct 2016 10:37:48 -0400 From: Halil Pasic To: qemu-devel@nongnu.org Date: Fri, 21 Oct 2016 16:37:40 +0200 X-Mailer: git-send-email 2.8.4 In-Reply-To: <20161021143741.8597-1-pasic@linux.vnet.ibm.com> References: <20161021143741.8597-1-pasic@linux.vnet.ibm.com> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16102114-0028-0000-0000-0000023678AD X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16102114-0029-0000-0000-000020E70622 Message-Id: <20161021143741.8597-4-pasic@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-10-21_09:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1610210265 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 148.163.156.1 Subject: [Qemu-devel] [RFC PATCH 3/4] migration/vmstate: fix array of pointers to struct X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Amit Shah , Halil Pasic , Guenther Hutzl , "Dr. David Alan Gilbert" , Juan Quintela Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the reward for trying to migrate an array with some null pointers in it was an illegal memory access, that is a swift and painless death of the process. Let's make vmstate cope with this scenario at least for pointers to structs. The general approach is when we encounter a null pointer (element) instead of following the pointer to save/load the data behind it we save/load a placeholder. This way we can detect if we expected a null pointer at the load side but not null data was saved instead. Sadly all other error scenarios are not detected by this scheme (and would require the usage of the JSON meta data). Limitations: Does not work for pointers to primitives. Signed-off-by: Halil Pasic Reviewed-by: Guenther Hutzl --- We will need this to load/save some on demand created state from within the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an example). I'm not sure about some asserts I introduced. There may be a better way to handle these conditions (like returning an error code in load for example). --- include/migration/vmstate.h | 2 + migration/vmstate.c | 91 ++++++++++++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 1638ee5..1e0c71c 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -236,6 +236,7 @@ extern const VMStateInfo vmstate_info_uint8; extern const VMStateInfo vmstate_info_uint16; extern const VMStateInfo vmstate_info_uint32; extern const VMStateInfo vmstate_info_uint64; +extern const VMStateInfo vmstate_info_nullptr; extern const VMStateInfo vmstate_info_float64; extern const VMStateInfo vmstate_info_cpudouble; @@ -454,6 +455,7 @@ extern const VMStateInfo vmstate_info_bitmap; .size = sizeof(_type *), \ .flags = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER, \ .offset = vmstate_offset_array(_s, _f, _type*, _n), \ + .info = &vmstate_info_nullptr, \ } #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, _vmsd, _type) { \ diff --git a/migration/vmstate.c b/migration/vmstate.c index 0bc9f35..1e65a93 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -46,33 +46,18 @@ static int vmstate_size(void *opaque, VMStateField *field) size *= field->size; } } - return size; } -static void *vmstate_base_addr(void *opaque, VMStateField *field, bool alloc) +static void vmstate_handle_alloc(void *ptr, VMStateField *field, void *opaque) { - void *base_addr = opaque + field->offset; - - if (field->flags & VMS_POINTER) { - if (alloc && (field->flags & VMS_ALLOC)) { - gsize size = 0; - if (field->flags & VMS_VBUFFER) { - size = vmstate_size(opaque, field); - } else { - int n_elems = vmstate_n_elems(opaque, field); - if (n_elems) { - size = n_elems * field->size; - } - } - if (size) { - *((void **)base_addr + field->start) = g_malloc(size); - } + if (field->flags & VMS_POINTER && field->flags & VMS_ALLOC) { + gsize size = vmstate_size(opaque, field); + size *= vmstate_n_elems(opaque, field); + if (size) { + *(void **)ptr = g_malloc(size); } - base_addr = *(void **)base_addr + field->start; } - - return base_addr; } int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, @@ -108,21 +93,30 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, field->field_exists(opaque, version_id)) || (!field->field_exists && field->version_id <= version_id)) { - void *base_addr = vmstate_base_addr(opaque, field, true); + void *first_elem = opaque + field->offset; int i, n_elems = vmstate_n_elems(opaque, field); int size = vmstate_size(opaque, field); + vmstate_handle_alloc(first_elem, field, opaque); + if (field->flags & VMS_POINTER) { + first_elem = *(void **)first_elem; + assert(first_elem); + } for (i = 0; i < n_elems; i++) { - void *addr = base_addr + size * i; + void *curr_elem = first_elem + size * i; if (field->flags & VMS_ARRAY_OF_POINTER) { - addr = *(void **)addr; + curr_elem = *(void **)curr_elem; } - if (field->flags & VMS_STRUCT) { - ret = vmstate_load_state(f, field->vmsd, addr, + if (!curr_elem) { + /* if null pointer check placeholder and do not follow */ + assert(field->flags & VMS_ARRAY_OF_POINTER); + vmstate_info_nullptr.get(f, curr_elem, size); + } else if (field->flags & VMS_STRUCT) { + ret = vmstate_load_state(f, field->vmsd, curr_elem, field->vmsd->version_id); } else { - ret = field->info->get(f, addr, size); + ret = field->info->get(f, curr_elem, size); } if (ret >= 0) { @@ -312,25 +306,33 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, while (field->name) { if (!field->field_exists || field->field_exists(opaque, vmsd->version_id)) { - void *base_addr = vmstate_base_addr(opaque, field, false); + void *first_elem = opaque + field->offset; int i, n_elems = vmstate_n_elems(opaque, field); int size = vmstate_size(opaque, field); int64_t old_offset, written_bytes; QJSON *vmdesc_loop = vmdesc; + if (field->flags & VMS_POINTER) { + first_elem = *(void **)first_elem; + assert(first_elem); + } for (i = 0; i < n_elems; i++) { - void *addr = base_addr + size * i; + void *curr_elem = first_elem + size * i; vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems); old_offset = qemu_ftell_fast(f); - if (field->flags & VMS_ARRAY_OF_POINTER) { - addr = *(void **)addr; + assert(curr_elem); + curr_elem = *(void **)curr_elem; } - if (field->flags & VMS_STRUCT) { - vmstate_save_state(f, field->vmsd, addr, vmdesc_loop); + if (!curr_elem) { + /* if null pointer write placeholder and do not follow */ + assert(field->flags & VMS_ARRAY_OF_POINTER); + vmstate_info_nullptr.put(f, curr_elem, size); + } else if (field->flags & VMS_STRUCT) { + vmstate_save_state(f, field->vmsd, curr_elem, vmdesc_loop); } else { - field->info->put(f, addr, size); + field->info->put(f, curr_elem, size); } written_bytes = qemu_ftell_fast(f) - old_offset; @@ -720,6 +722,27 @@ const VMStateInfo vmstate_info_uint64 = { .put = put_uint64, }; +static int get_nullptr(QEMUFile *f, void *pv, size_t size) +{ + int8_t tmp; + qemu_get_s8s(f, &tmp); + assert(tmp == 0); + return 0; +} + +static void put_nullptr(QEMUFile *f, void *pv, size_t size) +{ + int8_t tmp = 0; + assert(pv == NULL); + qemu_put_s8s(f, &tmp); +} + +const VMStateInfo vmstate_info_nullptr = { + .name = "uint64", + .get = get_nullptr, + .put = put_nullptr, +}; + /* 64 bit unsigned int. See that the received value is the same than the one in the field */