Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1275778 > unrolled thread

[PATCH 0/2] block-rbd: Deletion of an unnecessary check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/2] block-rbd: Deletion of an unnecessary check SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-23 20:50 +0100
    [PATCH 1/2] block-rbd: Delete an unnecessary check before the  function call "rbd_dev_destroy" SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-23 20:50 +0100
      Re: [PATCH 1/2] block-rbd: Delete an unnecessary check before the  function call "rbd_dev_destroy" Ilya Dryomov <idryomov@gmail.com> - 2015-11-24 14:30 +0100
    [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-23 20:50 +0100
      Re: [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection Ilya Dryomov <idryomov@gmail.com> - 2015-11-24 14:30 +0100
        Re: [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-24 20:30 +0100
          Re: [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection Ilya Dryomov <idryomov@gmail.com> - 2015-11-24 21:30 +0100
            Re: block-rbd: One function call less in rbd_dev_probe_parent() after  error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-24 21:40 +0100
              Re: block-rbd: One function call less in rbd_dev_probe_parent() after  error detection Ilya Dryomov <idryomov@gmail.com> - 2015-11-24 23:00 +0100
                Re: block-rbd: One function call less in rbd_dev_probe_parent() after  error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-25 11:30 +0100
            Re: [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection Dan Carpenter <dan.carpenter@oracle.com> - 2015-11-25 13:00 +0100
              Re: [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection Ilya Dryomov <idryomov@gmail.com> - 2015-11-25 15:10 +0100
                Re: [PATCH 2/2] block-rbd: One function call less in  rbd_dev_probe_parent() after error detection Dan Carpenter <dan.carpenter@oracle.com> - 2015-11-25 16:30 +0100
            Re: block-rbd: One function call less in rbd_dev_probe_parent() after  error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-26 09:00 +0100
              Re: block-rbd: One function call less in rbd_dev_probe_parent() after  error detection Ilya Dryomov <idryomov@gmail.com> - 2015-11-26 12:50 +0100
                Re: block-rbd: One function call less in rbd_dev_probe_parent() after  error detection SF Markus Elfring <elfring@users.sourceforge.net> - 2015-11-26 14:50 +0100

#1275778 — [PATCH 0/2] block-rbd: Deletion of an unnecessary check

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 23 Nov 2015 20:33:59 +0100

Another update suggestion was taken into account after a patch was applied
from static source code analysis.

Markus Elfring (2):
  Delete an unnecessary check before the function call "rbd_dev_destroy"
  One function call less in rbd_dev_probe_parent() after error detection

 drivers/block/rbd.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1275779 — [PATCH 1/2] block-rbd: Delete an unnecessary check before the function call "rbd_dev_destroy"

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 23 Nov 2015 20:16:45 +0100

The rbd_dev_destroy() function tests whether its argument is NULL
and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/rbd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 235708c..24a757e 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -5184,8 +5184,7 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
 
 out_err:
 	rbd_dev_unparent(rbd_dev);
-	if (parent)
-		rbd_dev_destroy(parent);
+	rbd_dev_destroy(parent);
 	return ret;
 }
 
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276440 — Re: [PATCH 1/2] block-rbd: Delete an unnecessary check before the function call "rbd_dev_destroy"

On Mon, Nov 23, 2015 at 8:44 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 23 Nov 2015 20:16:45 +0100
>
> The rbd_dev_destroy() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/rbd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 235708c..24a757e 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -5184,8 +5184,7 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>
>  out_err:
>         rbd_dev_unparent(rbd_dev);
> -       if (parent)
> -               rbd_dev_destroy(parent);
> +       rbd_dev_destroy(parent);
>         return ret;
>  }

Applied, see https://github.com/ceph/ceph-client/commit/b0098c3eadd33fa212987dab0529e35c948aaff4.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1275781 — [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 23 Nov 2015 20:22:41 +0100

The rbd_dev_destroy() function was called in two cases by the
rbd_dev_probe_parent() function during error handling even if
the passed variable contained a null pointer.

* This implementation detail could be improved by adjustments
  for jump targets according to the Linux coding style convention.

* Drop an unnecessary initialisation for the variable "parent" then.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/rbd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 24a757e..2ad9092 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -5148,7 +5148,7 @@ out_err:
  */
 static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
 {
-	struct rbd_device *parent = NULL;
+	struct rbd_device *parent;
 	int ret;
 
 	if (!rbd_dev->parent_spec)
@@ -5157,14 +5157,14 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
 	if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
 		pr_info("parent chain is too long (%d)\n", depth);
 		ret = -EINVAL;
-		goto out_err;
+		goto unparent_device;
 	}
 
 	parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
 				NULL);
 	if (!parent) {
 		ret = -ENOMEM;
-		goto out_err;
+		goto unparent_device;
 	}
 
 	/*
@@ -5176,15 +5176,15 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
 
 	ret = rbd_dev_image_probe(parent, depth);
 	if (ret < 0)
-		goto out_err;
+		goto destroy_device;
 
 	rbd_dev->parent = parent;
 	atomic_set(&rbd_dev->parent_ref, 1);
 	return 0;
-
-out_err:
-	rbd_dev_unparent(rbd_dev);
+destroy_device:
 	rbd_dev_destroy(parent);
+unparent_device:
+	rbd_dev_unparent(rbd_dev);
 	return ret;
 }
 
-- 
2.6.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276443 — Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

On Mon, Nov 23, 2015 at 8:46 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 23 Nov 2015 20:22:41 +0100
>
> The rbd_dev_destroy() function was called in two cases by the
> rbd_dev_probe_parent() function during error handling even if
> the passed variable contained a null pointer.
>
> * This implementation detail could be improved by adjustments
>   for jump targets according to the Linux coding style convention.
>
> * Drop an unnecessary initialisation for the variable "parent" then.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/rbd.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 24a757e..2ad9092 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -5148,7 +5148,7 @@ out_err:
>   */
>  static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>  {
> -       struct rbd_device *parent = NULL;
> +       struct rbd_device *parent;
>         int ret;
>
>         if (!rbd_dev->parent_spec)
> @@ -5157,14 +5157,14 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>         if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
>                 pr_info("parent chain is too long (%d)\n", depth);
>                 ret = -EINVAL;
> -               goto out_err;
> +               goto unparent_device;
>         }
>
>         parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
>                                 NULL);
>         if (!parent) {
>                 ret = -ENOMEM;
> -               goto out_err;
> +               goto unparent_device;
>         }
>
>         /*
> @@ -5176,15 +5176,15 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>
>         ret = rbd_dev_image_probe(parent, depth);
>         if (ret < 0)
> -               goto out_err;
> +               goto destroy_device;
>
>         rbd_dev->parent = parent;
>         atomic_set(&rbd_dev->parent_ref, 1);
>         return 0;
> -
> -out_err:
> -       rbd_dev_unparent(rbd_dev);
> +destroy_device:
>         rbd_dev_destroy(parent);
> +unparent_device:
> +       rbd_dev_unparent(rbd_dev);
>         return ret;
>  }

Cleanup here is (and should be) done in reverse order.  We allocate
parent rbd_device and then link it with what we already have, so the
order in which we cleanup is unlink ("unparent"), destroy.

Changing it is just asking for use-after-free bugs.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276724 — Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

>> @@ -5157,14 +5157,14 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>>         if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
>>                 pr_info("parent chain is too long (%d)\n", depth);
>>                 ret = -EINVAL;
>> -               goto out_err;
>> +               goto unparent_device;
>>         }
>>
>>         parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
>>                                 NULL);
>>         if (!parent) {
>>                 ret = -ENOMEM;
>> -               goto out_err;
>> +               goto unparent_device;
>>         }
>>
>>         /*
>> @@ -5176,15 +5176,15 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>>
>>         ret = rbd_dev_image_probe(parent, depth);
>>         if (ret < 0)
>> -               goto out_err;
>> +               goto destroy_device;
>>
>>         rbd_dev->parent = parent;
>>         atomic_set(&rbd_dev->parent_ref, 1);
>>         return 0;
>> -
>> -out_err:
>> -       rbd_dev_unparent(rbd_dev);
>> +destroy_device:
>>         rbd_dev_destroy(parent);
>> +unparent_device:
>> +       rbd_dev_unparent(rbd_dev);
>>         return ret;
>>  }
> 
> Cleanup here is (and should be) done in reverse order.

I have got an other impression about the appropriate order for the corresponding
clean-up function calls.


> We allocate parent rbd_device and then link it with what we already have,

I guess that we have got a different understanding about the relevant "linking".


> so the order in which we cleanup is unlink ("unparent"), destroy.

I interpreted the eventual passing of a null pointer to the rbd_dev_destroy()
function as an indication for further source code adjustments.


> Changing it is just asking for use-after-free bugs.

Do the affected implementation details need a bit more clarification?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276746 — Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

On Tue, Nov 24, 2015 at 8:23 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> @@ -5157,14 +5157,14 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>>>         if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
>>>                 pr_info("parent chain is too long (%d)\n", depth);
>>>                 ret = -EINVAL;
>>> -               goto out_err;
>>> +               goto unparent_device;
>>>         }
>>>
>>>         parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
>>>                                 NULL);
>>>         if (!parent) {
>>>                 ret = -ENOMEM;
>>> -               goto out_err;
>>> +               goto unparent_device;
>>>         }
>>>
>>>         /*
>>> @@ -5176,15 +5176,15 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>>>
>>>         ret = rbd_dev_image_probe(parent, depth);
>>>         if (ret < 0)
>>> -               goto out_err;
>>> +               goto destroy_device;
>>>
>>>         rbd_dev->parent = parent;
>>>         atomic_set(&rbd_dev->parent_ref, 1);
>>>         return 0;
>>> -
>>> -out_err:
>>> -       rbd_dev_unparent(rbd_dev);
>>> +destroy_device:
>>>         rbd_dev_destroy(parent);
>>> +unparent_device:
>>> +       rbd_dev_unparent(rbd_dev);
>>>         return ret;
>>>  }
>>
>> Cleanup here is (and should be) done in reverse order.
>
> I have got an other impression about the appropriate order for the corresponding
> clean-up function calls.
>
>
>> We allocate parent rbd_device and then link it with what we already have,
>
> I guess that we have got a different understanding about the relevant "linking".

Well, there isn't any _literal_ linking (e.g. adding to a link list,
etc) in this case.  We just bump some refs and do probe to fill in the
newly allocated parent.  If probe fails, we put refs and free parent,
reversing the "alloc parent, bump refs" order.

The actual linking (rbd_dev->parent = parent) is done right before
returning so we never have to undo it in rbd_dev_probe_parent() and
that's the only reason your patch probably doesn't break anything.
Think about what happens if, after your patch is applied, someone moves
that assignment up or adds an extra step that can fail after it...

>
>
>> so the order in which we cleanup is unlink ("unparent"), destroy.
>
> I interpreted the eventual passing of a null pointer to the rbd_dev_destroy()
> function as an indication for further source code adjustments.

If all error paths could be adjusted so that NULL pointers are never
passed in, destroy functions wouldn't need to have a NULL check, would
they?

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276754 — Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection

> Well, there isn't any _literal_ linking (e.g. adding to a link list,
> etc) in this case.  We just bump some refs and do probe to fill in the
> newly allocated parent.

Thanks for your clarification.


> The actual linking (rbd_dev->parent = parent) is done right before
> returning so we never have to undo it in rbd_dev_probe_parent() and
> that's the only reason your patch probably doesn't break anything.

Is this function implementation just also affected by an issue
which is mentioned in the Linux document "CodingStyle" as "one err bugs"?


> Think about what happens if, after your patch is applied, someone moves
> that assignment up or adds an extra step that can fail after it...

Is such a software maintenance concern really enough to delay (or reject)
my second update suggestion in this small patch series?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1276797 — Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection

On Tue, Nov 24, 2015 at 9:34 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Well, there isn't any _literal_ linking (e.g. adding to a link list,
>> etc) in this case.  We just bump some refs and do probe to fill in the
>> newly allocated parent.
>
> Thanks for your clarification.
>
>
>> The actual linking (rbd_dev->parent = parent) is done right before
>> returning so we never have to undo it in rbd_dev_probe_parent() and
>> that's the only reason your patch probably doesn't break anything.
>
> Is this function implementation just also affected by an issue
> which is mentioned in the Linux document "CodingStyle" as "one err bugs"?

No, why?  "one err bug" as per CodingStyle is a NULL deref on line 2 if
foo is NULL.  If it was just "err: kfree(foo); return ret;", a NULL foo
would be perfectly OK.

1       err:
2               kfree(foo->bar);
3               kfree(foo);
4               return ret;

If you can spot such a NULL deref in rbd_dev_probe_parent(), I'd gladly
take a patch.

>
>
>> Think about what happens if, after your patch is applied, someone moves
>> that assignment up or adds an extra step that can fail after it...
>
> Is such a software maintenance concern really enough to delay (or reject)
> my second update suggestion in this small patch series?

Yes - it's rejected because it messes up the order of cleanup for no
good reason.  I realize why you think the patch is correct and it's not
without merit, but it just doesn't fit the weird rbd_dev_probe_parent()
contract.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277167 — Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection

> "one err bug" as per CodingStyle is a NULL deref on line 2 if foo is NULL.
> If it was just "err: kfree(foo); return ret;", a NULL foo would be perfectly OK.

Would it make sense to rename such an issue as the "one error jump label bug"?
https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277279 — Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

On Tue, Nov 24, 2015 at 09:21:06PM +0100, Ilya Dryomov wrote:
> >> Cleanup here is (and should be) done in reverse order.
> >

Yes.  This is true.

> > I have got an other impression about the appropriate order for the corresponding
> > clean-up function calls.
> >
> >
> >> We allocate parent rbd_device and then link it with what we already have,
> >
> > I guess that we have got a different understanding about the relevant "linking".
> 
> Well, there isn't any _literal_ linking (e.g. adding to a link list,
> etc) in this case.  We just bump some refs and do probe to fill in the
> newly allocated parent.  If probe fails, we put refs and free parent,
> reversing the "alloc parent, bump refs" order.
> 
> The actual linking (rbd_dev->parent = parent) is done right before
> returning so we never have to undo it in rbd_dev_probe_parent() and
> that's the only reason your patch probably doesn't break anything.
> Think about what happens if, after your patch is applied, someone moves
> that assignment up or adds an extra step that can fail after it...
> 

The problem is that the unwind code should be a mirror of the allocate
code but rbd_dev_unparent() doesn't mirror anything.  Generally, writing
future proof stubs like this is a wrong thing because predicting the
future is hard and in the mean time we are left stubs which confuse
everyone.

> If all error paths could be adjusted so that NULL pointers are never
> passed in, destroy functions wouldn't need to have a NULL check, would
> they?

Yep.  We agree on the right way to do it.  I am probably the number one
kernel developer for removing the most sanity checks.  :P  (As opposed
to patch 1/1 where we now rely on the sanity check inside
rbd_dev_destroy().)

drivers/block/rbd.c
  5149  static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
  5150  {
  5151          struct rbd_device *parent = NULL;
  5152          int ret;
  5153  
  5154          if (!rbd_dev->parent_spec)
  5155                  return 0;
  5156  
  5157          if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
  5158                  pr_info("parent chain is too long (%d)\n", depth);
  5159                  ret = -EINVAL;
  5160                  goto out_err;

We haven't allocated anything so this should just be return -EINVAL;
In the original code, we decrement the kref count on ->parent_spec on
this error path so that is a classic One Err Bug.

  5161          }
  5162  
  5163          parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
  5164                                  NULL);
  5165          if (!parent) {
  5166                  ret = -ENOMEM;
  5167                  goto out_err;

Still haven't allocated anything so return -ENOMEM, but if we fail after
this point we will need to call rbd_dev_destroy().

  5168          }
  5169  
  5170          /*
  5171           * Images related by parent/child relationships always share
  5172           * rbd_client and spec/parent_spec, so bump their refcounts.
  5173           */
  5174          __rbd_get_client(rbd_dev->rbd_client);
  5175          rbd_spec_get(rbd_dev->parent_spec);

We will need to put these on any later error paths.

  5176  
  5177          ret = rbd_dev_image_probe(parent, depth);
  5178          if (ret < 0)
  5179                  goto out_err;

Ok.  We need to put the ->parent_spec, ->rbd_client and free the parent.

  5180  
  5181          rbd_dev->parent = parent;
  5182          atomic_set(&rbd_dev->parent_ref, 1);
  5183          return 0;
  5184  
  5185  out_err:
  5186          rbd_dev_unparent(rbd_dev);

This is a complicated way to say rbd_spec_put(rbd_dev->parent_spec);

Also, is it really necessary to set ->parent_spec to NULL?  If we didn't
put the last reference then doesn't setting it to NULL mean we are
leaking?  Setting it to NULL is confusing and feels like a layering
violation.

  5187          if (parent)
  5188                  rbd_dev_destroy(parent);
  5189          return ret;
  5190  }

I feel like we should be calling rbd_put_client() on this error path or
else the code is buggy or has layer violations.  So I *think* it should
look like this:

dec_ref_counts:
	rbd_spec_put(rbd_dev->parent_spec);
	rbd_put_client(rbd_dev->rbd_client);

	rbd_dev_destroy(parent);

	return ret;

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277410 — Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

On Wed, Nov 25, 2015 at 12:55 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Tue, Nov 24, 2015 at 09:21:06PM +0100, Ilya Dryomov wrote:
>> >> Cleanup here is (and should be) done in reverse order.
>> >
>
> Yes.  This is true.
>
>> > I have got an other impression about the appropriate order for the corresponding
>> > clean-up function calls.
>> >
>> >
>> >> We allocate parent rbd_device and then link it with what we already have,
>> >
>> > I guess that we have got a different understanding about the relevant "linking".
>>
>> Well, there isn't any _literal_ linking (e.g. adding to a link list,
>> etc) in this case.  We just bump some refs and do probe to fill in the
>> newly allocated parent.  If probe fails, we put refs and free parent,
>> reversing the "alloc parent, bump refs" order.
>>
>> The actual linking (rbd_dev->parent = parent) is done right before
>> returning so we never have to undo it in rbd_dev_probe_parent() and
>> that's the only reason your patch probably doesn't break anything.
>> Think about what happens if, after your patch is applied, someone moves
>> that assignment up or adds an extra step that can fail after it...
>>
>
> The problem is that the unwind code should be a mirror of the allocate
> code but rbd_dev_unparent() doesn't mirror anything.  Generally, writing
> future proof stubs like this is a wrong thing because predicting the
> future is hard and in the mean time we are left stubs which confuse
> everyone.

