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

[Zhiqiang Liu]

On 2020/7/16 1:41, Martin Wilck wrote:
> 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
> 

Thanks for your detailed analysis.

Look forward to the new version with solving the issue.

Regards,
Zhiqiang Liu