[v2,-next] usb/usbip: fix wrong data added to platform device

Commit Message

Hongren Zheng Oct. 14, 2023, 7:46 a.m. UTC

.data of platform_device_info will be copied into .platform_data of
struct device via platform_device_add_data.

However, vhcis[i] contains a spinlock, is dynamically allocated and
used by other code, so it is not meant to be copied. The workaround
was to use void *vhci as an agent, but it was removed in the commit
suggested below.

This patch adds back the workaround and changes the way of using
platform_data accordingly.

Reported-by: [e-mail]
Closes: https://lore.kernel.org/r/[e-mail]/
Reported-by: [e-mail]
Closes: https://lore.kernel.org/r/[e-mail]/
Fixes: b8aaf639b403 ("usbip: Use platform_device_register_full()")
Tested-by: [e-mail]
Link: https://lore.kernel.org/r/[e-mail]/
Signed-off-by: Hongren Zheng <[e-mail]>
---
 drivers/usb/usbip/vhci_hcd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

v2:
code style change suggested by Andy Shevchenko

Comments

Andy Shevchenko Oct. 16, 2023, 7:40 a.m. UTC | #1

On Sat, Oct 14, 2023 at 03:46:04PM +0800, Hongren Zheng wrote:
> .data of platform_device_info will be copied into .platform_data of
> struct device via platform_device_add_data.

platform_device_add_data()

> However, vhcis[i] contains a spinlock, is dynamically allocated and
> used by other code, so it is not meant to be copied. The workaround
> was to use void *vhci as an agent, but it was removed in the commit
> suggested below.
> 
> This patch adds back the workaround and changes the way of using
> platform_data accordingly.

Good learning to me, thank you for the fix!
Reviewed-by: Andy Shevchenko <[e-mail]>

Shuah Khan Oct. 16, 2023, 5:19 p.m. UTC | #2

On 10/16/23 01:40, Andy Shevchenko wrote:
> On Sat, Oct 14, 2023 at 03:46:04PM +0800, Hongren Zheng wrote:
>> .data of platform_device_info will be copied into .platform_data of
>> struct device via platform_device_add_data.
> 
> platform_device_add_data()
> 
>> However, vhcis[i] contains a spinlock, is dynamically allocated and
>> used by other code, so it is not meant to be copied. The workaround
>> was to use void *vhci as an agent, but it was removed in the commit
>> suggested below.
>>
>> This patch adds back the workaround and changes the way of using
>> platform_data accordingly.
> 
> Good learning to me, thank you for the fix!
> Reviewed-by: Andy Shevchenko <[e-mail]>
> 

Thank you both.

Acked-by: Shuah Khan <[e-mail]>

thanks,
-- Shuah

Greg Kroah-Hartman Oct. 16, 2023, 5:42 p.m. UTC | #3

On Sat, Oct 14, 2023 at 03:46:04PM +0800, Hongren Zheng wrote:
> .data of platform_device_info will be copied into .platform_data of
> struct device via platform_device_add_data.

That is crazy.

> However, vhcis[i] contains a spinlock, is dynamically allocated and
> used by other code, so it is not meant to be copied. The workaround
> was to use void *vhci as an agent, but it was removed in the commit
> suggested below.

Again, crazy, I'm amazed it works at all.

> This patch adds back the workaround and changes the way of using
> platform_data accordingly.
> 
> Reported-by: [e-mail]
> Closes: https://lore.kernel.org/r/[e-mail]/
> Reported-by: [e-mail]
> Closes: https://lore.kernel.org/r/[e-mail]/
> Fixes: b8aaf639b403 ("usbip: Use platform_device_register_full()")
> Tested-by: [e-mail]
> Link: https://lore.kernel.org/r/[e-mail]/
> Signed-off-by: Hongren Zheng <[e-mail]>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

I'll take this now, as it fixes a reported issue, but really, the root
problem should be fixed instead.  This is not how to treat platform data
at all.

thanks,

greg k-h

Patch

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index f845b91848b9..82650c11e451 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1139,7 +1139,7 @@  static int hcd_name_to_id(const char *name)
 
 static int vhci_setup(struct usb_hcd *hcd)
 {
-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 
 	if (usb_hcd_is_primary_hcd(hcd)) {
 		vhci->vhci_hcd_hs = hcd_to_vhci_hcd(hcd);
@@ -1257,7 +1257,7 @@  static int vhci_get_frame_number(struct usb_hcd *hcd)
 /* FIXME: suspend/resume */
 static int vhci_bus_suspend(struct usb_hcd *hcd)
 {
-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 	unsigned long flags;
 
 	dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__);
@@ -1271,7 +1271,7 @@  static int vhci_bus_suspend(struct usb_hcd *hcd)
 
 static int vhci_bus_resume(struct usb_hcd *hcd)
 {
-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 	int rc = 0;
 	unsigned long flags;
 
@@ -1338,7 +1338,7 @@  static const struct hc_driver vhci_hc_driver = {
 
 static int vhci_hcd_probe(struct platform_device *pdev)
 {
-	struct vhci             *vhci = dev_get_platdata(&pdev->dev);
+	struct vhci             *vhci = *((void **)dev_get_platdata(&pdev->dev));
 	struct usb_hcd		*hcd_hs;
 	struct usb_hcd		*hcd_ss;
 	int			ret;
@@ -1396,7 +1396,7 @@  static int vhci_hcd_probe(struct platform_device *pdev)
 
 static void vhci_hcd_remove(struct platform_device *pdev)
 {
-	struct vhci *vhci = dev_get_platdata(&pdev->dev);
+	struct vhci *vhci = *((void **)dev_get_platdata(&pdev->dev));
 
 	/*
 	 * Disconnects the root hub,
@@ -1431,7 +1431,7 @@  static int vhci_hcd_suspend(struct platform_device *pdev, pm_message_t state)
 	if (!hcd)
 		return 0;
 
-	vhci = dev_get_platdata(hcd->self.controller);
+	vhci = *((void **)dev_get_platdata(hcd->self.controller));
 
 	spin_lock_irqsave(&vhci->lock, flags);
 
@@ -1522,10 +1522,11 @@  static int __init vhci_hcd_init(void)
 		goto err_driver_register;
 
 	for (i = 0; i < vhci_num_controllers; i++) {
+		void *vhci = &vhcis[i];
 		struct platform_device_info pdevinfo = {
 			.name = driver_name,
 			.id = i,
-			.data = &vhcis[i],
+			.data = &vhci,
 			.size_data = sizeof(void *),
 		};