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

[Zdenek Kabelac]

Dne 13. 07. 20 v 12:08 Martin Wilck napsal(a):
> Hi Zdenek,
> 
> On Mon, 2020-07-13 at 11:56 +0200, Zdenek Kabelac wrote:
>> Dne 13. 07. 20 v 11:21 Martin Wilck napsal(a):
>>> Hi Lixiaokeng,
>>>
>>>
>>> @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. So
>> converting libdm
>> into a fully threaded 'version' would basically require to duplicate
>> all API
>> functions with extended 'context' structure passed in - where all
>> buffers can
>> be maintained properly (and it's getting more complicated with signal
>> handling
>> and debug logging).
> 
> Sure. I don't think this is where we're going for. But we need to
> figure out how to handle this properly in libmultipath.
> I take it that we must really protect *every* libdm call in
> libmultipath and multipathd if we want to do it right.
> 
>> ATM it doesn't look like there is a big need for threaded support of
>> DM usage
>> as majority of tools spends most of their time outside thus
>> 'serialization'
>> of lvm2 or dmeventd on libdm access look doesn't look like a big
>> issue
>> (let's say there are far bigger fishes to hunt).
> 
> Agreed.
> 
>> 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.
> 
> Ben has already added support for dm_hold_control_dev() in libmultipath
> (e24d8b1 ("libmutipath: don't close fd on dm_lib_release")). But this
> doesn't protect us from calling _open_control() simultaneously in
> separate code paths, as Lixiaokeng has pointed out.
> 
> I don't see how it would, as dm_hold_control_dev() also just changes
> a static variable.


Here I'm not familiar with  multipath codebase (more a question for Ben 
likely), but is 'dm_hold...()' being used in non-threaded way - i.e. in 
dmeventd these functions are called  before/after threads are finished - so 
there would be no way 2 threads may call this function in parallel ?

Zdenek