Re: [PATCH 6/8] Fix the reference counting of tty_port

[Peter Hurley <peter@hurleysoftware.com>]

On 07/17/2013 01:05 PM, Gianluca Anzolin wrote:
> On Wed, Jul 17, 2013 at 10:02:31AM -0400, Peter Hurley wrote:
>>>
>>> @@ -687,7 +665,9 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
>>>   					return;
>>>   				}
>>>
>>> -				rfcomm_dev_del(dev);
>>> +				set_bit(RFCOMM_TTY_RELEASED, &dev->flags);
>>> +				tty_port_put(&dev->port);
>>> +
>>>   				tty_port_put(&dev->port);
>>>   				rfcomm_dlc_lock(dlc);
>>>   			}
>>
>> While this is functionally correct, it ignores the larger issue in
>> rfcomm_dev_state_change(); namely, what prevents the rfcomm_dev from being
>> destructed immediately after
>>
>> 	struct rfcomm_dev *dev = dlc->owner;
>>
>> If the answer to that question is the dlc lock, then the whole function is
>> _broken_.
>>
>> No amount of reference counting will prevent the rfcomm_dev destructor
>> from completing once the dlc lock is dropped. (Presumably the dlc is not
>> subject to destruction once the lock is dropped. Is this true?)
>>
>> This means:
>> 1. Holding the dlc lock from the caller is pointless and should be dropped.
>> 2. Some other solution is required to either preserve rfcomm_dev lifetime
>>     or determine that destruction is already in progress.
>
> I'm afraid I lied in the commit message: there are three places where the
> tty_port may be released and the code above is the third one.
>
> I wrote that message because at first I wanted to remove that code path but
> then noticed I shouldn't.
>
> Maybe we can simply save the dev->id before releasing the lock and then feed
> that integer to rfcomm_dev_get? After all if the destruction is in progress
> rfcomm_dev_get(id) == NULL and we return immediately. Otherwise we release the
> tty_port.

I had forgotten about your earlier change to remove the rfcomm_dev node
from the list as the first op of the destructor. That does ensure
rfcomm_dev_get(id) will not return a rfcomm_dev which is destructing.

That said, preventing rfcomm_dev destruction by holding the dlc lock
is poor design (not that I'm suggesting you should be required to fix it though)
and something that at least needs documenting.

Regarding acquiring a snapshot of dev->id is fine, provided that the id
cannot be reallocated in between dropping the dlc lock and subsequently
scanning the rfcomm_dev_list for that id.

Regards,
Peter Hurley