[PATCH 1/1] ceph: Deletion of unnecessary checks before two function calls

[PATCH 1/1] ceph: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

The functions ceph_put_snap_context() and iput() test whether their argument
is NULL and then return 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>
---
 fs/ceph/caps.c       | 3 +--
 fs/ceph/mds_client.c | 6 ++----
 fs/ceph/snap.c       | 9 +++------
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 6d1cd45..7d99fc8 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3136,8 +3136,7 @@ flush_cap_releases:
 done:
 	mutex_unlock(&session->s_mutex);
 done_unlocked:
-	if (inode)
-		iput(inode);
+	iput(inode);
 	return;

 bad:
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index bad07c0..3b0ab05 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -523,8 +523,7 @@ void ceph_mdsc_release_request(struct kref *kref)
 	}
 	if (req->r_locked_dir)
 		ceph_put_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
-	if (req->r_target_inode)
-		iput(req->r_target_inode);
+	iput(req->r_target_inode);
 	if (req->r_dentry)
 		dput(req->r_dentry);
 	if (req->r_old_dentry)
@@ -995,8 +994,7 @@ out:
 	session->s_cap_iterator = NULL;
 	spin_unlock(&session->s_cap_lock);

-	if (last_inode)
-		iput(last_inode);
+	iput(last_inode);
 	if (old_cap)
 		ceph_put_cap(session->s_mdsc, old_cap);

diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
index f01645a..c1cc993 100644
--- a/fs/ceph/snap.c
+++ b/fs/ceph/snap.c
@@ -365,8 +365,7 @@ static int build_snap_context(struct ceph_snap_realm *realm)
 	     realm->ino, realm, snapc, snapc->seq,
 	     (unsigned int) snapc->num_snaps);

-	if (realm->cached_context)
-		ceph_put_snap_context(realm->cached_context);
+	ceph_put_snap_context(realm->cached_context);
 	realm->cached_context = snapc;
 	return 0;

@@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
*realm)
 		if (!inode)
 			continue;
 		spin_unlock(&realm->inodes_with_caps_lock);
-		if (lastinode)
-			iput(lastinode);
+		iput(lastinode);
 		lastinode = inode;
 		ceph_queue_cap_snap(ci);
 		spin_lock(&realm->inodes_with_caps_lock);
 	}
 	spin_unlock(&realm->inodes_with_caps_lock);
-	if (lastinode)
-		iput(lastinode);
+	iput(lastinode);

 	list_for_each_entry(child, &realm->children, child_item) {
 		dout("queue_realm_cap_snaps %p %llx queue child %p %llx\n",
-- 
2.1.3

Re: [PATCH 1/1] ceph: Deletion of unnecessary checks before two function calls

[Ilya Dryomov]

On Sun, Nov 2, 2014 at 5:20 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> The functions ceph_put_snap_context() and iput() test whether their argument
> is NULL and then return 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>
> ---
>  fs/ceph/caps.c       | 3 +--
>  fs/ceph/mds_client.c | 6 ++----
>  fs/ceph/snap.c       | 9 +++------
>  3 files changed, 6 insertions(+), 12 deletions(-)

[CC'ed Zheng]

Applied, but see below.

>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 6d1cd45..7d99fc8 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3136,8 +3136,7 @@ flush_cap_releases:
>  done:
>         mutex_unlock(&session->s_mutex);
>  done_unlocked:
> -       if (inode)
> -               iput(inode);
> +       iput(inode);
>         return;
>
>  bad:
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index bad07c0..3b0ab05 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -523,8 +523,7 @@ void ceph_mdsc_release_request(struct kref *kref)
>         }
>         if (req->r_locked_dir)
>                 ceph_put_cap_refs(ceph_inode(req->r_locked_dir), CEPH_CAP_PIN);
> -       if (req->r_target_inode)
> -               iput(req->r_target_inode);
> +       iput(req->r_target_inode);
>         if (req->r_dentry)
>                 dput(req->r_dentry);

dput() also checks for NULL argument, but the check is wrapped into
unlikely(), which is why I presume it wasn't picked up.  It would be
great if you could improve your coccinelle script to handle
{un,}likely() as well.

>         if (req->r_old_dentry)
> @@ -995,8 +994,7 @@ out:
>         session->s_cap_iterator = NULL;
>         spin_unlock(&session->s_cap_lock);
>
> -       if (last_inode)
> -               iput(last_inode);
> +       iput(last_inode);
>         if (old_cap)
>                 ceph_put_cap(session->s_mdsc, old_cap);
>
> diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> index f01645a..c1cc993 100644
> --- a/fs/ceph/snap.c
> +++ b/fs/ceph/snap.c
> @@ -365,8 +365,7 @@ static int build_snap_context(struct ceph_snap_realm *realm)
>              realm->ino, realm, snapc, snapc->seq,
>              (unsigned int) snapc->num_snaps);
>
> -       if (realm->cached_context)
> -               ceph_put_snap_context(realm->cached_context);
> +       ceph_put_snap_context(realm->cached_context);
>         realm->cached_context = snapc;
>         return 0;
>
> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
> *realm)

