On Tue, Jun 08, 2021 at 04:18:00PM +0800, Hillf Danton wrote: > On Mon, 7 Jun 2021 12:31:39 +0200 Greg KH wrote: > >On Mon, Jun 07, 2021 at 06:02:01PM +0800, Hillf Danton wrote: > >> After taking another look at the added user track, I realised that it serves > >> no more than a one-off state word that prevents channel from being released. > >> Then the race behind the uaf can be fixed by adding a state on top of the > >> dryrun introduced even without tracking users. > >> > >> The state machine works as the following, > >> 1) it is initialised to be backoff that means channel cannot be released > >> at the moment. > >> 2) it is changed to be dryrun on releasing to cut the race that survived > >> backoff. > >> 3) it is finally set to zero for release after cutting the chance for race. > > > >Adding another state on top of this feels rough, does it really solve > >the race here? > > No, frankly, given the list_del_rcu() in hci_chan_del(). > > >Normally a reference count should be enough to properly > >tear things down when needed, rolling back from a "can I try this now" > >state still seems racy without the needed lock somewhere. > > The rollback is added only for making sure that the channel released in > l2cap_conn_del() would not be freed in the other pathes. That exclusiveness > adds more barriers than thought to fixing the rare race with kref and spinlock > in the usual and simple manner. > > If OTOH channel is created with the exclusiveness taken into account by adding > the exclusive create and delete methods for l2cap, then the race can be fixed > by checking the exclusive mark in addition to aquiring the hdev lock at release > time. One would think that the state machine for the channel would fix this whole mess, why do we need an "additional" state here in the first place? Would be nice if one of the bluetooth maintainers weighed in on this... thanks, greg k-h
