From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39240) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJhyC-0005Sh-29 for qemu-devel@nongnu.org; Fri, 06 Feb 2015 07:25:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJhy8-0001x1-T4 for qemu-devel@nongnu.org; Fri, 06 Feb 2015 07:25:12 -0500 Received: from mx2.parallels.com ([199.115.105.18]:48514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJhy8-0001wh-Nl for qemu-devel@nongnu.org; Fri, 06 Feb 2015 07:25:08 -0500 Message-ID: <54D4B296.10603@openvz.org> Date: Fri, 6 Feb 2015 15:24:54 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1423220051-16058-1-git-send-email-pl@kamp.de> <1423221883-16804-1-git-send-email-den@openvz.org> <20150206115347.GB13081@noname.redhat.com> <54D4AC9C.9000005@openvz.org> <20150206120745.GC13081@noname.redhat.com> <54D4B0D6.4020402@openvz.org> <54D4B1F9.4020906@kamp.de> In-Reply-To: <54D4B1F9.4020906@kamp.de> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] nbd: fix max_discard/max_transfer_length List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , Kevin Wolf Cc: qemu-devel@nongnu.org On 06/02/15 15:22, Peter Lieven wrote: > Am 06.02.2015 um 13:17 schrieb Denis V. Lunev: >> On 06/02/15 15:07, Kevin Wolf wrote: >>> Am 06.02.2015 um 12:59 hat Denis V. Lunev geschrieben: >>>> On 06/02/15 14:53, Kevin Wolf wrote: >>>>> Am 06.02.2015 um 12:24 hat Denis V. Lunev geschrieben: >>>>>> nbd_co_discard calls nbd_client_session_co_discard which uses uint32_t >>>>>> as the length in bytes of the data to discard due to the following >>>>>> definition: >>>>>> >>>>>> struct nbd_request { >>>>>> uint32_t magic; >>>>>> uint32_t type; >>>>>> uint64_t handle; >>>>>> uint64_t from; >>>>>> uint32_t len; <-- the length of data to be discarded, in bytes >>>>>> } QEMU_PACKED; >>>>>> >>>>>> Thus we should limit bl_max_discard to UINT32_MAX >> BDRV_SECTOR_BITS to >>>>>> avoid overflow. >>>>>> >>>>>> NBD read/write code uses the same structure for transfers. Fix >>>>>> max_transfer_length accordingly. >>>>>> >>>>>> Signed-off-by: Denis V. Lunev >>>>>> CC: Peter Lieven >>>>>> CC: Kevin Wolf >>>>> Thanks, I have applied both Peter's and your patch. Can you guys please >>>>> check whether the current state of my block branch is correct or whether >>>>> I forgot to include or remove some patch? >>>> can you give me tree URL? >>> Sure: >>> >>> git: git://repo.or.cz/qemu/kevin.git block >>> Web: http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/block >>> >>>>> By the way, I don't think this NBD patch is strictly necessary as you'll >>>>> have a hard time finding a platform where INT_MAX > UINT32_MAX, but I >>>>> think it's good documentation at least and a safeguard if we ever decide >>>>> to lift the general block layer restrictions. >>>>> >>>>> Kevin >>>> nope, it is absolutely mandatory >>>> >>>> stdint.h: >>>> >>>> /* Limit of `size_t' type. */ >>>> # if __WORDSIZE == 64 >>>> # define SIZE_MAX (18446744073709551615UL) >>>> # else >>>> # define SIZE_MAX (4294967295U) >>>> # endif >>> But Peter defined it like this: >>> >>> #define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \ >>> INT_MAX >> BDRV_SECTOR_BITS) >>> >>> And having integers with more the 32 bits is at least unusual. I don't >>> know of any platform that has them. >>> >>> Anyway, as I said, your patch is good documentation, so I'm happy to >>> apply it nevertheless. >>> >>> Kevin >> I have misinterpreted this. >> >> Actually I think then the limit should be MAX() rather then MIN() >> as the stack is ready to size_t transfers. In the other case >> there is no need at all to use this construction. INT_MAX will be >> always less than SIZE_MAX. I do not know any platform >> where this is violated. > That doesn't work. All internal routines have int (32-bit) as type for nb_sectors > whereas size_t is often 64-bit. > > I also think that INT_MAX is always less than SIZE_MAX, but I would leave it > in just to be absolutely sure. Its evaluated at compile time and will not > hurt. > > Peter > OK :) let it be. Staying on safe side is always good. Kevin, all my stuff we have agreed on is applied. Regards, Den