The patch was corrupted, that should have been a single line.  I fixed
it up but you may want to look into your email client settings.

Thanks,

                Ilya

Re: ceph: Deletion of unnecessary checks before two function calls

[SF Markus Elfring]

> dput() also checks for NULL argument, but the check is wrapped into
> unlikely(), which is why I presume it wasn't picked up.  It would be
> great if you could improve your coccinelle script to handle
> {un,}likely() as well.

Thanks for your suggestion.

Should I consider any more fine-tuning for the affected script
"list_input_parameter_validation1.cocci" in the near future?
https://lkml.org/lkml/2014/3/5/362
http://article.gmane.org/gmane.comp.version-control.coccinelle/3514


>> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
>> *realm)
> 
> The patch was corrupted, that should have been a single line.  I fixed
> it up but you may want to look into your email client settings.

Thanks for your feedback.

Does this example show a conflict between long comments after patch ranges and
line length limitation for email eventually?

Regards,
Markus

Re: ceph: Deletion of unnecessary checks before two function calls

[Ilya Dryomov]

On Mon, Nov 3, 2014 at 4:27 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> dput() also checks for NULL argument, but the check is wrapped into
>> unlikely(), which is why I presume it wasn't picked up.  It would be
>> great if you could improve your coccinelle script to handle
>> {un,}likely() as well.
>
> Thanks for your suggestion.
>
> Should I consider any more fine-tuning for the affected script
> "list_input_parameter_validation1.cocci" in the near future?
> https://lkml.org/lkml/2014/3/5/362
> http://article.gmane.org/gmane.comp.version-control.coccinelle/3514

Make sure it at least catches stuff like:

{
    if (input) {

    }
}

{
    if (likely(input)) {

    }
}

{
    if (!input)
        return;

    ...
}

{
    if (unlikely(!input))
        return;

    ...
}

And of course each match then has to be validated manually.

>
>
>>> @@ -590,15 +589,13 @@ static void queue_realm_cap_snaps(struct ceph_snap_realm
>>> *realm)
>>
>> The patch was corrupted, that should have been a single line.  I fixed
>> it up but you may want to look into your email client settings.
>
> Thanks for your feedback.
>
> Does this example show a conflict between long comments after patch ranges and
> line length limitation for email eventually?

There is no line length limitation for email, at least one that would
be relevant here.  Patches should be sent verbatim, no line wrapping,
expandtab, etc or they won't apply.  I'd recommend git-send-email, but
if you want to make thunderbird work for patches (which is what you
seem to be using) have a look at the "Thunderbird (GUI)" section of
Documentation/email-clients.txt in the kernel tree.

Thanks,

                Ilya

[PATCH 1/1] block-rbd: Delete a check before ceph_put_snap_context()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 1 Dec 2014 11:18:21 +0100

The ceph_put_snap_context() 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 27b71a0..3c34ab5 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -3405,8 +3405,7 @@ err_rq:
 	if (result)
 		rbd_warn(rbd_dev, "%s %llx at %llx result %d",
 			 obj_op_name(op_type), length, offset, result);
-	if (snapc)
-		ceph_put_snap_context(snapc);
+	ceph_put_snap_context(snapc);
 	blk_end_request_all(rq, result);
 }
 
-- 
2.1.3

Re: [PATCH 1/1] block-rbd: Delete a check before ceph_put_snap_context()

[Ilya Dryomov]

On Mon, Dec 1, 2014 at 1:23 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 1 Dec 2014 11:18:21 +0100
>
> The ceph_put_snap_context() 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 27b71a0..3c34ab5 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -3405,8 +3405,7 @@ err_rq:
>         if (result)
>                 rbd_warn(rbd_dev, "%s %llx at %llx result %d",
>                          obj_op_name(op_type), length, offset, result);
> -       if (snapc)
> -               ceph_put_snap_context(snapc);
> +       ceph_put_snap_context(snapc);
>         blk_end_request_all(rq, result);
>  }
>

I squashed it into your "ceph: Deletion of unnecessary checks before two
function calls", which deals with ceph_put_snap_context() in fs/ceph.

Thanks,

                Ilya

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

[SF Markus Elfring]

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

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

[SF Markus Elfring]

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

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

[Ilya Dryomov]

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

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

[SF Markus Elfring]

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

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

[Ilya Dryomov]

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

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

[SF Markus Elfring]

>> @@ -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

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

[Ilya Dryomov]

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

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

[SF Markus Elfring]

> 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

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

[Ilya Dryomov]

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

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

[SF Markus Elfring]

> "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

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

[Dan Carpenter]

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

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

[Ilya Dryomov]

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

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

[Dan Carpenter]

Ah... I see now.  Thanks.

regards,
dan carpenter

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

[SF Markus Elfring]

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

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

[Ilya Dryomov]

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

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

[SF Markus Elfring]

>> * 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