It's not a future proof stub.  It's just some crufty code that was
fixed over time to not leak things.  I won't defend it - it is
confusing and could definitely be improved - but that can't be done
without refactoring a fair bunch of calling code.  A patch changing
rbd_dev_probe_parent() alone just won't do it.

>
>> If all error paths could be adjusted so that NULL pointers are never
>> passed in, destroy functions wouldn't need to have a NULL check, would
>> they?
>
> Yep.  We agree on the right way to do it.  I am probably the number one
> kernel developer for removing the most sanity checks.  :P  (As opposed
> to patch 1/1 where we now rely on the sanity check inside
> rbd_dev_destroy().)
>
> drivers/block/rbd.c
>   5149  static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>   5150  {
>   5151          struct rbd_device *parent = NULL;
>   5152          int ret;
>   5153
>   5154          if (!rbd_dev->parent_spec)
>   5155                  return 0;
>   5156
>   5157          if (++depth > RBD_MAX_PARENT_CHAIN_LEN) {
>   5158                  pr_info("parent chain is too long (%d)\n", depth);
>   5159                  ret = -EINVAL;
>   5160                  goto out_err;
>
> We haven't allocated anything so this should just be return -EINVAL;
> In the original code, we decrement the kref count on ->parent_spec on
> this error path so that is a classic One Err Bug.

The caller expects rbd_dev->parent_spec to be put on any error.  Notice
that we return right away if !rbd_dev->parent_spec.

>
>   5161          }
>   5162
>   5163          parent = rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec,
>   5164                                  NULL);
>   5165          if (!parent) {
>   5166                  ret = -ENOMEM;
>   5167                  goto out_err;
>
> Still haven't allocated anything so return -ENOMEM, but if we fail after
> this point we will need to call rbd_dev_destroy().
>
>   5168          }
>   5169
>   5170          /*
>   5171           * Images related by parent/child relationships always share
>   5172           * rbd_client and spec/parent_spec, so bump their refcounts.
>   5173           */
>   5174          __rbd_get_client(rbd_dev->rbd_client);
>   5175          rbd_spec_get(rbd_dev->parent_spec);
>
> We will need to put these on any later error paths.

