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

[Martin Wilck]

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.

Regards
Martin