lkml.org 
[lkml]   [2015]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe_parent() after error detection
From
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


\
 
 \ /
  Last update: 2015-11-24 14:41    [W:0.916 / U:5.228 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site