linux-kernel - Re: [PATCH 2/2] block-rbd: One function call less in rbd_dev_probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5654B91D.4040808@users.sourceforge.net>
Date:	Tue, 24 Nov 2015 20:23:09 +0100
From:	SF Markus Elfring <elfring@...rs.sourceforge.net>
To:	Ilya Dryomov <idryomov@...il.com>
Cc:	Alex Elder <elder@...nel.org>, Sage Weil <sage@...hat.com>,
	Ceph Development <ceph-devel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	kernel-janitors@...r.kernel.org,
	Julia Lawall <julia.lawall@...6.fr>
Subject: 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/