Re: [PATCH 00/24] rfcomm fixes

Marcel Holtmann
2014-02-14T21:45:53Z

Hi Peter,

> This patch series addresses a number of previously unknown issues
> with the RFCOMM tty device implementation, in addition to
> addressing the locking regression recently reported [1].
>
> As Gianluca suggested and I agree, this series first reverts
> 3 of the 4 patches of 3.14-rc1 for bluetooth/rfcomm/tty.c.
>
> The reasoning is detailed in the changelog for
> Revert "Bluetooth: Always wait for a connection on RFCOMM open()"
> but the short answer is that it re-implements a long-standing
> bug by blocking on a non-blocking open.
>
> This patch series corrects the reported regressions from 3.13
> (to the extent that correction is required). Specifically,
> the ModemManager regression reported by Gianluca Anzolin [2]
> and the rfcomm bind with wvdial reported by Andrey Vihrov [3].
>
> tty: Fix ref counting for port krefs
> Bluetooth: Fix racy acquire of rfcomm_dev reference
> Bluetooth: Exclude released devices from RFCOMMGETDEVLIST ioctl
> Bluetooth: Release rfcomm_dev only once
> Bluetooth: Fix unreleased rfcomm_dev reference
> These first 5 patches after the reverts
> fix 4 different rfcomm_dev ref count mishandling bugs.
>
> Bluetooth: Fix RFCOMM tty teardown race and
> Bluetooth: Serialize RFCOMMCREATEDEV and RFCOMMRELEASEDEV ioctls
> Fix races which occur due to the design of the rfcomm ioctls
> (note that buses don't have these kinds of races).
>
> Bluetooth: Verify dlci not in use before rfcomm_dev create
> Bluetooth: Simplify RFCOMM session state eval
> Bluetooth: Refactor deferred setup test in rfcomm_dlc_close()
> Bluetooth: Refactor dlc disconnect logic in rfcomm_dlc_close()
> Bluetooth: Directly close dlc for not yet started RFCOMM session
> These 5 patches fix issues with reusing the dlci after
> closing the tty (found by unit test).
>
> Bluetooth: Fix unsafe RFCOMM device parenting
> Bluetooth: Fix RFCOMM parent device for reused dlc
> These 2 patches fix the ModemManager regression.
>
> Bluetooth: Refactor rfcomm_dev_add()
> Bluetooth: Cleanup RFCOMM device registration error handling
> These 2 patches fix an unreleased module reference while
> error handling.
>
> Bluetooth: Rename __rfcomm_dev_get() to __rfcomm_dev_lookup()
> This is a trivial naming patch with no functional impact.
>
> Bluetooth: Force -EIO from tty read/write if .activate() fails
> The tty core provides an existing mechanism for failing
> reads/writes if device activation fails (like an error
> allocating the dlc).
>
> Bluetooth: Don't fail RFCOMM tty writes
> This patch implements buffered writes even if the device
> is not connected.
>
> While unit testing this, I discovered a serious defect in
> the way available space is computed that under-utilizes
> rfcomm i/o and may even halt further tx on that link, which
> is fixed by:
> Bluetooth: Refactor write_room() calculation
> Bluetooth: Fix write_room() calculation
>
>
> Note that this series does not fix the naively inefficient
> method of packetizing tty output; packetizing should be
> done on the krfcommd thread to take advantage of aggregating
> multiple tty writes into 1 or more packets. Look at any
> line-by-line console output to realize how under-utilized
> the rfcomm tty packeting is.
>
> [1] http://www.spinics.net/lists/linux-wireless/msg117818.html
> [2] http://www.spinics.net/lists/linux-bluetooth/msg42075.html
> [3] http://www.spinics.net/lists/linux-bluetooth/msg42057.html
>
>
> Regards,
>
>
> Peter Hurley (24):
> Revert "Bluetooth: Remove rfcomm_carrier_raised()"
> Revert "Bluetooth: Always wait for a connection on RFCOMM open()"
> Revert "Bluetooth: Move rfcomm_get_device() before
> rfcomm_dev_activate()”

these 3 patches are in bluetooth-next to give them extra testing. We will get them into 3.14-rc4 after an extra week of them being in linux-next.

> tty: Fix ref counting for port krefs

I am taking this one with Greg’s ack through bluetooth-next. Unless someone objects.

> Bluetooth: Fix racy acquire of rfcomm_dev reference
> Bluetooth: Exclude released devices from RFCOMMGETDEVLIST ioctl
> Bluetooth: Release rfcomm_dev only once
> Bluetooth: Fix unreleased rfcomm_dev reference
> Bluetooth: Fix RFCOMM tty teardown race
> Bluetooth: Verify dlci not in use before rfcomm_dev create
> Bluetooth: Simplify RFCOMM session state eval
> Bluetooth: Refactor deferred setup test in rfcomm_dlc_close()
> Bluetooth: Refactor dlc disconnect logic in rfcomm_dlc_close()
> Bluetooth: Directly close dlc for not yet started RFCOMM session
> Bluetooth: Fix unsafe RFCOMM device parenting
> Bluetooth: Fix RFCOMM parent device for reused dlc
> Bluetooth: Rename __rfcomm_dev_get() to __rfcomm_dev_lookup()
> Bluetooth: Serialize RFCOMMCREATEDEV and RFCOMMRELEASEDEV ioctls
> Bluetooth: Refactor rfcomm_dev_add()
> Bluetooth: Cleanup RFCOMM device registration error handling
> Bluetooth: Force -EIO from tty read/write if .activate() fails
> Bluetooth: Don't fail RFCOMM tty writes
> Bluetooth: Refactor write_room() calculation
> Bluetooth: Fix write_room() calculation
>
> include/linux/tty.h | 6 +-
> include/net/bluetooth/rfcomm.h | 9 +-
> net/bluetooth/rfcomm/core.c | 88 ++++++++++----
> net/bluetooth/rfcomm/tty.c | 262 ++++++++++++++++++++++-------------------
> 4 files changed, 223 insertions(+), 142 deletions(-)

all patches have been applied to bluetooth-next tree.

Regards

Marcel