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

[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