| Message ID | 1423221883-16804-1-git-send-email-den@openvz.org |
|---|---|
| State | New |
| Headers | show |
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 <den@openvz.org> > CC: Peter Lieven <pl@kamp.de> > CC: Kevin Wolf <kwolf@redhat.com> 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? 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
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 <den@openvz.org> >> CC: Peter Lieven <pl@kamp.de> >> CC: Kevin Wolf <kwolf@redhat.com> > 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? > 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 Den
Am 06.02.2015 um 12:59 schrieb Denis V. Lunev: > 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 <den@openvz.org> >>> CC: Peter Lieven <pl@kamp.de> >>> CC: Kevin Wolf <kwolf@redhat.com> >> 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? > >> 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 > > Den Yes, but we limit to MIN(SIZE_MAX >> BDRV_SECTOR_BITS, INT_MAX >> BDRV_SECTOR_BITS) anyway. I do not know if there is a platform where INT_MAX is 2^63 - 1 and not 2^31 - 1 ? We can keep Dens patch in. Just in case. It doesn't hurt. Peter
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 <den@openvz.org> > >>CC: Peter Lieven <pl@kamp.de> > >>CC: Kevin Wolf <kwolf@redhat.com> > >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
Am 06.02.2015 um 12:53 schrieb Kevin Wolf: > 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 <den@openvz.org> >> CC: Peter Lieven <pl@kamp.de> >> CC: Kevin Wolf <kwolf@redhat.com> > 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? Looks good from my point of view. Just to be sure has it to be if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) or if (size > (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)) ? If the latter is right, can you please fix that line in my patch. I am afk now. Thanks, Peter
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 <den@openvz.org> >>>> CC: Peter Lieven <pl@kamp.de> >>>> CC: Kevin Wolf <kwolf@redhat.com> >>> 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. Den
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 <den@openvz.org> >>>>> CC: Peter Lieven <pl@kamp.de> >>>>> CC: Kevin Wolf <kwolf@redhat.com> >>>> 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
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 <den@openvz.org> >>>>>> CC: Peter Lieven <pl@kamp.de> >>>>>> CC: Kevin Wolf <kwolf@redhat.com> >>>>> 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
Am 06.02.2015 um 13:16 hat Peter Lieven geschrieben: > Am 06.02.2015 um 12:53 schrieb Kevin Wolf: > > 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 <den@openvz.org> > >> CC: Peter Lieven <pl@kamp.de> > >> CC: Kevin Wolf <kwolf@redhat.com> > > 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? > > Looks good from my point of view. > > Just to be sure has it to be > > if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) > > or > > if (size > (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)) > > ? > > If the latter is right, can you please fix that line in my patch. I am afk now. Both versions are correct. Kevin
diff --git a/block/nbd.c b/block/nbd.c index 04cc845..dda0b0b 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -301,6 +301,12 @@ static int nbd_co_flush(BlockDriverState *bs) return nbd_client_session_co_flush(&s->client); } +static void nbd_refresh_limits(BlockDriverState *bs, Error **errp) +{ + bs->bl.max_discard = UINT32_MAX >> BDRV_SECTOR_BITS; + bs->bl.max_transfer_length = UINT32_MAX >> BDRV_SECTOR_BITS; +} + static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors) { @@ -396,6 +402,7 @@ static BlockDriver bdrv_nbd = { .bdrv_close = nbd_close, .bdrv_co_flush_to_os = nbd_co_flush, .bdrv_co_discard = nbd_co_discard, + .bdrv_refresh_limits = nbd_refresh_limits, .bdrv_getlength = nbd_getlength, .bdrv_detach_aio_context = nbd_detach_aio_context, .bdrv_attach_aio_context = nbd_attach_aio_context, @@ -413,6 +420,7 @@ static BlockDriver bdrv_nbd_tcp = { .bdrv_close = nbd_close, .bdrv_co_flush_to_os = nbd_co_flush, .bdrv_co_discard = nbd_co_discard, + .bdrv_refresh_limits = nbd_refresh_limits, .bdrv_getlength = nbd_getlength, .bdrv_detach_aio_context = nbd_detach_aio_context, .bdrv_attach_aio_context = nbd_attach_aio_context, @@ -430,6 +438,7 @@ static BlockDriver bdrv_nbd_unix = { .bdrv_close = nbd_close, .bdrv_co_flush_to_os = nbd_co_flush, .bdrv_co_discard = nbd_co_discard, + .bdrv_refresh_limits = nbd_refresh_limits, .bdrv_getlength = nbd_getlength, .bdrv_detach_aio_context = nbd_detach_aio_context, .bdrv_attach_aio_context = nbd_attach_aio_context,
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 <den@openvz.org> CC: Peter Lieven <pl@kamp.de> CC: Kevin Wolf <kwolf@redhat.com> --- block/nbd.c | 9 +++++++++ 1 file changed, 9 insertions(+)