Re: [PATCH] Bluetooth: fix oops in rfcomm tty code (v2)

["Vegard Nossum" <vegard.nossum@gmail.com>]

On Sat, Jul 19, 2008 at 11:38 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Vegard,
>
>> This is a resend of a patch I sent earlier. It fixes a reproducible
>> oops (see http://lkml.org/lkml/2008/7/13/89 for test case), and I'd
>> be very happy for some feedback from Bluetooth people. Can this be
>> included for testing somewhere? I don't have any bluetooth devices
>> myself, so all my testing is limited to creating/releasing devices
>> with ioctl (I'm not sure that's good enough).
>>
>> Dave: I have extended the rfcomm_dev_lock to include all the setup and
>> teardown of a single device. On second thought, it doesn't really make
>> sense to use a separate lock for that. May I have your opinion on this
>> second version? (I've fixed the comments/BUG_ON that you pointed out.)
>
> it seems it is the actually the first time, I see this one. Anyway, so
> holding that lock is a bad idea. Fixing this the right way might be
> tricky since the TTY layer is involved here and own the kobject. So I
> would say we have to make sure that the RFCOMM TTY will hangup when
> calling RELEASEDEV or otherwise fail.

On Mon, Jul 21, 2008 at 7:14 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> The patch titled
>>      Bluetooth: fix oops in rfcomm tty code
>> has been added to the -mm tree.  Its filename is
>>      bluetooth-fix-oops-in-rfcomm-tty-code-v2.patch
>>
> just a quick note that I am not okay with this patch. Holding the lock
> isn't the right solution since it would also block any other application
> from creating new devices. We can't do this.

Hi,

We are not holding the lock across any ioctl/syscall boundary, which
would be an error anyway.

The lock now additionally protects the calls to:

    tty_register_device
    tty_unregister_device
    device_create_file

I don't think these functions block or do anything with the device
apart from just registering/unregistering the files.

Can you please explain in more detail what is wrong with the patch?
Where are we preventing other applications from creating new devices?
My intention was to prevent other applications from creating the
_same_ devices while they are still in use, although attempting to
register a new device (with an already-in-use ID) should simply fail,
and not block.

Thank you,


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036



-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036