| Message ID | 1565334327-7454-1-git-send-email-dongsheng.yang@easystack.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rbd: fix response length parameter for rbd_obj_method_sync() | expand |
On Fri, Aug 9, 2019 at 9:05 AM Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > > Response will be an encoded string, which includes a length. > So we need to allocate more buf of sizeof (__le32) in reply > buffer, and pass the reply buffer size as a parameter in > rbd_obj_method_sync function. > > Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> > --- > drivers/block/rbd.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index 3327192..db55ece 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -5661,14 +5661,17 @@ static int rbd_dev_v2_object_prefix(struct rbd_device *rbd_dev) > void *reply_buf; > int ret; > void *p; > + size_t size; > > - reply_buf = kzalloc(RBD_OBJ_PREFIX_LEN_MAX, GFP_KERNEL); > + /* Response will be an encoded string, which includes a length */ > + size = sizeof (__le32) + RBD_OBJ_PREFIX_LEN_MAX; > + reply_buf = kzalloc(size, GFP_KERNEL); > if (!reply_buf) > return -ENOMEM; > > ret = rbd_obj_method_sync(rbd_dev, &rbd_dev->header_oid, > &rbd_dev->header_oloc, "get_object_prefix", > - NULL, 0, reply_buf, RBD_OBJ_PREFIX_LEN_MAX); > + NULL, 0, reply_buf, size); > dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); > if (ret < 0) > goto out; > @@ -6697,7 +6700,7 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) > > ret = rbd_obj_method_sync(rbd_dev, &oid, &rbd_dev->header_oloc, > "get_id", NULL, 0, > - response, RBD_IMAGE_ID_LEN_MAX); > + response, size); > dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); > if (ret == -ENOENT) { > image_id = kstrdup("", GFP_KERNEL); Hi Dongsheng, AFAIR RBD_OBJ_PREFIX_LEN_MAX and RBD_IMAGE_ID_LEN_MAX are arbitrary constants with enough slack for length, etc. We shouldn't ever see object prefixes or image ids that are longer than 62 bytes. Thanks, Ilya
On 08/09/2019 03:34 PM, Ilya Dryomov wrote: > On Fri, Aug 9, 2019 at 9:05 AM Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> Response will be an encoded string, which includes a length. >> So we need to allocate more buf of sizeof (__le32) in reply >> buffer, and pass the reply buffer size as a parameter in >> rbd_obj_method_sync function. >> >> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> >> --- >> drivers/block/rbd.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index 3327192..db55ece 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -5661,14 +5661,17 @@ static int rbd_dev_v2_object_prefix(struct rbd_device *rbd_dev) >> void *reply_buf; >> int ret; >> void *p; >> + size_t size; >> >> - reply_buf = kzalloc(RBD_OBJ_PREFIX_LEN_MAX, GFP_KERNEL); >> + /* Response will be an encoded string, which includes a length */ >> + size = sizeof (__le32) + RBD_OBJ_PREFIX_LEN_MAX; >> + reply_buf = kzalloc(size, GFP_KERNEL); >> if (!reply_buf) >> return -ENOMEM; >> >> ret = rbd_obj_method_sync(rbd_dev, &rbd_dev->header_oid, >> &rbd_dev->header_oloc, "get_object_prefix", >> - NULL, 0, reply_buf, RBD_OBJ_PREFIX_LEN_MAX); >> + NULL, 0, reply_buf, size); >> dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); >> if (ret < 0) >> goto out; >> @@ -6697,7 +6700,7 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) >> >> ret = rbd_obj_method_sync(rbd_dev, &oid, &rbd_dev->header_oloc, >> "get_id", NULL, 0, >> - response, RBD_IMAGE_ID_LEN_MAX); >> + response, size); >> dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); >> if (ret == -ENOENT) { >> image_id = kstrdup("", GFP_KERNEL); > Hi Dongsheng, > > AFAIR RBD_OBJ_PREFIX_LEN_MAX and RBD_IMAGE_ID_LEN_MAX are arbitrary > constants with enough slack for length, etc. We shouldn't ever see > object prefixes or image ids that are longer than 62 bytes. Hi Ilya, Yes, this patch is not fixing a real problem, as you mentioned we dont have prefixes or image ids longer than 62 bytes. But this patch is going to make it logical consistent. The most confusing case is for rbd_dev_image_id(), size of response is already RBD_IMAGE_ID_LEN_MAX + sizeof (__le32). but the @resp_length in rbd_obj_method_sync() is still RBD_IMAGE_ID_LEN_MAX. Thanx > > Thanks, > > Ilya >
On Fri, Aug 9, 2019 at 9:45 AM Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > > > > On 08/09/2019 03:34 PM, Ilya Dryomov wrote: > > On Fri, Aug 9, 2019 at 9:05 AM Dongsheng Yang > > <dongsheng.yang@easystack.cn> wrote: > >> Response will be an encoded string, which includes a length. > >> So we need to allocate more buf of sizeof (__le32) in reply > >> buffer, and pass the reply buffer size as a parameter in > >> rbd_obj_method_sync function. > >> > >> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> > >> --- > >> drivers/block/rbd.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > >> index 3327192..db55ece 100644 > >> --- a/drivers/block/rbd.c > >> +++ b/drivers/block/rbd.c > >> @@ -5661,14 +5661,17 @@ static int rbd_dev_v2_object_prefix(struct rbd_device *rbd_dev) > >> void *reply_buf; > >> int ret; > >> void *p; > >> + size_t size; > >> > >> - reply_buf = kzalloc(RBD_OBJ_PREFIX_LEN_MAX, GFP_KERNEL); > >> + /* Response will be an encoded string, which includes a length */ > >> + size = sizeof (__le32) + RBD_OBJ_PREFIX_LEN_MAX; > >> + reply_buf = kzalloc(size, GFP_KERNEL); > >> if (!reply_buf) > >> return -ENOMEM; > >> > >> ret = rbd_obj_method_sync(rbd_dev, &rbd_dev->header_oid, > >> &rbd_dev->header_oloc, "get_object_prefix", > >> - NULL, 0, reply_buf, RBD_OBJ_PREFIX_LEN_MAX); > >> + NULL, 0, reply_buf, size); > >> dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); > >> if (ret < 0) > >> goto out; > >> @@ -6697,7 +6700,7 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) > >> > >> ret = rbd_obj_method_sync(rbd_dev, &oid, &rbd_dev->header_oloc, > >> "get_id", NULL, 0, > >> - response, RBD_IMAGE_ID_LEN_MAX); > >> + response, size); > >> dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); > >> if (ret == -ENOENT) { > >> image_id = kstrdup("", GFP_KERNEL); > > Hi Dongsheng, > > > > AFAIR RBD_OBJ_PREFIX_LEN_MAX and RBD_IMAGE_ID_LEN_MAX are arbitrary > > constants with enough slack for length, etc. We shouldn't ever see > > object prefixes or image ids that are longer than 62 bytes. > > Hi Ilya, > Yes, this patch is not fixing a real problem, as you mentioned we > dont have > prefixes or image ids longer than 62 bytes. But this patch is going to > make it > logical consistent. > > The most confusing case is for rbd_dev_image_id(), size of response is > already > RBD_IMAGE_ID_LEN_MAX + sizeof (__le32). but the @resp_length in > rbd_obj_method_sync() > is still RBD_IMAGE_ID_LEN_MAX. I see, consistency is a good thing. I amended the changelog and applied with a minor whitespace fixup: https://github.com/ceph/ceph-client/commit/1a25b186a7526185e74ee799a956f8bd32aa82fb Thanks, Ilya
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 3327192..db55ece 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -5661,14 +5661,17 @@ static int rbd_dev_v2_object_prefix(struct rbd_device *rbd_dev) void *reply_buf; int ret; void *p; + size_t size; - reply_buf = kzalloc(RBD_OBJ_PREFIX_LEN_MAX, GFP_KERNEL); + /* Response will be an encoded string, which includes a length */ + size = sizeof (__le32) + RBD_OBJ_PREFIX_LEN_MAX; + reply_buf = kzalloc(size, GFP_KERNEL); if (!reply_buf) return -ENOMEM; ret = rbd_obj_method_sync(rbd_dev, &rbd_dev->header_oid, &rbd_dev->header_oloc, "get_object_prefix", - NULL, 0, reply_buf, RBD_OBJ_PREFIX_LEN_MAX); + NULL, 0, reply_buf, size); dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); if (ret < 0) goto out; @@ -6697,7 +6700,7 @@ static int rbd_dev_image_id(struct rbd_device *rbd_dev) ret = rbd_obj_method_sync(rbd_dev, &oid, &rbd_dev->header_oloc, "get_id", NULL, 0, - response, RBD_IMAGE_ID_LEN_MAX); + response, size); dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret); if (ret == -ENOENT) { image_id = kstrdup("", GFP_KERNEL);
Response will be an encoded string, which includes a length. So we need to allocate more buf of sizeof (__le32) in reply buffer, and pass the reply buffer size as a parameter in rbd_obj_method_sync function. Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> --- drivers/block/rbd.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)