On 07/27/2013 12:48 AM, Gianluca Anzolin wrote:
On Fri, Jul 26, 2013 at 08:20:47PM -0400, Peter Hurley wrote:
On 07/26/2013 01:18 PM, Gianluca Anzolin wrote:
@@ -614,7 +601,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);
Since this code can execute concurrently with rfcomm_release_dev(),
and the 'initial' port reference must only be dropped once, this should be
if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags)
tty_port_put(&dev->port);
Regards,
Peter Hurley
I somehow convinced myself that it was safe but clearly it wasn't. Should I
also change the same way the code in rfcomm_tty_hangup()?
Yes, I think that's wise. For example,
CPU 0 | CPU 1
|
rfcomm_dev_state_change |
tty_port_tty_get |
tty_hangup | rfcomm_release_dev
. | .
. | .
. | .
do_tty_hangup | .
__tty_hangup | .
rfcomm_tty_hangup | .
tty_port_hangup | .
| if (!test_and_set_bit(RFCOMM_TTY_RELEASED))
| tty_port_put
set_bit(RFCOMM_TTY_RELEASED) |
tty_port_put |
Also, in the commit message you should explain that
!test_and_set_bit(RFCOMM_TTY_RELEASED) ensures that the 'initial'
tty_port reference is only dropped once.
On a side note, I think this exposes a flaw in the tty layer hangup handling.
It seems possible that an async hangup could be scheduled while a
sync hangup is in-progress (and vice versa). While the two hangups would
not execute concurrently (because the tty lock serializes hangups),
they would execute sequentially and that would be bad.
I'm testing a mock-up now.
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
