On Thu, Jul 31, 2025 at 12:20:36PM +0100, Salah Triki wrote: > 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. Yes, I agree with the documentation, I wrote it :) But, I am saying, you are NOT actually fixing anything here. It's a "best practice" but due to the fact that the dev pointer is only being reference counted by your change across the probe/release function, it is a pointless change. It's also a "dangerous" change in that you are trying to say "this fixes a security issue!" when it does not do anything like that at all. > > > 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. Please test this to see if it actually makes any difference in the code before making claims that it fixes a real bug. thanks, greg k-h
