[dm-devel] [lvm-devel] master - multipathd: fix fd leak when iscsi device logs in

[Benjamin Marzinski]

On Mon, Jul 13, 2020 at 09:59:43PM +0200, Martin Wilck wrote:
> On Mon, 2020-07-13 at 11:56 +0200, Zdenek Kabelac wrote:
> > 
> > > @Zdenek, do we have to protect every libdm call, or is it
> > > sufficient
> > > to protect only dm_task_run(), as lixiaokeng suggested?
> > > 
> > 
> > Hi
> > 
> > It's actually hard to answer it in a simple way.
> > Several properties are held in library static variables.
> >
> > ...
> >
> > As for the issue of keeping control_fd open - there should be a
> > synchronized 
> > call of dm_hold_control_dev(0/1) -  see the codebase of  dmeventd.c
> > in lvm2 
> > code base - how we solve the control_fd handling.
> 
> I've made an assessment of the libdm calls that multipath-tools use.
> 
> Most of them are trivial getters and setters and need no locking,
> because they don't operate on any static or global data structures.
> 
> The exceptions I found are are listed here:
> 
> dm_driver_version
> dm_hold_control_dev
> dm_is_dm_major
> dm_lib_exit
> dm_lib_release
> dm_log_init
> dm_log_init_verbose
> dm_task_create
> dm_task_run
> dm_task_set_cookie
> dm_udev_set_sync_support
> dm_udev_wait
> 
> The reported race around _control_fd could probably be fixed simply
> by calling dm_hold_control_dev() and dm_driver_version() early on e.g.
> via pthread_once(), because dm_driver_version() calls dm_task_create()
> and dm_task_run(), and thus implicitly initializes the _control_fd.
> (libmultipath currently doesn't do these calls in the right order).
> This should also avoid the need for locking dm_is_dm_major() (access to
> _bitset) and dm_task_create (check_version()). The early init function
> could also call dm_log_init(), dm_log_init_verbose(), and
> dm_udev_set_sync_support(), setting up the libdm parameters once and
> for all.
> 
> However, there are other possible races, as you noted. Mainly related
> to manipulating the node_ops stack - this concerns dm_task_run(),
> dm_udev_wait(), dm_lib_release(), dm_lib_exit(). I'm uncertain about 
> dm_task_set_cookie(). I suppose it doesn't need protecting, but I tend
> to be on the safe side.
> 
> Anyway, that's my summary: the 4 or 5 functions mentioned in the
> previous paragraph would need wrapping under a lock. The rest can be
> handled by making sure the initialization is made early on, and only
> once.
> 
> Comments welcome.

We've agreed that dm_lib_release() is unnecessary, since we always call
dm_udev_wait() when update_devs() is needed. dm_lib_exit() should
already be safe, since we only call it after cleaning up the other
threads. dm_task_set_cookie() looks safe to me as well, assuming that we
have early initialization. So we are really talking about dm_task_run()
and dm_udev_wait(). Clearly dm_udev_wait() always needs to run under a
lock. Two threads trying to remove items from a list at the same time
isn't safe, and there's no way to know if there will be items on the
_node_ops list. The one thing I'm not sure of, is whether every call to
dm_task_run() always needs to be locked. clearly we could, and it would
be safer. Also, clearly for calls that add elements to _node_ops it
must. But for calls like DM_DEVICE_TABLE or DM_DEVICE_LIST, the only
issue that I can see with concurrent calls is the possibility that
_ioctl_buffer_double_factor gets increased too much. Perhaps that's
enough of a problem that we should just lock all dm_task_run() calls as
well.

-Ben
 
> Regards
> Martin
>