[dm-devel] [PATCH 36/57] libmultipath: pass in 'cookie' as argument for dm_addmap()

[Benjamin Marzinski]

On Tue, May 03, 2016 at 11:31:19AM +0200, Hannes Reinecke wrote:
> 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 at suse.de>
> >> ---
> >>  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.

dm_task_run returns a 0 on error, not success.

-Ben

> 
> _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 at suse.de			               +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)