Hello Greg, Thanks for your feedback. On Thu, Jul 31, 2025 at 06:32:35AM +0200, Greg KH wrote: > On Thu, Jul 31, 2025 at 03:19:19AM +0100, Salah Triki wrote: > > The driver stores a reference to the `usb_device` structure (`udev`) > > in its private data (`data->udev`), which can persist beyond the > > immediate context of the `bfusb_probe()` function. > > > > Without proper reference count management, this can lead to two issues: > > > > 1. A `use-after-free` scenario if `udev` is accessed after its main > > reference count drops to zero (e.g., if the device is disconnected > > and the `data` structure is still active). > > How can that happen as during the probe/remove cycle, the reference > count is always properly incremetned. > > > 2. A `memory leak` if `udev`'s reference count is not properly > > decremented during driver disconnect, preventing the `usb_device` > > object from being freed. > > There is no leak here at all, sorry. > I understand your concern about the existence of a memory leak or use-after-free scenario in the driver's current context. My intention with this patch is to ensure the driver adheres to best practices for managing `usb_device` structure references, as outlined in the kernel's documentation. The `usb_get_dev()` function is explicitly designed for use when a driver stores a reference to a `usb_device` structure in its private data, which is the case here with `data->udev`. As the documentation for `usb_get_dev()` states: ``Each live reference to a device should be refcounted. Drivers for USB interfaces should normally record such references in their probe() methods, when they bind to an interface, and release them by calling usb_put_dev(), in their disconnect() methods.`` By following this recommendation, adding `usb_get_dev(udev)` in `bfusb_probe()` and `usb_put_dev(data->udev)` in `bfusb_disconnect()` ensures the `udev` structure's lifetime is explicitly managed by the driver as long as it's being referenced. This proactively prevents potential issues that could arise in future scenarios, even if a specific problem hasn't been observed or reported yet. > > To correctly manage the `udev` lifetime, explicitly increment its > > reference count with `usb_get_dev(udev)` when storing it in the > > driver's private data. Correspondingly, decrement the reference count > > with `usb_put_dev(data->udev)` in the `bfusb_disconnect()` callback. > > > > This ensures `udev` remains valid while referenced by the driver's > > private data and is properly released when no longer needed. > > How was this tested? > > I'm not saying the change is wrong, just that I don't think it's > actually a leak, or fix of anything real. > > Or do you have a workload that shows this is needed? If so, what is the > crash reported? > While I don't have a specific workload that reproduces a current crash or memory leak, this patch aims to enhance the driver's robustness by aligning its behavior with the established conventions for managing `usb_device` object references. It's a preventive measure to ensure the driver correctly handles the lifetime of the `usb_device` object it references, even in scenarios of unexpected disconnection or re-enumeration that might otherwise have unforeseen consequences. Please let me know if you have any further questions. Regards, Salah Triki
