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
| ||
|
Message-ID: <f5969fbb-bdbb-6841-e1db-3c32a7a27061@huaweicloud.com> Date: Tue, 22 Nov 2022 09:10:12 +0800 From: Wang Yufen <wangyufen@...weicloud.com> To: Wei Yongjun <weiyongjun@...weicloud.com>, Oleh Kravchenko <oleg@....org.ua> Cc: linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org, Pavel Machek <pavel@....cz> Subject: Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering 在 2022/11/15 10:06, Wei Yongjun 写道: > Hi Oleh, > > On 2022/11/11 18:39, Oleh Kravchenko wrote: >> Hello Wang, >> >>> 11 лист. 2022 р. о 11:21 wangyufen <wangyufen@...wei.com> написав(ла): >>> >>> >>> 在 2022/11/9 18:43, Oleh Kravchenko 写道: >>>> >>>> >>>>> 9 лист. 2022 р. о 12:25 wangyufen <wangyufen@...wei.com> написав(ла): >>>>> >>>>> >>>>> 在 2022/11/9 17:39, Oleh Kravchenko 写道: >>>>> >>>>>>> -static void el15203000_remove(struct spi_device *spi) >>>>>>> >>>>>> Is remove() callback from struct spi_driver deprecated? >>>>>> >>>>> It is not that remove() callback is deprecated, >>>>> it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(), >>>>> remove() callback is unnecessary here. >>>>> >>>> When remove() is called, the memory allocated by devm_*() is valid. >>>> So what you try to fix here? >>> >>> Fix the &priv->lock used after destroy, for details, please see patch #0 >>> LKML: Wang Yufen: [PATCH 00/13] leds: Fix devm vs. non-devm ordering >> >> It doesn’t make any sense for me. >> You saying that remove() called before devm_* allocation >> if it true then set_brightness_delayed() will crash the system in anyway. >> >> LED device has a parent SPI device; LED device can’t exist without SPI device. >> >> So deallocation order should be next: >> 1. LED device devm_*() >> 2. SPI device remove() > > The allocation order is as follows: > > el15203000_probe() > mutex_init(&priv->lock); > el15203000_probe_dt(priv) > device_for_each_child_node(priv->dev, child) { > ... > led->ldev.brightness_set_blocking = el15203000_set_blocking; > ... > devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data); > dr = devres_alloc(devm_led_classdev_release, sizeof(*dr), GFP_KERNEL); > <-- dr->node.release = devm_led_classdev_release() > ... > devres_add(parent, dr); > <-- add dr->node to &priv->dev->devres_head > > And the full deallocation order should be this: > > 1. SPI device .remove callback > 2. LED device devm_*() > 3. SPI device deallocation > > spi_unregister_device() > device_del() > bus_remove_device() > device_release_driver_internal() > __device_release_driver() > ... > device_remove() > spi_remove() <-- call el15203000_remove() here, mutex_destroy(&priv->lock), lock destroy > ... > device_unbind_cleanup() > devres_release_all() > release_nodes() > <-- traverse spi->dev->devres_head list and call dr->node.release in sequence. > devm_led_classdev_release() > led_classdev_unregister() > <-- flush set_brightness_work here, before the work flush, set_brightness_work may be sched. > <-- that is el15203000_set_blocking()..-> mutex_lock(&led->priv->lock) is called, > <-- this leads to the priv->lock use after destroy. > put_device(&spi->dev) <-- spi device is deallocation in here > > Hi Oleh, Judging from the deallocation order above, there is a issue that the &priv->lock used after destroy, right? And thanks Wei for the detailed explanation. Thanks, Wang > Regards, > Wei Yongjun >
Powered by blists - more mailing lists