Return-Path: Message-ID: <51E6DE1D.4020808@hurleysoftware.com> Date: Wed, 17 Jul 2013 14:10:37 -0400 From: Peter Hurley MIME-Version: 1.0 To: Gianluca Anzolin CC: gustavo@padovan.org, linux-bluetooth@vger.kernel.org, marcel@holtmann.org Subject: Re: [PATCH 6/8] Fix the reference counting of tty_port References: <1373661649-1385-1-git-send-email-gianluca@sottospazio.it> <1373661649-1385-6-git-send-email-gianluca@sottospazio.it> <51E6A3F7.20202@hurleysoftware.com> <20130717170500.GA10640@sottospazio.it> In-Reply-To: <20130717170500.GA10640@sottospazio.it> Content-Type: text/plain; charset=UTF-8; format=flowed List-ID: 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