[PATCH] rfcomm: don't release the port in rfcomm_dev_state_change()

[PATCH] rfcomm: don't release the port in rfcomm_dev_state_change()

[Gianluca Anzolin]

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 <gianluca@sottospazio.it>
---
 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

Re: [PATCH] rfcomm: don't release the port in rfcomm_dev_state_change()

[Peter Hurley]

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 <peter@hurleysoftware.com>

Re: [PATCH] rfcomm: don't release the port in rfcomm_dev_state_change()

[Gustavo Padovan]

Hi Gianluca,

2013-08-27 Gianluca Anzolin <gianluca@sottospazio.it>:

> 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 <gianluca@sottospazio.it>
> ---
>  net/bluetooth/rfcomm/tty.c | 35 ++---------------------------------
>  1 file changed, 2 insertions(+), 33 deletions(-)

Patch has been applied to bluetooth.git. Thanks.

	Gustavo

[REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Alexander Holler]

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 <gianluca@sottospazio.it>:
>
>> 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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Peter Hurley]

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 <gianluca@sottospazio.it>:
>>
>>> 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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Peter Hurley]

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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

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

[-- Attachment #2: rfc.patch --]
[-- Type: text/x-diff, Size: 578 bytes --]

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..917b441 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -129,6 +129,11 @@ static void rfcomm_dev_shutdown(struct tty_port *port)
 
 	/* close the dlc */
 	rfcomm_dlc_close(dev->dlc, 0);
+
+	/* release the port if it was created with the flag RELEASE_ONHUP */
+	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
+	    !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+		tty_port_put(&dev->port);
 }
 
 static const struct tty_port_operations rfcomm_port_ops = {

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Alexander Holler]

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: stable@vger.kernel.org so that at least 
3.12 will have a working rfcomm. You might also add a Reported-by: 
and/or Tested-by: <holler@ahsoftware.de> 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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Peter Hurley]

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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

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...

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]

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

[-- Attachment #2: rfc2.patch --]
[-- Type: text/x-diff, Size: 1221 bytes --]

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..f455a22 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,9 +437,6 @@ static int rfcomm_release_dev(void __user *arg)
 		tty_kref_put(tty);
 	}
 
-	if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
-		tty_port_put(&dev->port);
-
 	tty_port_put(&dev->port);
 	return 0;
 }
@@ -673,6 +670,14 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 	if (err)
 		rfcomm_tty_cleanup(tty);
 
+	/* take over the tty_port reference if it was created with the
+	 * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+	 * when the last process closes the tty. This behaviour is expected by
+	 * userspace.
+	 */
+	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+		tty_port_put(&dev->port);
+
 	return err;
 }
 
@@ -1010,10 +1015,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
 	BT_DBG("tty %p dev %p", tty, dev);
 
 	tty_port_hangup(&dev->port);
-
-	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
-	    !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
-		tty_port_put(&dev->port);
 }
 
 static int rfcomm_tty_tiocmget(struct tty_struct *tty)

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

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

[-- Attachment #2: rfc3.patch --]
[-- Type: text/x-diff, Size: 1319 bytes --]

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 84fcf9f..0357dcf 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -437,7 +437,8 @@ static int rfcomm_release_dev(void __user *arg)
 		tty_kref_put(tty);
 	}
 
-	if (!test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
+	if (!test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
+	    !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
 		tty_port_put(&dev->port);
 
 	tty_port_put(&dev->port);
@@ -673,6 +674,14 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 	if (err)
 		rfcomm_tty_cleanup(tty);
 
+	/* take over the tty_port reference if it was created with the
+	 * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
+	 * when the last process closes the tty. This behaviour is expected by
+	 * userspace.
+	 */
+	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags))
+		tty_port_put(&dev->port);
+
 	return err;
 }
 
@@ -1010,10 +1019,6 @@ static void rfcomm_tty_hangup(struct tty_struct *tty)
 	BT_DBG("tty %p dev %p", tty, dev);
 
 	tty_port_hangup(&dev->port);
-
-	if (test_bit(RFCOMM_RELEASE_ONHUP, &dev->flags) &&
-	    !test_and_set_bit(RFCOMM_TTY_RELEASED, &dev->flags))
-		tty_port_put(&dev->port);
 }
 
 static int rfcomm_tty_tiocmget(struct tty_struct *tty)

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Alexander Holler]

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

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Benson Chow]

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 <gianluca@sottospazio.it>
> To: Peter Hurley <peter@hurleysoftware.com>
> Cc: Alexander Holler <holler@ahsoftware.de>, marcel@holtmann.org,
>     Gustavo Padovan <gustavo@padovan.org>, linux-bluetooth@vger.kernel.org,
>     gregkh@linuxfoundation.org, jslaby@suse.cz, linux-kernel@vger.kernel.org
> 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
>

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Gianluca Anzolin]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

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

[-- Attachment #2: modman.patch --]
[-- Type: text/x-diff, Size: 1558 bytes --]

diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c
index 0357dcf..90c1872 100644
--- a/net/bluetooth/rfcomm/tty.c
+++ b/net/bluetooth/rfcomm/tty.c
@@ -124,9 +124,6 @@ static void rfcomm_dev_shutdown(struct tty_port *port)
 {
 	struct rfcomm_dev *dev = container_of(port, struct rfcomm_dev, port);
 
-	if (dev->tty_dev->parent)
-		device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
-
 	/* close the dlc */
 	rfcomm_dlc_close(dev->dlc, 0);
 }
@@ -577,9 +574,6 @@ static void rfcomm_dev_state_change(struct rfcomm_dlc *dlc, int err)
 
 	dev->err = err;
 	if (dlc->state == BT_CONNECTED) {
-		device_move(dev->tty_dev, rfcomm_get_device(dev),
-			    DPM_ORDER_DEV_AFTER_PARENT);
-
 		wake_up_interruptible(&dev->port.open_wait);
 	} else if (dlc->state == BT_CLOSED)
 		tty_port_tty_hangup(&dev->port, false);
@@ -632,6 +626,9 @@ static void rfcomm_tty_cleanup(struct tty_struct *tty)
 {
 	struct rfcomm_dev *dev = tty->driver_data;
 
+	if (dev->tty_dev->parent)
+		device_move(dev->tty_dev, NULL, DPM_ORDER_DEV_LAST);
+
 	clear_bit(RFCOMM_TTY_ATTACHED, &dev->flags);
 
 	rfcomm_dlc_lock(dev->dlc);
@@ -674,6 +671,9 @@ static int rfcomm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 	if (err)
 		rfcomm_tty_cleanup(tty);
 
+	device_move(dev->tty_dev, rfcomm_get_device(dev),
+		    DPM_ORDER_DEV_AFTER_PARENT);
+
 	/* take over the tty_port reference if it was created with the
 	 * flag RFCOMM_RELEASE_ONHUP. This will force the release of the port
 	 * when the last process closes the tty. This behaviour is expected by

Re: [REGRESSION] rfcomm (userland) broken by commit 29cd718b

[Benson Chow]

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 <gianluca@sottospazio.it>
> To: Benson Chow <blc+bluez@mail.vanade.com>
> Cc: linux-bluetooth@vger.kernel.org
> 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.