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@xxxxxxxxxxxxxxxxxxxxx> wrote:
> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/