When the dlc is closed, rfcomm_dev_state_change() tries to release the
port in the case it cannot get a reference to the tty. However this is
racy and not even needed.
Infact as Peter Hurley points out:
1. Only consider dlcs that are 'stolen' from a connected socket, ie.
reused. Allocated dlcs cannot have been closed prior to port
activate and so for these dlcs a tty reference will always be avail
in rfcomm_dev_state_change() -- except for the conditions covered by
#2b below.
2. If a tty was at some point previously created for this rfcomm, then
either
(a) the tty reference is still avail, so rfcomm_dev_state_change()
will perform a hangup. So nothing to do, or,
(b) the tty reference is no longer avail, and the tty_port will be
destroyed by the last tty_port_put() in rfcomm_tty_cleanup.
Again, no action required.
3. Prior to obtaining the dlc lock in rfcomm_dev_add(),
rfcomm_dev_state_change() will not 'see' a rfcomm_dev so nothing to
do here.
4. After releasing the dlc lock in rfcomm_dev_add(),
rfcomm_dev_state_change() will 'see' an incomplete rfcomm_dev if a
tty reference could not be obtained. Again, the best thing to do here
is nothing. Any future attempted open() will block on
rfcomm_dev_carrier_raised(). The unconnected device will exist until
released by ioctl(RFCOMMRELEASEDEV).
The patch removes the aforementioned code and uses the
tty_port_tty_hangup() helper to hangup the tty.
Signed-off-by: Gianluca Anzolin <[email protected]>
---
net/bluetooth/rfcomm/tty.c | 35 ++---------------------------------
1 file changed, 2 insertions(+), 33 deletions(-)
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 6d126fa..84fcf9f 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -569,7 +569,6 @@ static void rfcomm_dev_data_ready(struct rfcomm_dlc *dlc, struct sk_buff *skb)
static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
{
struct rfcomm_dev *dev = dlc->owner;
- struct tty_struct *tty;
if (!dev)
return;
@@ -581,38 +580,8 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
DPM_ORDER_DEV_AFTER_PARENT);
wake_up_interruptible(&dev->port.open_wait);
- } else if (dlc->state == BT_CLOSED) {
- tty = tty_port_tty_get(&dev->port);
- if (!tty) {
- if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags)) {
- /* Drop DLC lock here to avoid deadlock
- * 1. rfcomm_dev_get will take rfcomm_dev_lock
- * but in rfcomm_dev_add there's lock order:
- * rfcomm_dev_lock -> dlc lock
- * 2. tty_port_put will deadlock if it's
- * the last reference
- *
- * FIXME: when we release the lock anything
- * could happen to dev, even its destruction
- */
- rfcomm_dlc_unlock(dlc);
- if (rfcomm_dev_get(dev->id) == NULL) {
- rfcomm_dlc_lock(dlc);
- return;
- }
-
- if (!test_and_set_bit(RFCOMM_TTY_RELEASED,
- &dev->flags))
- tty_port_put(&dev->port);
-
- tty_port_put(&dev->port);
- rfcomm_dlc_lock(dlc);
- }
- } else {
- tty_hangup(tty);
- tty_kref_put(tty);
- }
- }
+ } else if (dlc->state == BT_CLOSED)
+ tty_port_tty_hangup(&dev->port, false);
}
static void rfcomm_dev_modem_status(struct rfcomm_dlc *dlc, u8 v24_sig)
--
1.8.4
Hi Gianluca,
2013-08-27 Gianluca Anzolin <[email protected]>:
> When the dlc is closed, rfcomm_dev_state_change() tries to release the
> port in the case it cannot get a reference to the tty. However this is
> racy and not even needed.
>
> Infact as Peter Hurley points out:
>
> 1. Only consider dlcs that are 'stolen' from a connected socket, ie.
> reused. Allocated dlcs cannot have been closed prior to port
> activate and so for these dlcs a tty reference will always be avail
> in rfcomm_dev_state_change() -- except for the conditions covered by
> #2b below.
> 2. If a tty was at some point previously created for this rfcomm, then
> either
> (a) the tty reference is still avail, so rfcomm_dev_state_change()
> will perform a hangup. So nothing to do, or,
> (b) the tty reference is no longer avail, and the tty_port will be
> destroyed by the last tty_port_put() in rfcomm_tty_cleanup.
> Again, no action required.
> 3. Prior to obtaining the dlc lock in rfcomm_dev_add(),
> rfcomm_dev_state_change() will not 'see' a rfcomm_dev so nothing to
> do here.
> 4. After releasing the dlc lock in rfcomm_dev_add(),
> rfcomm_dev_state_change() will 'see' an incomplete rfcomm_dev if a
> tty reference could not be obtained. Again, the best thing to do here
> is nothing. Any future attempted open() will block on
> rfcomm_dev_carrier_raised(). The unconnected device will exist until
> released by ioctl(RFCOMMRELEASEDEV).
>
> The patch removes the aforementioned code and uses the
> tty_port_tty_hangup() helper to hangup the tty.
>
> Signed-off-by: Gianluca Anzolin <[email protected]>
> ---
> net/bluetooth/rfcomm/tty.c | 35 ++---------------------------------
> 1 file changed, 2 insertions(+), 33 deletions(-)
Patch has been applied to bluetooth.git. Thanks.
Gustavo
On 08/27/2013 12:28 PM, Gianluca Anzolin wrote:
> When the dlc is closed, rfcomm_dev_state_change() tries to release the
> port in the case it cannot get a reference to the tty. However this is
> racy and not even needed.
>
> Infact as Peter Hurley points out:
>
> 1. Only consider dlcs that are 'stolen' from a connected socket, ie.
> reused. Allocated dlcs cannot have been closed prior to port
> activate and so for these dlcs a tty reference will always be avail
> in rfcomm_dev_state_change() -- except for the conditions covered by
> #2b below.
> 2. If a tty was at some point previously created for this rfcomm, then
> either
> (a) the tty reference is still avail, so rfcomm_dev_state_change()
> will perform a hangup. So nothing to do, or,
> (b) the tty reference is no longer avail, and the tty_port will be
> destroyed by the last tty_port_put() in rfcomm_tty_cleanup.
> Again, no action required.
> 3. Prior to obtaining the dlc lock in rfcomm_dev_add(),
> rfcomm_dev_state_change() will not 'see' a rfcomm_dev so nothing to
> do here.
> 4. After releasing the dlc lock in rfcomm_dev_add(),
> rfcomm_dev_state_change() will 'see' an incomplete rfcomm_dev if a
> tty reference could not be obtained. Again, the best thing to do here
> is nothing. Any future attempted open() will block on
> rfcomm_dev_carrier_raised(). The unconnected device will exist until
> released by ioctl(RFCOMMRELEASEDEV).
>
> The patch removes the aforementioned code and uses the
> tty_port_tty_hangup() helper to hangup the tty.
Sorry for the delay in reviewing.
Reviewed-by: Peter Hurley <[email protected]>
On Fri, Dec 27, 2013 at 04:01:27PM -0700, Benson Chow wrote:
> First off, thanks for the fix to stop rfcomm from taking down the
> machine. However, I have noted that blueman and
> networkmanager/modemmanager no longer recognize the /dev/rfcomm0
> device as a valid dialup device. This seems to be a
> kernel-userspace interface regression as I can boot into 3.6.11 and
> it would work just fine.
>
> When I saw this thread, I agree there appears to be some
> kernel-userspace changes that broke something, but the recent patch
> still did not seem to let modemmanger detect the bluetooth device as
> it did pre linux-3.12.
>
> Blueman reports "connection failed: modem manager did not support
> the connection" implying there's still some userspace differences
> from the old behavior.
>
> I do notice I can run a terminal emulator on /dev/rfcomm0 and able
> to run modem AT-commands which means that I can communicate with the
> phone through bluetooth, so that part is working. Plus, tearing up
> that connection no longer results in a crash like before linux-3.8.
>
> Any other information I could get from my system to help debug
> what's going on here? Or perhaps modem-manager will need to be
> updated to work with the new behavior?
>
> Thanks,
> -Benson
Thank you for the report.
Could you try the attached patch on top of the last rfc3.patch and see if it
works?
Regards,
Gianluca
First off, thanks for the fix to stop rfcomm from taking down the machine.
However, I have noted that blueman and networkmanager/modemmanager no
longer recognize the /dev/rfcomm0 device as a valid dialup device. This
seems to be a kernel-userspace interface regression as I can boot into
3.6.11 and it would work just fine.
When I saw this thread, I agree there appears to be some
kernel-userspace changes that broke something, but the recent patch still
did not seem to let modemmanger detect the bluetooth device as it did pre
linux-3.12.
Blueman reports "connection failed: modem manager did not support
the connection" implying there's still some userspace differences from the
old behavior.
I do notice I can run a terminal emulator on /dev/rfcomm0 and able to run
modem AT-commands which means that I can communicate with the phone
through bluetooth, so that part is working. Plus, tearing up that
connection no longer results in a crash like before linux-3.8.
Any other information I could get from my system to help debug what's
going on here? Or perhaps modem-manager will need to be updated to
work with the new behavior?
Thanks,
-Benson
On Mon, 16 Dec 2013, Gianluca Anzolin wrote:
> Date: Mon, 16 Dec 2013 22:15:42 +0100
> From: Gianluca Anzolin <[email protected]>
> To: Peter Hurley <[email protected]>
> Cc: Alexander Holler <[email protected]>, [email protected],
> Gustavo Padovan <[email protected]>, [email protected],
> [email protected], [email protected], [email protected]
> Subject: Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
>
> On Mon, Dec 16, 2013 at 09:58:58PM +0100, Gianluca Anzolin wrote:
>> On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
>>> On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
>>>> On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
>>>>>
>>>>> This solution is acceptable to me, but I think the comment should briefly
>>>>> explain why this fix is necessary, and the changelog should explain why in detail.
>>>>>
>>>>> Perhaps with a fixme comment that rfcomm_tty_install() should just take over
>>>>> the port reference (instead of adding one) and rfcomm_tty_cleanup() should
>>>>> conditionally release on RFCOMM_RELEASE_ONHUP.
>>>>>
>>>>> Because then:
>>>>> 1) this fix would not be necessary.
>>>>> 2) the release in rfcomm_tty_hangup() would not be necessary
>>>>> 3) the second release in rfcomm_release_dev would not be necessary
>>>>> 4) the RFCOMM_TTY_RELEASED bit could be removed
>>>>>
>>>>>
>>>>> Regards,
>>>>> Peter Hurley
>>>>
>>>> Taking over the refcount in the install method would certainly look better. I'm
>>>> going to test it ASAP :D
>>>>
>>>> But why getting rid of the release in in rfcomm_tty_hangup()?
>>>> We could lose the bluetooth connection at any time and the dlc callback
>>>> would have to hangup the tty (and release the port if necessary).
>>>>
>>>> Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
>>>> created without the RFCOMM_RELEASE_ONHUP flag.
>>>>
>>>> Besides any process could release the port behind us (with the command rfcomm
>>>> release rfcomm1 for example).
>>>>
>>>> Gianluca
>>>
>>> Nevermind I figured it out the reason...
>>
>> I'm testing the attached patch ATM, which does what you described. It works
>> very well.
>>
>> It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
>> that bit.
>
> ok, replying to myself again (sorry for that). RFCOMM_TTY_RELEASE cannot go
> away. We have still to manage the case where the port is opened without
> RFCOMM_RELEASE_ONHUP.
>
> This last patch does release the port in that situation.
>
> Tested with:
> # rfcomm bind 1 <addr>
> # rfcomm release 1
>
> and with
> # rfcomm connect 1 <addr>
>
> Thanks,
> Gianluca
>
Hello,
Am 16.12.2013 22:15, schrieb Gianluca Anzolin:
> This last patch does release the port in that situation.
>
> Tested with:
> # rfcomm bind 1 <addr>
> # rfcomm release 1
>
> and with
> # rfcomm connect 1 <addr>
I've just tested the patch rfc3.patch as attached to the mail I'm
replying to on top of 3.12.6, it works here too.
I've tested 3 use cases:
1. rfcomm connect then ctrl-c,
2. rfcomm connect, screen /dev/rfcommN, ctrl-c for rfcomm then quit screen
3. rfcomm connect, disappearing remote device (hard power down of the
remote device)
Everything worked as expected.
Thanks again, I wish everyone a merry christmas,
Alexander Holler
On Mon, Dec 16, 2013 at 09:58:58PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
> > On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> > > On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> > > >
> > > > This solution is acceptable to me, but I think the comment should briefly
> > > > explain why this fix is necessary, and the changelog should explain why in detail.
> > > >
> > > > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > > > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > > > conditionally release on RFCOMM_RELEASE_ONHUP.
> > > >
> > > > Because then:
> > > > 1) this fix would not be necessary.
> > > > 2) the release in rfcomm_tty_hangup() would not be necessary
> > > > 3) the second release in rfcomm_release_dev would not be necessary
> > > > 4) the RFCOMM_TTY_RELEASED bit could be removed
> > > >
> > > >
> > > > Regards,
> > > > Peter Hurley
> > >
> > > Taking over the refcount in the install method would certainly look better. I'm
> > > going to test it ASAP :D
> > >
> > > But why getting rid of the release in in rfcomm_tty_hangup()?
> > > We could lose the bluetooth connection at any time and the dlc callback
> > > would have to hangup the tty (and release the port if necessary).
> > >
> > > Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> > > created without the RFCOMM_RELEASE_ONHUP flag.
> > >
> > > Besides any process could release the port behind us (with the command rfcomm
> > > release rfcomm1 for example).
> > >
> > > Gianluca
> >
> > Nevermind I figured it out the reason...
>
> I'm testing the attached patch ATM, which does what you described. It works
> very well.
>
> It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
> that bit.
ok, replying to myself again (sorry for that). RFCOMM_TTY_RELEASE cannot go
away. We have still to manage the case where the port is opened without
RFCOMM_RELEASE_ONHUP.
This last patch does release the port in that situation.
Tested with:
# rfcomm bind 1 <addr>
# rfcomm release 1
and with
# rfcomm connect 1 <addr>
Thanks,
Gianluca
On Mon, Dec 16, 2013 at 09:27:20PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> > On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> > >
> > > This solution is acceptable to me, but I think the comment should briefly
> > > explain why this fix is necessary, and the changelog should explain why in detail.
> > >
> > > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > > conditionally release on RFCOMM_RELEASE_ONHUP.
> > >
> > > Because then:
> > > 1) this fix would not be necessary.
> > > 2) the release in rfcomm_tty_hangup() would not be necessary
> > > 3) the second release in rfcomm_release_dev would not be necessary
> > > 4) the RFCOMM_TTY_RELEASED bit could be removed
> > >
> > >
> > > Regards,
> > > Peter Hurley
> >
> > Taking over the refcount in the install method would certainly look better. I'm
> > going to test it ASAP :D
> >
> > But why getting rid of the release in in rfcomm_tty_hangup()?
> > We could lose the bluetooth connection at any time and the dlc callback
> > would have to hangup the tty (and release the port if necessary).
> >
> > Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> > created without the RFCOMM_RELEASE_ONHUP flag.
> >
> > Besides any process could release the port behind us (with the command rfcomm
> > release rfcomm1 for example).
> >
> > Gianluca
>
> Nevermind I figured it out the reason...
I'm testing the attached patch ATM, which does what you described. It works
very well.
It doesn't remove the RFCOMM_TTY_RELEASE flag yet, another patch should remove
that bit.
Does it look better?
Thanks,
Gianluca
On Mon, Dec 16, 2013 at 09:20:44PM +0100, Gianluca Anzolin wrote:
> On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
> >
> > This solution is acceptable to me, but I think the comment should briefly
> > explain why this fix is necessary, and the changelog should explain why in detail.
> >
> > Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> > the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> > conditionally release on RFCOMM_RELEASE_ONHUP.
> >
> > Because then:
> > 1) this fix would not be necessary.
> > 2) the release in rfcomm_tty_hangup() would not be necessary
> > 3) the second release in rfcomm_release_dev would not be necessary
> > 4) the RFCOMM_TTY_RELEASED bit could be removed
> >
> >
> > Regards,
> > Peter Hurley
>
> Taking over the refcount in the install method would certainly look better. I'm
> going to test it ASAP :D
>
> But why getting rid of the release in in rfcomm_tty_hangup()?
> We could lose the bluetooth connection at any time and the dlc callback
> would have to hangup the tty (and release the port if necessary).
>
> Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
> created without the RFCOMM_RELEASE_ONHUP flag.
>
> Besides any process could release the port behind us (with the command rfcomm
> release rfcomm1 for example).
>
> Gianluca
Nevermind I figured it out the reason...
On Mon, Dec 16, 2013 at 02:34:12PM -0500, Peter Hurley wrote:
>
> This solution is acceptable to me, but I think the comment should briefly
> explain why this fix is necessary, and the changelog should explain why in detail.
>
> Perhaps with a fixme comment that rfcomm_tty_install() should just take over
> the port reference (instead of adding one) and rfcomm_tty_cleanup() should
> conditionally release on RFCOMM_RELEASE_ONHUP.
>
> Because then:
> 1) this fix would not be necessary.
> 2) the release in rfcomm_tty_hangup() would not be necessary
> 3) the second release in rfcomm_release_dev would not be necessary
> 4) the RFCOMM_TTY_RELEASED bit could be removed
>
>
> Regards,
> Peter Hurley
Taking over the refcount in the install method would certainly look better. I'm
going to test it ASAP :D
But why getting rid of the release in in rfcomm_tty_hangup()?
We could lose the bluetooth connection at any time and the dlc callback
would have to hangup the tty (and release the port if necessary).
Also the RFCOMM_TTY_RELEASED bit should still be necessary if the port is
created without the RFCOMM_RELEASE_ONHUP flag.
Besides any process could release the port behind us (with the command rfcomm
release rfcomm1 for example).
Gianluca
On 12/15/2013 10:08 AM, Gianluca Anzolin wrote:
> On Sun, Dec 15, 2013 at 09:03:35AM -0500, Peter Hurley wrote:
>> On 12/15/2013 06:24 AM, Gianluca Anzolin wrote:
>>> On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote:
>>>> Am 12.12.2013 21:36, schrieb Peter Hurley:
>>>>
>>>>>> What currently happens is that when one kills rfcomm (and any other
>>>>>> terminal which might use that tty), the entry in /dev doesn't
>>>>>> disappear. That means the same call to refcomm with the same device
>>>>>> (e.g. [/dev/]rfcomm1 doesn't work.
>>>>>
>>>>> Thanks for the report, Alexander.
>>>>>
>>>>> Point 4 above details a different situation; something else is
>>>>> happening.
>>>>>
>>>>> Would you please detail the necessary steps to reproduce this regression?
>>>>> (How do you 'kill' rfcomm? etc. Shell command lines would be best.)
>>>>
>>>> Just call
>>>>
>>>> rfcomm connect rfcomm9 01:23:45:67:89:ab
>>>>
>>>> wait until the connection happened (a message will appear) and then
>>>> press ctrl-c. This still terminates the bluetooth connection, but the
>>>> device in /dev is now left.
>>>
>>> Yes I'm able to reproduce the regression which is indeed caused by that
>>> commit.
>>>
>>> However I'm puzzled. Surely there is a fifth case I didn't cover because
>>> when rfcomm_dev_state_change() is called, the tty_port is there but the tty is
>>> not, and therefore I cannot get a reference to it and send the HUP.
>>
>> There is a fifth case, but it's crazy.
>>
>> The tty has been properly shutdown and destroyed because the tty file handle
>> was closed, not hungup. The rfcomm device reference was properly put
>> when the tty was released.
>>
>> But when the remote hangs up (and sends disc), then rfcomm_dev_state_change()
>> is called -- to kill the port reference (thus the rfcomm device) that was
>> instantiated locally! Ridiculous. Doubly ridiculous because it's the local
>> port shutdown that closes the dlc locally that sends the disconnect (and sets
>> the local dlc state) that triggers the received rfcomm_dev_state_change()!
>>
>> If this behavior is desirable (or necessary because it's been exposed to
>> userspace), then why was the design ever reference-counted to begin with?
>>
>> Regards,
>> Peter Hurley
>
> The attached patch fixes the regression by releasing the tty_port in the
> shutdown method(). This way we can avoid strange games in the dlc callback
> where we are constrained by the dlc lock.
>
> If this kind of approach is acceptable I will submit the patch for inclusion in
> a separate email.
This solution is acceptable to me, but I think the comment should briefly
explain why this fix is necessary, and the changelog should explain why in detail.
Perhaps with a fixme comment that rfcomm_tty_install() should just take over
the port reference (instead of adding one) and rfcomm_tty_cleanup() should
conditionally release on RFCOMM_RELEASE_ONHUP.
Because then:
1) this fix would not be necessary.
2) the release in rfcomm_tty_hangup() would not be necessary
3) the second release in rfcomm_release_dev would not be necessary
4) the RFCOMM_TTY_RELEASED bit could be removed
Regards,
Peter Hurley
Am 15.12.2013 16:08, schrieb Gianluca Anzolin:
> The attached patch fixes the regression by releasing the tty_port in the
> shutdown method(). This way we can avoid strange games in the dlc callback
> where we are constrained by the dlc lock.
Yes, it fixes the problem here too.
> If this kind of approach is acceptable I will submit the patch for inclusion in
> a separate email.
If you do so, please add a Cc: [email protected] so that at least
3.12 will have a working rfcomm. You might also add a Reported-by:
and/or Tested-by: <[email protected]> if that makes someone happy ;)
I've missed this patch previously, because it came late and I was happy
with the series of patches you've already had done.
Thanks a lot (for the previous series too).
Regards,
Alexander Holler
On Sun, Dec 15, 2013 at 09:03:35AM -0500, Peter Hurley wrote:
> On 12/15/2013 06:24 AM, Gianluca Anzolin wrote:
> >On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote:
> >>Am 12.12.2013 21:36, schrieb Peter Hurley:
> >>
> >>>>What currently happens is that when one kills rfcomm (and any other
> >>>>terminal which might use that tty), the entry in /dev doesn't
> >>>>disappear. That means the same call to refcomm with the same device
> >>>>(e.g. [/dev/]rfcomm1 doesn't work.
> >>>
> >>>Thanks for the report, Alexander.
> >>>
> >>>Point 4 above details a different situation; something else is
> >>>happening.
> >>>
> >>>Would you please detail the necessary steps to reproduce this regression?
> >>>(How do you 'kill' rfcomm? etc. Shell command lines would be best.)
> >>
> >>Just call
> >>
> >>rfcomm connect rfcomm9 01:23:45:67:89:ab
> >>
> >>wait until the connection happened (a message will appear) and then
> >>press ctrl-c. This still terminates the bluetooth connection, but the
> >>device in /dev is now left.
> >
> >Yes I'm able to reproduce the regression which is indeed caused by that
> >commit.
> >
> >However I'm puzzled. Surely there is a fifth case I didn't cover because
> >when rfcomm_dev_state_change() is called, the tty_port is there but the tty is
> >not, and therefore I cannot get a reference to it and send the HUP.
>
> There is a fifth case, but it's crazy.
>
> The tty has been properly shutdown and destroyed because the tty file handle
> was closed, not hungup. The rfcomm device reference was properly put
> when the tty was released.
>
> But when the remote hangs up (and sends disc), then rfcomm_dev_state_change()
> is called -- to kill the port reference (thus the rfcomm device) that was
> instantiated locally! Ridiculous. Doubly ridiculous because it's the local
> port shutdown that closes the dlc locally that sends the disconnect (and sets
> the local dlc state) that triggers the received rfcomm_dev_state_change()!
>
> If this behavior is desirable (or necessary because it's been exposed to
> userspace), then why was the design ever reference-counted to begin with?
>
> Regards,
> Peter Hurley
The attached patch fixes the regression by releasing the tty_port in the
shutdown method(). This way we can avoid strange games in the dlc callback
where we are constrained by the dlc lock.
If this kind of approach is acceptable I will submit the patch for inclusion in
a separate email.
Thanks,
Gianluca
On 12/15/2013 06:24 AM, Gianluca Anzolin wrote:
> On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote:
>> Am 12.12.2013 21:36, schrieb Peter Hurley:
>>
>>>> What currently happens is that when one kills rfcomm (and any other
>>>> terminal which might use that tty), the entry in /dev doesn't
>>>> disappear. That means the same call to refcomm with the same device
>>>> (e.g. [/dev/]rfcomm1 doesn't work.
>>>
>>> Thanks for the report, Alexander.
>>>
>>> Point 4 above details a different situation; something else is
>>> happening.
>>>
>>> Would you please detail the necessary steps to reproduce this regression?
>>> (How do you 'kill' rfcomm? etc. Shell command lines would be best.)
>>
>> Just call
>>
>> rfcomm connect rfcomm9 01:23:45:67:89:ab
>>
>> wait until the connection happened (a message will appear) and then
>> press ctrl-c. This still terminates the bluetooth connection, but the
>> device in /dev is now left.
>
> Yes I'm able to reproduce the regression which is indeed caused by that
> commit.
>
> However I'm puzzled. Surely there is a fifth case I didn't cover because
> when rfcomm_dev_state_change() is called, the tty_port is there but the tty is
> not, and therefore I cannot get a reference to it and send the HUP.
There is a fifth case, but it's crazy.
The tty has been properly shutdown and destroyed because the tty file handle
was closed, not hungup. The rfcomm device reference was properly put
when the tty was released.
But when the remote hangs up (and sends disc), then rfcomm_dev_state_change()
is called -- to kill the port reference (thus the rfcomm device) that was
instantiated locally! Ridiculous. Doubly ridiculous because it's the local
port shutdown that closes the dlc locally that sends the disconnect (and sets
the local dlc state) that triggers the received rfcomm_dev_state_change()!
If this behavior is desirable (or necessary because it's been exposed to
userspace), then why was the design ever reference-counted to begin with?
Regards,
Peter Hurley
On Fri, Dec 13, 2013 at 12:35:26AM +0100, Alexander Holler wrote:
> Am 12.12.2013 21:36, schrieb Peter Hurley:
>
> >> What currently happens is that when one kills rfcomm (and any other
> >> terminal which might use that tty), the entry in /dev doesn't
> >> disappear. That means the same call to refcomm with the same device
> >> (e.g. [/dev/]rfcomm1 doesn't work.
> >
> > Thanks for the report, Alexander.
> >
> > Point 4 above details a different situation; something else is
> > happening.
> >
> > Would you please detail the necessary steps to reproduce this regression?
> > (How do you 'kill' rfcomm? etc. Shell command lines would be best.)
>
> Just call
>
> rfcomm connect rfcomm9 01:23:45:67:89:ab
>
> wait until the connection happened (a message will appear) and then
> press ctrl-c. This still terminates the bluetooth connection, but the
> device in /dev is now left.
Yes I'm able to reproduce the regression which is indeed caused by that
commit.
However I'm puzzled. Surely there is a fifth case I didn't cover because
when rfcomm_dev_state_change() is called, the tty_port is there but the tty is
not, and therefore I cannot get a reference to it and send the HUP.
I'll let you know when I have something working.
>
> Regards,
>
> Alexander Holler
Am 12.12.2013 21:36, schrieb Peter Hurley:
>> What currently happens is that when one kills rfcomm (and any other
>> terminal which might use that tty), the entry in /dev doesn't
>> disappear. That means the same call to refcomm with the same device
>> (e.g. [/dev/]rfcomm1 doesn't work.
>
> Thanks for the report, Alexander.
>
> Point 4 above details a different situation; something else is
> happening.
>
> Would you please detail the necessary steps to reproduce this regression?
> (How do you 'kill' rfcomm? etc. Shell command lines would be best.)
Just call
rfcomm connect rfcomm9 01:23:45:67:89:ab
wait until the connection happened (a message will appear) and then
press ctrl-c. This still terminates the bluetooth connection, but the
device in /dev is now left.
Regards,
Alexander Holler
On 12/12/2013 03:11 PM, Alexander Holler wrote:
> Hello,
>
> since commit 29cd718beba999bda4bdbbf59b5a4d25c07e1547 "rfcomm: don't release the port in rfcomm_dev_state_change()" the userland utility rfcomm (both from bluez 4.101 and 5.12) is broken.
>
> In detail the following note in the patch
>
> Am 19.09.2013 18:24, schrieb Gustavo Padovan:
>> Hi Gianluca,
>>
>> 2013-08-27 Gianluca Anzolin <[email protected]>:
>>
>>> When the dlc is closed, rfcomm_dev_state_change() tries to release the
>>> port in the case it cannot get a reference to the tty. However this is
>>> racy and not even needed.
>>>
>>> Infact as Peter Hurley points out:
>
> (...)
>
>>> 4. After releasing the dlc lock in rfcomm_dev_add(),
>>> rfcomm_dev_state_change() will 'see' an incomplete rfcomm_dev if a
>>> tty reference could not be obtained. Again, the best thing to do here
>>> is nothing. Any future attempted open() will block on
>>> rfcomm_dev_carrier_raised(). The unconnected device will exist until
>>> released by ioctl(RFCOMMRELEASEDEV).
>>>
>>> The patch removes the aforementioned code and uses the
>>> tty_port_tty_hangup() helper to hangup the tty.
>
> reads like the usage of that ioctl now necessary.
>
> What currently happens is that when one kills rfcomm (and any other terminal which might use that tty), the entry in /dev doesn't disappear. That means the same call to refcomm with the same device (e.g. [/dev/]rfcomm1 doesn't work.
Thanks for the report, Alexander.
Point 4 above details a different situation; something else is
happening.
Would you please detail the necessary steps to reproduce this regression?
(How do you 'kill' rfcomm? etc. Shell command lines would be best.)
> My current solution is to just revert that commit.
> I haven't tested if modifying (the userland utility) rfcomm (adding that ioctl) will help, but that looks only like a longterm solution.
Changes to userspace should not be required; rfcomm should be
handling teardown without help.
Regards,
Peter Hurley
Hello,
since commit 29cd718beba999bda4bdbbf59b5a4d25c07e1547 "rfcomm: don't
release the port in rfcomm_dev_state_change()" the userland utility
rfcomm (both from bluez 4.101 and 5.12) is broken.
In detail the following note in the patch
Am 19.09.2013 18:24, schrieb Gustavo Padovan:
> Hi Gianluca,
>
> 2013-08-27 Gianluca Anzolin <[email protected]>:
>
>> When the dlc is closed, rfcomm_dev_state_change() tries to release the
>> port in the case it cannot get a reference to the tty. However this is
>> racy and not even needed.
>>
>> Infact as Peter Hurley points out:
(...)
>> 4. After releasing the dlc lock in rfcomm_dev_add(),
>> rfcomm_dev_state_change() will 'see' an incomplete rfcomm_dev if a
>> tty reference could not be obtained. Again, the best thing to do here
>> is nothing. Any future attempted open() will block on
>> rfcomm_dev_carrier_raised(). The unconnected device will exist until
>> released by ioctl(RFCOMMRELEASEDEV).
>>
>> The patch removes the aforementioned code and uses the
>> tty_port_tty_hangup() helper to hangup the tty.
reads like the usage of that ioctl now necessary.
What currently happens is that when one kills rfcomm (and any other
terminal which might use that tty), the entry in /dev doesn't disappear.
That means the same call to refcomm with the same device (e.g.
[/dev/]rfcomm1 doesn't work.
My current solution is to just revert that commit.
I haven't tested if modifying (the userland utility) rfcomm (adding that
ioctl) will help, but that looks only like a longterm solution.
Regards,
Alexander Holler
Gianluca,
This patch looks like it did it, from a quick check, everything looks like
it's working as well as what it was before the 3.8 patch.
Thanks a bunch!
-Benson
On Sat, 28 Dec 2013, Gianluca Anzolin wrote:
> Date: Sat, 28 Dec 2013 09:44:21 +0100
> From: Gianluca Anzolin <[email protected]>
> To: Benson Chow <[email protected]>
> Cc: [email protected]
> Subject: Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b
>
> On Fri, Dec 27, 2013 at 04:01:27PM -0700, Benson Chow wrote:
>> First off, thanks for the fix to stop rfcomm from taking down the
>> machine. However, I have noted that blueman and
>> networkmanager/modemmanager no longer recognize the /dev/rfcomm0
>> device as a valid dialup device. This seems to be a
>> kernel-userspace interface regression as I can boot into 3.6.11 and
>> it would work just fine.
>>
>> When I saw this thread, I agree there appears to be some
>> kernel-userspace changes that broke something, but the recent patch
>> still did not seem to let modemmanger detect the bluetooth device as
>> it did pre linux-3.12.
>>
>> Blueman reports "connection failed: modem manager did not support
>> the connection" implying there's still some userspace differences
>> from the old behavior.
>>
>> I do notice I can run a terminal emulator on /dev/rfcomm0 and able
>> to run modem AT-commands which means that I can communicate with the
>> phone through bluetooth, so that part is working. Plus, tearing up
>> that connection no longer results in a crash like before linux-3.8.
>>
>> Any other information I could get from my system to help debug
>> what's going on here? Or perhaps modem-manager will need to be
>> updated to work with the new behavior?
>>
>> Thanks,
>> -Benson
>
> Thank you for the report.
>
> Could you try the attached patch on top of the last rfc3.patch and see if it
> works?
>
> Regards,
> Gianluca
>
WARNING: All HTML emails get auto deleted. DO NOT SEND HTML MAIL.
WARNING: Emails with large typo counts/spelling errors will also be deleted.