And we do, in rbd_dev_destroy(parent), since these are references for
the parent.

>
>   5176
>   5177          ret = rbd_dev_image_probe(parent, depth);
>   5178          if (ret < 0)
>   5179                  goto out_err;
>
> Ok.  We need to put the ->parent_spec, ->rbd_client and free the parent.
>
>   5180
>   5181          rbd_dev->parent = parent;
>   5182          atomic_set(&rbd_dev->parent_ref, 1);
>   5183          return 0;
>   5184
>   5185  out_err:
>   5186          rbd_dev_unparent(rbd_dev);
>
> This is a complicated way to say rbd_spec_put(rbd_dev->parent_spec);
>
> Also, is it really necessary to set ->parent_spec to NULL?  If we didn't
> put the last reference then doesn't setting it to NULL mean we are
> leaking?  Setting it to NULL is confusing and feels like a layering
> violation.

Yes, because as it is, ->parent_spec is a determinant of whether or not
the image has a parent.  If we fail in rbd_dev_probe_parent(), it needs
to be set to NULL to signify that the image doesn't have a parent.
Even if the entire thing was refactored, we'd still have to do the same
because not every image has a parent and the same error path has to
work for all images.  The layering violation is that we have to do in
rbd_dev_probe_parent() even though we didn't allocate it there.

