[dm-devel] [PATCH 56/57] multipathd: push down lock in checkerloop()

[Hannes Reinecke]

On 05/04/2016 12:17 AM, Benjamin Marzinski wrote:
> On Wed, Apr 27, 2016 at 01:10:57PM +0200, Hannes Reinecke wrote:
>> Instead of grabbing the lock at the start of the checkerloop
>> and releasing it at the end we should be holding it only
>> during the time when we actually need it.
> 
> I'm pretty sure that this can cause crashes if multipathd reconfigures
> when it's just started servicing a uevent. It goes like this. The
> uevq_thr thread calls uev_trigger(), which checks the running_state and
> sees that it's DAEMON_IDLE.  The uxlsnr_thr thread gets a
> reconfiguration request, which eventually calls set_config_state().
> set_config_state() sees the state is DAEMON_IDLE, sets it to
> DAEMON_CONFIGURE, and returns without waiting.  This wakes up the child
> thread, which runs reconfigure().  reconfigure() sets conf to NULL. Then
> the uevq_thr thread in uev_trigger() calls filter_devnode()
> dereferencing conf->blist_devnode and conf->elist_devnode. conf is NULL,
> and thus it crashes.
> 
> The short version is that multipathd has been using the vecs lock to
> protect conf.  With your change to uev_trigger, you access conf outside
> of the vecs lock. This isn't a problem in checkerloop(), since you can't
> reconfigure while multipathd is in the DAEMON_RUNNING state, so that is
> protecting conf. You need to do the same
> set_config_state(DAEMON_RUNNING) in uev_trigger to make this safe.
> 
I have now moved the call to 'filter_devnode' into the individual
functions, so that it will be called with the vector lock held.

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)