On Mon. 5 Dec. 2022 at 17:39, Oliver Neukum <oneukum@xxxxxxxx> wrote: > On 03.12.22 14:31, Vincent Mailhol wrote: > > The core sets the usb_interface to NULL in [1]. Also setting it to > > NULL in usb_driver::disconnects() is at best useless, at worse risky. > > Hi, > > I am afraid there is a major issue with your series of patches. > The drivers you are removing this from often have a subsequent check > for the data they got from usb_get_intfdata() being NULL. ACK, but I do not see the connection. > That pattern is taken from drivers like btusb or CDC-ACM Where does CDC-ACM set *his* interface to NULL? Looking at: https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/class/cdc-acm.c#L1531 I can see that cdc-acm sets acm->control and acm->data to NULL in his disconnect(), but it doesn't set its own usb_interface to NULL. > which claim secondary interfaces disconnect() will be called a second time > for. Are you saying that the disconnect() of those CAN USB drivers is being called twice? I do not see this in the source code. The only caller of usb_driver::disconnect() I can see is: https://elixir.bootlin.com/linux/v6.0/source/drivers/usb/core/driver.c#L458 > In addition, a driver can use setting intfdata to NULL as a flag > for disconnect() having proceeded to a point where certain things > can no longer be safely done. Any reference that a driver can do that? This pattern seems racy. By the way, I did check all the drivers: * ems_usb: intf is only used in ems_usb_probe() and ems_usb_disconnect() functions. * esd_usb: intf is only used in the esd_usb_probe(), esd_usb_probe_one_net() (which is part of probing), esd_usb_disconnect() and a couple of sysfs functions (which only use intf to get a pointer to struct esd_usb). * gs_usb: intf is used several time but only to retrive struct usb_device. This seems useless, I will sent this patch to remove it: https://lore.kernel.org/linux-can/20221208081142.16936-3-mailhol.vincent@xxxxxxxxxx/ Aside of that, intf is only used in gs_usb_probe(), gs_make_candev() (which is part of probing) and gs_usb_disconnect() functions. * kvaser_usb: intf is only used in kvaser_usb_probe() and kvaser_usb_disconnect() functions. * mcba_usb: intf is only used in mcba_usb_probe() and mcba_usb_disconnect() functions. * ucan: intf is only used in ucan_probe() and ucan_disconnect(). struct ucan_priv also has a pointer to intf but it is never used. I sent this patch to remove it: https://lore.kernel.org/linux-can/20221208081142.16936-2-mailhol.vincent@xxxxxxxxxx/ * usb_8dev: intf is only used in usb_8dev_probe() and usb_8dev_disconnect(). With no significant use of intf outside of the probe() and disconnect(), there is definitely no such "use intf as a flag" in any of these drivers. > You need to check for that in every driver > you remove this code from and if you decide that it can safely be removed, What makes you assume that I didn't check this in the first place? Or do you see something I missed? > which is likely, then please also remove checks like this: > > struct ems_usb *dev = usb_get_intfdata(intf); > > usb_set_intfdata(intf, NULL); > > if (dev) { > unregister_netdev(dev->netdev); How is the if (dev) check related? There is no correlation between setting intf to NULL and dev not being NULL. I think dev is never NULL, but I did not assess that dev could not be NULL. > Either it can be called a second time, then you need to leave it > as is, Really?! The first thing disconnect() does is calling usb_get_intfdata(intf) which dereferences intf without checking if it is NULL, c.f.: https://elixir.bootlin.com/linux/v6.0/source/include/linux/usb.h#L265 Then it sets intf to NULL. The second time you call disconnect(), the usb_get_intfdata(intf) would be a NULL pointer dereference. > or the check for NULL is superfluous. But only removing setting > the pointer to NULL never makes sense. Yours sincerely, Vincent Mailhol
