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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
