Re: [EXT] Re: [PATCH] usb: roles: try to get/put all relevant modules

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jan 12, 2024 at 09:28:04AM +0000, Xu Yang wrote:
> Hi Greg,
> 
> > 
> > On Fri, Jan 12, 2024 at 09:24:11AM +0100, Greg KH wrote:
> > > On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> > > > +void usb_role_switch_get_modules(struct device *dev)
> > > > +{
> > > > +   while (dev) {
> > > > +           if (dev->driver)
> > > > +                   WARN_ON(!try_module_get(dev->driver->owner));
> > >
> > > You just crashed all systems that have panic-on-warn enabled, which is
> > > by far (i.e. in the billions) the huge majority of Linux systems in the
> > > world.
> > >
> > > If this is something that can fail, then properly handle the issue,
> > > don't just give up and say "let's fill the kernel log with a mess and
> > > reboot the box!".
> > 
> > Ah, I see now you are just moving the code, but please, let's fix this
> > properly, don't persist in the wrong code here.
> 
> This is a true module dependency issue and it only occurs when
> load/unload modules. The dependency of usb controller glue driver,
> usb controller driver and the user driver (such as tcpm) of usb role
> switch is not correctly established.

Then the driver model is not being used properly, as no module
references should be needed here.

> This patch is used to adjust dependency of them, without it, two issues
> may happen:
> 1. "NULL pointer dereference" kernel dump will be shown

For when?

> 2.  The reference count of usb controller module may never decrease to 0

The reference shouldn't have been increased as you want to be able to
unload the driver if a device is still present in the system.

So I think there's a larger issue here, module references shouldn't be
incremented just if a driver binds to a device, right?  Only if other
code is using the module, which is different.

thanks,

greg k-h