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

[Martin Wilck]

On Tue, 2020-07-14 at 17:22 -0500, Benjamin Marzinski wrote:
> On Mon, Jul 13, 2020 at 09:59:43PM +0200, Martin Wilck wrote:
> > 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.

Yes. We shouldn't try to be too smart. It's easier to verify that every
call is wrapped under a lock than to figure out later whether we made
the case distinction right.

So, action plan:

 - remove dm_lib_release() calls
 - Combine all possibly racy calls related to libdm initialization in
   some early init call, protected by pthread_once()
 - make sure dm_lib_exit() is handled correctly
 - add locking around dm_task_run() and dm_udev_wait()

Regards
Martin