linux-kernel - Re: [BUG] 2.6.30-rc4 hid bluetooth not working

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1241293366.4412.14.camel@localhost.localdomain>
Date:	Sat, 02 May 2009 12:42:46 -0700
From:	Marcel Holtmann <marcel@...tmann.org>
To:	Dave Young <hidave.darkstar@...il.com>
Cc:	Marc Pignat <Marc.Pignat@...s.ch>, jkosina@...e.cz,
	ext-roger.quadros@...ia.com, linux-bluetooth@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [BUG] 2.6.30-rc4 hid bluetooth not working

Hi Dave,

> >>> > Subject: bluetooth: Fix serialization when adding/deleting connections in hci_sysfs
> >>> >
> >>> > add_conn and del_conn should be serialized, but flush_workqueue can't be used
> >>> > by the worker thread on it's own queue, so use flush_work to serialize add_conn
> >>> > and del_conn against each other.
> >>> >
> >>> > Signed-off-by: Marc Pignat <marc.pignat@...s.ch>
> >>>
> >>> Acked-by: Jiri Kosina <jkosina@...e.cz>
> >>>
> >>> FWIW.
> >>
> >>nak from my side since I think it is the wrong fix. We really wanna wait
> >>for all works to finish here. This includes work from other connection
> >>attempts or terminations.
> >
> > IMHO, there is no need to wait for work currently running, since this is a
> > singlethread workqueue.
> 
> Yes, sounds right.
> 
> >
> > But it is perhaps simpler to use a lock (mutex or watherver locking primitive).
> 
> I'm here a little bit late. Marcel, I'm quite busy recently, I just
> see the commit and then this thread.
> 
> Let me explain why I add two workqueue originally,  because workqueue
> will be defered, so   we must guarantee "connection deletion" finished
> before "connection adding with same bt addr",  or the "connection
> adding" will fail.
> 
> On the other hand flush "adding" workqueue in "connection deletion"
> function is not necessary.
> 
> To fix this bug, I think we can just use the two work struct for
> add/del, at the same time keeping the original two workqueue.
> 
> Please see following patch for this, (building-test only, I have no
> bluetooth device at hand, I can test this the day after tommorrow)

so I spent the whole day figuring out what is going on here and we keep
making the wrong assumptions over and over again.

First of all, we only add the sysfs device when we have a successful
connection. And we identify it with the handle. This means that we can
NOT have any name clashes anymore since the controller has to make sure
a handle is only assigned once. Previously we did this on the BD_ADDR
value and that lead to it. That is no longer the case.

Second of all the two work queues introduces way too much complexity for
a really simple task of adding and removing a sysfs device entry.

The real problem we have right now are that we are not initializing the
sysfs device when creating the hci_conn. This is just wrong and can lead
to all kinds of weird invalid data access. And as a result the adding of
the sysfs device should only set the name and add it.

We also check device_registered before making sure that device_add has
been run. And instead of adding more locking or crazy work queue
dependencies, we should use the single thread work queue to ensure the
correct order of things.

The attached patch introduces a hci_conn_init_sysfs step to make sure we
setup the sysfs device correctly. I left the flush_work calls, but I
think they are not needed since a del_conn before add_conn is no longer
possible now.

Regards

Marcel

View attachment "patch-bluetooth-fix-sysfs-workq" of type "text/x-patch" (4403 bytes)