Messages in this thread | | | Date | Thu, 28 Apr 2022 21:22:11 +0800 (GMT+08:00) | From | "Lin Ma" <> | Subject | Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able |
| |
Hello there,
> > Yes, that looks better,
Cool, thanks again for giving comments. :)
> but what is the root problem here that you are > trying to solve? Why does NFC need this when no other subsystem does? >
Well, in fact, me and Duoming are keep finding concurrency bugs that happen between the device cleanup/detach routine and other undergoing routines.
That is to say, when a device, no matter real or virtual, is detached from the machine, the kernel awake cleanup routine to reclaim the resource. In current case, the cleanup routine will call nfc_unregister_device().
Other routines, mainly from user-space system calls, need to be careful of the cleanup event. In another word, the kernel need to synchronize these routines to avoid race bugs.
In our practice, we find that many subsystems are prone to this type of bug.
For example, in bluetooth we fix
BT subsystem * e2cb6b891ad2 ("bluetooth: eliminate the potential race condition when removing the HCI controller") * fa78d2d1d64f ("Bluetooth: fix data races in smp_unregister(), smp_del_chan()") ..
AX25 subsystem * 1ade48d0c27d ("ax25: NPD bug when detaching AX25 device") ..
we currently focus on the net relevant subsystems and we now is auditing the NFC code.
In another word, all subsystems need to take care of the synchronization issues. But seems that the solutions are varied between different subsystem.
Empirically speaking, most of them use specific flags + specific locks to prevent the race.
In such cases, if the cleanup routine first hold the lock, the other routines will wait on the locks. Since the cleanup routine write the specific flag, the other routine, after check the specific flag, will be aware of the cleanup stuff and just abort their tasks. If the other routines first hold the lock, the cleanup routine just wait them to finish.
NFC here is special because it uses device_is_registered. I thought the author may believe this macro is race free. However, it is not. So we need to replace this check to make sure the netlink functions will 100 percent be aware of the cleanup routine and abort the task if they grab the device_lock lately. Otherwise, the nelink routine will call sub-layer code and possilby dereference resources that already freed.
For example, one of my recent fix 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device") takes the suggestion from maintainer as he thought the device_is_registered is enough. And for now we find out this device_is_registered is not enough.
If you are wondering if other subsystems use the combination of device_is_registered as check to avoid the race, I have to say I don't know. If the answer is yes, that code is possibly also prone to this type of concurrent bug.
> thansk, > > greg k-h
Thanks again.
Lin Ma | |