On Thu, Apr 28, 2022 at 03:15:02PM +0800, Lin Ma wrote:
> Hello Jakub,
>
> and hello there maintainers, when we tried to fix this race problem, we found another very weird issue as below
>
> >
> > You can't use a single global variable, there can be many devices
> > each with their own lock.
> >
> > Paolo suggested adding a lock, if spin lock doesn't fit the bill
> > why not add a mutex?
>
> The lock patch can be added to nfcmrvl code but we prefer to fix this in the NFC core layer hence every other driver that supports the firmware downloading task will be free of such a problem.
>
> But when we analyze the race between the netlink tasks and the cleanup routine, we find some *condition checks* fail to fulfill their responsibility.
>
> For example, we once though that the device_lock + device_is_registered check can help to fix the race as below.
>
> netlink task | cleanup routine
> |
> nfc_genl_fw_download | nfc_unregister_device
> nfc_fw_download | device_del
> device_lock | device_lock
> // wait lock | kobject_del
> // ... | ...
> device_is_registered | device_unlock
> rc = -ENODEV |
>
> However, by dynamic debugging this issue, we find out that **even after the device_del**, the device_is_registered check still returns TRUE!
You should not be making these types of checks outside of the driver
core.
> This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.
Please do not do that.
>
> -> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")
>
> To find out why, we find out the device_is_registered is implemented like below:
>
> static inline int device_is_registered(struct device *dev)
> {
> return dev->kobj.state_in_sysfs;
> }
>
> By debugging, we find out in normal case, this kobj.state_in_sysfs will be clear out like below
>
> [#0] 0xffffffff81f0743a → __kobject_del(kobj=0xffff888009ca7018)
> [#1] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
> [#2] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
> [#3] 0xffffffff827708db → device_del(dev=0xffff888009ca7018)
> [#4] 0xffffffff8396496f → nfc_unregister_device(dev=0xffff888009ca7000)
> [#5] 0xffffffff839850a9 → nci_unregister_device(ndev=0xffff888009ca3000)
> [#6] 0xffffffff82811308 → nfcmrvl_nci_unregister_dev(priv=0xffff88800c805c00)
> [#7] 0xffffffff83990c4f → nci_uart_tty_close(tty=0xffff88800b450000)
> [#8] 0xffffffff820f6bd3 → tty_ldisc_kill(tty=0xffff88800b450000)
> [#9] 0xffffffff820f7fb1 → tty_ldisc_hangup(tty=0xffff88800b450000, reinit=0x0)
>
> The clear out is in function __kobject_del
>
> static void __kobject_del(struct kobject *kobj)
> {
> // ...
>
> kobj->state_in_sysfs = 0;
> kobj_kset_leave(kobj);
> kobj->parent = NULL;
> }
>
> The structure of device_del is like below
>
> void device_del(struct device *dev)
> {
> struct device *parent = dev->parent;
> struct kobject *glue_dir = NULL;
> struct class_interface *class_intf;
> unsigned int noio_flag;
>
> device_lock(dev);
> kill_device(dev);
> device_unlock(dev);
>
> // ...
> kobject_del(&dev->kobj);
> cleanup_glue_dir(dev, glue_dir);
> memalloc_noio_restore(noio_flag);
> put_device(parent);
> }
>
> In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.
Nor should it be.
> This means the device_lock + device_is_registered is still prone to the data race. And this is not just the problem with firmware downloading. The all relevant netlink tasks that use the device_lock + device_is_registered is possible to be raced.
>
> To this end, we will come out with two patches, one for fixing this device_is_registered by using another status variable instead. The other is the patch that reorders the code in nci_unregister_device.
Why is this somehow unique to these devices? Why do no other buses have
this issue? Are you somehow allowing a code path that should not be
happening?
thanks,
greg k-h
Hello Greg,
>
> You should not be making these types of checks outside of the driver
> core.
>
> > This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.
>
> Please do not do that.
>
> >
> > -> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")
> >
> <...>
> >
> > In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.
>
> Nor should it be.
>
I may have mistakenly presented my point. In fact, there is nothing wrong with the device core, nothing to do with the internal of device_del and device_is_registered implementation. And, of course, we will not add any code or do any modification to the device/driver base code.
The point is the combination of device_is_registered + device_del, which is used in NFC core, is not safe.
That is to say, even the device_is_registered can return True even the device_del is executing in another thread.
(By debugging we think this is true, correct me if it is not)
Hence we want to add additional state in nfc_dev object to fix that, not going to add any state in device/driver core.
> > This means the device_lock + device_is_registered is still prone to the data race. And this is not just the problem with firmware downloading. The all relevant netlink tasks that use the device_lock + device_is_registered is possible to be raced.
> >
> > To this end, we will come out with two patches, one for fixing this device_is_registered by using another status variable instead. The other is the patch that reorders the code in nci_unregister_device.
>
> Why is this somehow unique to these devices? Why do no other buses have
> this issue? Are you somehow allowing a code path that should not be
> happening?
>
> thanks,
>
> greg k-h
In fact, by searching the device_is_registered() use cases, I found that most of them are used in drier code instead of in the network stack. I have no idea whether or not they suffer from similar problems and I will check that out.
Thanks
Lin