Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp4164594ybg; Mon, 8 Jun 2020 00:14:43 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzN9/AoJ2onvc3v4YGh5nbswtiWahY6/0RiWEh1L45cuu5AH8du7aMl31XA5VO4XSrAA/7t X-Received: by 2002:aa7:c486:: with SMTP id m6mr18901002edq.234.1591600483237; Mon, 08 Jun 2020 00:14:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591600483; cv=none; d=google.com; s=arc-20160816; b=HWs5HbvLsfeXqhbnOau+qIQ6iCFe98LO38V2FvxH766XHRJho2CBlnFCvDh1kImTC+ aFx7RmuI+SKwRKtrtyDUqnlHrbcV0pE04Dm4jFmvoQOFrf3FJcfWvvxVya2m41m1Ayo8 8PDM+igUqep6MuBKKpuxq8FV486JHTc+2exC5EXwhkmPpogoYzhyKgDW2OKU7HfI6Amh /5ueTyu5IYqOmeZsdmaKaHKpgFx+pdiPkdl+CwUVb8o4nt5/zdzaa8MHEerNEocluEPx YMsm41ffBa/a5z4e15/jCJ9PCVFlGMpMuoWuePCWMhLJ1tZEo8sGFj1wuEZ0iUuvsVu9 uIWw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:to:references:message-id :content-transfer-encoding:cc:date:in-reply-to:from:subject :mime-version; bh=Q2PUiC/1pIYYzZbr+ZMFYSV2s5CNXGxVr95DkCxqhaQ=; b=p02LjkvW6BApBb7QKOduTNGzWhRy4SAcVUjJzax++5mU5dw+J1V+/WyuDq3ggJJfta u3bIG746hiH1CuaeXMygpgFtYsoYxOL2qzF4wuGptiKFraWSu3tUQXtbMqIdmXeToBu+ WYnBsPFMLNaemYqOrxhzu915h5YTrHWm7q7nZONG+wWafzUsAAyczRTtJKyTvnAeouxJ Rzw8js3WV9QDIaiH/tC4HhUHffCVbSz42UbkWbRjWeuF7tUF+iFFmfB8fogHvBV50b9L LBdp3a8HI37J7iZQBSve9RtQE1X4AUNPM9Wz4u27aNniiyeS7gWSLurbnttGmsfcRFTX lS4A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id e5si1187998edk.409.2020.06.08.00.14.20; Mon, 08 Jun 2020 00:14:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729067AbgFHHMW convert rfc822-to-8bit (ORCPT + 99 others); Mon, 8 Jun 2020 03:12:22 -0400 Received: from out30-42.freemail.mail.aliyun.com ([115.124.30.42]:58283 "EHLO out30-42.freemail.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728948AbgFHHMU (ORCPT ); Mon, 8 Jun 2020 03:12:20 -0400 X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R181e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e04426;MF=teawaterz@linux.alibaba.com;NM=1;PH=DS;RN=6;SR=0;TI=SMTPD_---0U-vT7af_1591600330; Received: from 127.0.0.1(mailfrom:teawaterz@linux.alibaba.com fp:SMTPD_---0U-vT7af_1591600330) by smtp.aliyun-inc.com(127.0.0.1); Mon, 08 Jun 2020 15:12:13 +0800 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [PATCH] virtio_mem: prevent overflow with subblock size From: teawater In-Reply-To: <0930c9d0-0708-c079-29bd-b80d4e3ce446@redhat.com> Date: Mon, 8 Jun 2020 15:12:09 +0800 Cc: "Michael S. Tsirkin" , LKML , Jason Wang , Pankaj Gupta , virtualization@lists.linux-foundation.org Content-Transfer-Encoding: 8BIT Message-Id: <2D9A07BA-6FDC-48FF-9A1F-62272695B3EF@linux.alibaba.com> References: <20200608061406.709211-1-mst@redhat.com> <0930c9d0-0708-c079-29bd-b80d4e3ce446@redhat.com> To: David Hildenbrand X-Mailer: Apple Mail (2.3608.80.23.2.2) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 2020年6月8日 14:58,David Hildenbrand 写道: > > On 08.06.20 08:14, Michael S. Tsirkin wrote: >> If subblock size is large (e.g. 1G) 32 bit math involving it >> can overflow. Rather than try to catch all instances of that, >> let's tweak block size to 64 bit. > > I fail to see where we could actually trigger an overflow. The reported > warning looked like a false positive to me. > >> >> It ripples through UAPI which is an ABI change, but it's not too late to >> make it, and it will allow supporting >4Gbyte blocks while might >> become necessary down the road. >> > > This might break cloud-hypervisor, who's already implementing this > protocol upstream (ccing Hui). > https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs > > (blocks in the gigabyte range were never the original intention of > virtio-mem, but I am not completely opposed to that) If you think virtio_mem need this patch, I think cloud-hypervisor should follow this update (I will post PR for it). Best, Hui > >> Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug") >> Signed-off-by: Michael S. Tsirkin >> --- >> drivers/virtio/virtio_mem.c | 14 +++++++------- >> include/uapi/linux/virtio_mem.h | 4 ++-- >> 2 files changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c >> index 2f357142ea5e..7b1bece8a331 100644 >> --- a/drivers/virtio/virtio_mem.c >> +++ b/drivers/virtio/virtio_mem.c >> @@ -77,7 +77,7 @@ struct virtio_mem { >> uint64_t requested_size; >> >> /* The device block size (for communicating with the device). */ >> - uint32_t device_block_size; >> + uint64_t device_block_size; >> /* The translated node id. NUMA_NO_NODE in case not specified. */ >> int nid; >> /* Physical start address of the memory region. */ >> @@ -86,7 +86,7 @@ struct virtio_mem { >> uint64_t region_size; >> >> /* The subblock size. */ >> - uint32_t subblock_size; >> + uint64_t subblock_size; >> /* The number of subblocks per memory block. */ >> uint32_t nb_sb_per_mb; >> >> @@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm) >> * - At least the device block size. >> * In the worst case, a single subblock per memory block. >> */ >> - vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1, >> - pageblock_order); >> - vm->subblock_size = max_t(uint32_t, vm->device_block_size, >> + vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1, >> + pageblock_order); >> + vm->subblock_size = max_t(uint64_t, vm->device_block_size, >> vm->subblock_size); >> vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size; >> >> @@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm) >> >> dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr); >> dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size); >> - dev_info(&vm->vdev->dev, "device block size: 0x%x", >> - vm->device_block_size); >> + dev_info(&vm->vdev->dev, "device block size: 0x%llx", >> + (unsigned long long)vm->device_block_size); >> dev_info(&vm->vdev->dev, "memory block size: 0x%lx", >> memory_block_size_bytes()); >> dev_info(&vm->vdev->dev, "subblock size: 0x%x", >> diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h >> index a455c488a995..a9ffe041843c 100644 >> --- a/include/uapi/linux/virtio_mem.h >> +++ b/include/uapi/linux/virtio_mem.h >> @@ -185,10 +185,10 @@ struct virtio_mem_resp { >> >> struct virtio_mem_config { >> /* Block size and alignment. Cannot change. */ >> - __u32 block_size; >> + __u64 block_size; >> /* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */ >> __u16 node_id; >> - __u16 padding; >> + __u8 padding[6]; >> /* Start address of the memory region. Cannot change. */ >> __u64 addr; >> /* Region size (maximum). Cannot change. */ >> > > > -- > Thanks, > > David / dhildenb