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: <CAMZ6RqKS0sUFZWQfmRU6q2ivWEUFD06uiQekDr=u94L3uij3yQ@mail.gmail.com> Date: Mon, 28 Nov 2022 14:32:23 +0900 From: Vincent MAILHOL <mailhol.vincent@...adoo.fr> To: Alan Stern <stern@...land.harvard.edu> Cc: Andrew Lunn <andrew@...n.ch>, linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>, linux-kernel@...r.kernel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, netdev@...r.kernel.org, linux-usb@...r.kernel.org, Saeed Mahameed <saeed@...nel.org>, Jiri Pirko <jiri@...dia.com>, Lukas Magel <lukas.magel@...teo.net> Subject: Re: [PATCH v4 2/6] can: etas_es58x: add devlink support On Mon. 28 Nov. 2022 at 10:34, Vincent MAILHOL <mailhol.vincent@...adoo.fr> wrote: > On Mon. 28 Nov. 2022 at 00:41, Alan Stern <stern@...land.harvard.edu> wrote: > > On Sun, Nov 27, 2022 at 02:10:32PM +0900, Vincent MAILHOL wrote: > > > > Should devlink_free() be after usb_set_inftdata()? > > > > > > A look at > > > $ git grep -W "usb_set_intfdata(.*NULL)" > > > > > > shows that the two patterns (freeing before or after > > > usb_set_intfdata()) coexist. > > > > > > You are raising an important question here. usb_set_intfdata() does > > > not have documentation that freeing before it is risky. And the > > > documentation of usb_driver::disconnect says that: > > > "@disconnect: Called when the interface is no longer accessible, > > > usually because its device has been (or is being) disconnected > > > or the driver module is being unloaded." > > > Ref: https://elixir.bootlin.com/linux/v6.1-rc6/source/include/linux/usb.h#L1130 > > > > > > So the interface no longer being accessible makes me assume that the > > > order does not matter. If it indeed matters, then this is a foot gun > > > and there is some clean-up work waiting for us on many drivers. > > > > > > @Greg, any thoughts on whether or not the order of usb_set_intfdata() > > > and resource freeing matters or not? > > > > In fact, drivers don't have to call usb_set_intfdata(NULL) at all; the > > USB core does it for them after the ->disconnect() callback returns. > > Interesting. This fact is widely unknown, cf: > $ git grep "usb_set_intfdata(.*NULL)" | wc -l > 215 > > I will do some clean-up later on, at least for the CAN USB drivers. > > > But if a driver does make the call, it should be careful to ensure that > > the call happens _after_ the driver is finished using the interface-data > > pointer. For example, after all outstanding URBs have completed, if the > > completion handlers will need to call usb_get_intfdata(). > > ACK. I understand that it should be called *after* the completion of > any ongoing task. > > My question was more on: > > devlink_free(priv_to_devlink(es58x_dev)); > usb_set_intfdata(intf, NULL); > > VS. > > usb_set_intfdata(intf, NULL); > devlink_free(priv_to_devlink(es58x_dev)); > > From your comments, I understand that both are fine. Do we agree that the usb-skeleton is doing it wrong? https://elixir.bootlin.com/linux/latest/source/drivers/usb/usb-skeleton.c#L567 usb_set_intfdata(interface, NULL) is called before deregistering the interface and terminating the outstanding URBs! > > Remember, the interface-data pointer is owned by the driver. Nothing > > else in the kernel uses it. So the driver merely has to be careful not > > to clear the pointer while it is still using it. > > Thanks for your comments! > > > Yours sincerely, > Vincent Mailhol
Powered by blists - more mailing lists