>
>   5187          if (parent)
>   5188                  rbd_dev_destroy(parent);
>   5189          return ret;
>   5190  }
>
> I feel like we should be calling rbd_put_client() on this error path or
> else the code is buggy or has layer violations.  So I *think* it should
> look like this:
>
> dec_ref_counts:
>         rbd_spec_put(rbd_dev->parent_spec);
>         rbd_put_client(rbd_dev->rbd_client);
>
>         rbd_dev_destroy(parent);
>
>         return ret;

We do, in rbd_dev_destroy(parent).

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277475 — Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection

Ah... I see now.  Thanks.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278039 — Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection

>> I interpreted the eventual passing of a null pointer to the rbd_dev_destroy()
>> function as an indication for further source code adjustments.
> 
> If all error paths could be adjusted so that NULL pointers are never passed in,
> destroy functions wouldn't need to have a NULL check, would they?

How do you think about to clarify corresponding implementation details a bit more?

* Why was the function "rbd_dev_probe_parent" implemented in the way
  that it relies on a sanity check in the function "rbd_dev_destroy" then?
* How are the chances to restructure the source code a bit (like changing a few
  jump labels) so that it should also work without an extra function call
  during error handling there?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278160 — Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection

On Thu, Nov 26, 2015 at 8:54 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> I interpreted the eventual passing of a null pointer to the rbd_dev_destroy()
>>> function as an indication for further source code adjustments.
>>
>> If all error paths could be adjusted so that NULL pointers are never passed in,
>> destroy functions wouldn't need to have a NULL check, would they?
>
> How do you think about to clarify corresponding implementation details a bit more?
>
> * Why was the function "rbd_dev_probe_parent" implemented in the way
>   that it relies on a sanity check in the function "rbd_dev_destroy" then?

