rbd: fix response length parameter for rbd_obj_method_sync()
diff mbox series

Message ID 1565334327-7454-1-git-send-email-dongsheng.yang@easystack.cn
State New
Headers show
Series
  • rbd: fix response length parameter for rbd_obj_method_sync()
Related show

Commit Message

Dongsheng Yang Aug. 9, 2019, 7:05 a.m. UTC
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(-)

Comments

Ilya Dryomov Aug. 9, 2019, 7:34 a.m. UTC | #1
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
Dongsheng Yang Aug. 9, 2019, 7:44 a.m. UTC | #2
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
>
Ilya Dryomov Aug. 9, 2019, 10:59 a.m. UTC | #3
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

Patch
diff mbox series

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);