On 05/03/2016 12:23 AM, Benjamin Marzinski wrote:
> On Wed, Apr 27, 2016 at 01:10:37PM +0200, Hannes Reinecke wrote:
>> Instead of generating the cookie internally we should be
>> passing in the cookie to dm_addmap().
>
> These look like they could actually cause problems. With
> dm_addmap_create and dm_addmap_reload, you could be in a situation where
> you get a cookie back on your first call to dm_task_set_cookie, wait on
> it, so that cookie is no longer in use, and then use that same cookie
> id again. It's possible that after you first waited on the cookie,
> someone else could have been given that cookie id (without looking into
> the dm cookie code more, I'm not sure how likely this is). If that is
> so, the second call to dm_addmap would be adding its command to the list
> of other commands for that cookie (since you are allowed to use a cookie
> for multiple dm commands). Calling dm_udev_wait would make it wait of
> all the commands.
>
> We can use a cookie multiple times and then wait on it once. But we
> can't keep reusing the cookie after we've waited on it, because someone
> else could be using the same cookie.
>
> -Ben
>
>>
>> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
>> ---
>> libmultipath/devmapper.c | 23 ++++++++++++++---------
>> 1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
>> index e6156f7..a96cc0e 100644
>> --- a/libmultipath/devmapper.c
>> +++ b/libmultipath/devmapper.c
>> @@ -274,11 +274,10 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
>>
>> extern int
>> dm_addmap (int task, const char *target, struct multipath *mpp,
>> - char * params, int ro) {
>> + char * params, int ro, uint32_t *cookie) {
>> int r = 0;
>> struct dm_task *dmt;
>> char *prefixed_uuid = NULL;
>> - uint32_t cookie = 0;
>>
>> if (!(dmt = dm_task_create (task)))
>> return 0;
>> @@ -319,18 +318,18 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>> dm_task_no_open_count(dmt);
>>
>> if (task == DM_DEVICE_CREATE &&
>> - !dm_task_set_cookie(dmt, &cookie,
>> + !dm_task_set_cookie(dmt, cookie,
>> DM_UDEV_DISABLE_LIBRARY_FALLBACK)) {
>> - dm_udev_complete(cookie);
>> + dm_udev_complete(*cookie);
>> goto freeout;
>> }
>> r = dm_task_run (dmt);
>>
>> if (task == DM_DEVICE_CREATE) {
>> if (!r)
>> - dm_udev_complete(cookie);
>> + dm_udev_complete(*cookie);
>> else
>> - dm_udev_wait(cookie);
>> + dm_udev_wait(*cookie);
>> }
>> freeout:
>> if (prefixed_uuid)
>> @@ -345,11 +344,13 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
>> extern int
>> dm_addmap_create (struct multipath *mpp, char * params) {
>> int ro;
>> + uint32_t cookie = 0;
>>
>> for (ro = 0; ro <= 1; ro++) {
>> int err;
>>
>> - if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro))
>> + if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH,
>> + mpp, params, ro, &cookie))
>> return 1;
>> /*
>> * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
>> @@ -374,11 +375,15 @@ dm_addmap_create (struct multipath *mpp, char * params) {
>>
>> extern int
>> dm_addmap_reload (struct multipath *mpp, char *params) {
>> - if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RW))
>> + uint32_t cookie = 0;
>> +
>> + if (dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
>> + ADDMAP_RW, &cookie))
>> return 1;
>> if (errno != EROFS)
>> return 0;
>> - return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params, ADDMAP_RO);
>> + return dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
>> + ADDMAP_RO, &cookie);
>> }
>>
>> extern int
>> --
>> 2.6.6
Looking closer, this entire section looks fishy.
>From the lvm source code I couldn't figure out _how_ we could get
into a situation where dm_task_run() would return 0 (ie no error
occurred), but errno would be set to EROFS.
_If_ we could rely on dm_task_run() to return an error here this
whole issue wouldn't occur, as we haven't actually waited on the
cookie, and the second dm_addmap() call would be legit, even with
the same cookie.
Checking with git how the above came about.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