Because it's not a bad thing?  What's wrong with an init to NULL,
a possible assignment, in this case from rbd_dev_create(), and an
unconditional rbd_dev_destroy()?

The NULL check in rbd_dev_destroy() is not a sanity check, it's
a feature.  It's not there to "fixup" callers that pass NULL - it's
there because it is _expected_ that some callers will pass NULL.

> * How are the chances to restructure the source code a bit (like changing a few
>   jump labels) so that it should also work without an extra function call
>   during error handling there?

As I said in my reply to Dan, the problem with rbd_dev_probe_parent()
is the calling code which expects it to call unparent if ->parent_spec.
This makes it stand out and confuses people, but can't be fixed without
refactoring a bunch of other code.

The extra function call is *not* a problem.

Thanks,

                Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278223 — Re: block-rbd: One function call less in rbd_dev_probe_parent() after error detection

>> * Why was the function "rbd_dev_probe_parent" implemented in the way
>>   that it relies on a sanity check in the function "rbd_dev_destroy" then?
> 
> Because it's not a bad thing?

There are different opinions about this implementation detail.


> What's wrong with an init to NULL, a possible assignment, in this case
> from rbd_dev_create(), and an unconditional rbd_dev_destroy()?

Does this approach look like it is affected by a "one error jump
label bug" symptom?


> The NULL check in rbd_dev_destroy() is not a sanity check,
> it's a feature.

I have got an other impression here.


> It's not there to "fixup" callers that pass NULL

It seems that the explanations on the detail why a function tolerates
passed null pointers can also be different.


> - it's there because it is _expected_ that some callers will pass NULL.

I find it still unnecessary to let a called function like "rbd_dev_destroy"
to repeat the check when you know already that the passed variable contains
a null pointer.


> As I said in my reply to Dan, the problem with rbd_dev_probe_parent()
> is the calling code which expects it to call unparent if ->parent_spec.
> This makes it stand out and confuses people, but can't be fixed without
> refactoring a bunch of other code.

I would appreciate if the discussed function could be also improved by itself.
More refactoring might follow at other source code places later.


> The extra function call is *not* a problem.

How many software developers and reviewers will care if corresponding
error handling can also become a bit more efficient?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web