usb/usbip: fix wrong data added to platform device

PATCH Superseded Hongren Zheng 2 participants 4 messages Oct 13, 2023

Subsystems (2)

USB USB over IP driver

lore

Share & tools

lore

Hongren Zheng

author reviewer

2023-10-13 10:52 UTC #0 lore raw

PATCH 1/1 usb/usbip: fix wrong data added to platform device

.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 [email protected]

Closes https://lore.kernel.org/linux-usb/[email protected]/ open here

Reported-by [email protected]

Closes https://lore.kernel.org/linux-usb/[email protected]/ open here

Fixes b8aaf639b403 ("usbip: Use platform_device_register_full()")

Signed-off-by Hongren Zheng <[email protected]>

---
 drivers/usb/usbip/vhci_hcd.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/usb/usbip/vhci_hcd.c +9 -8

index f845b91848b9..dfbdc77108e5 100644
@@ -1139,8 +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);
 		vhci->vhci_hcd_hs->vhci = vhci;

@@ -1257,7 +1256,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 +1270,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 +1337,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 +1395,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 +1430,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);

@@ -1506,6 +1505,7 @@ static void del_platform_devices(void)
 static int __init vhci_hcd_init(void)
 {
 	int i, ret;
+	void *vhci;
 
 	if (usb_disabled())
 		return -ENODEV;

@@ -1522,10 +1522,11 @@ static int __init vhci_hcd_init(void)
 		goto err_driver_register;
 
 	for (i = 0; i < vhci_num_controllers; i++) {
+		vhci = &vhcis[i];
 		struct platform_device_info pdevinfo = {
 			.name = driver_name,
 			.id = i,
-			.data = &vhcis[i],
+			.data = &vhci,
 			.size_data = sizeof(void *),
 		};

Signature · 1 line

2.37.2

Hongren Zheng

author reviewer Tested-by [email protected] on v1 1/1

2023-10-13 12:06 UTC #1 lore raw

Quoted 11 lines · Hongren Zheng

.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.

Quoted 7 lines

Reported-by [email protected]
Closes https://lore.kernel.org/linux-usb/[email protected]/ open here
Reported-by [email protected]
Closes https://lore.kernel.org/linux-usb/[email protected]/ open here
Fixes b8aaf639b403 ("usbip: Use platform_device_register_full()")
Signed-off-by Hongren Zheng <[email protected]>
---

Tested-by [email protected]

Link https://lore.kernel.org/r/[email protected]/ open here

Andy Shevchenko

2023-10-13 19:55 UTC #2 lore raw

In reply to Hongren Zheng

Quoted 10 lines · Hongren Zheng

.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.

Thanks for fixing this.

...

drivers/usb/usbip/vhci_hcd.c static int hcd_name_to_id(const char *name) · line 1143

Quoted 3 lines

drivers/usb/usbip/vhci_hcd.c +0 -2

-	struct vhci *vhci = dev_get_platdata(hcd->self.controller);
 
-

The blank line here is on purpose.

Quoted 1 line

(quoted diff) +1 -0

+	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2023-10-13 19:58 UTC #3 lore raw

In reply to Hongren Zheng

Quoted 10 lines · Hongren Zheng

.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.

...

One more thing...

drivers/usb/usbip/vhci_hcd.c static int __init vhci_hcd_init(void) · line 1526

Quoted 13 lines

static int __init vhci_hcd_init(void)
{

drivers/usb/usbip/vhci_hcd.c +2 -0

 	int i, ret;
+	void *vhci;
 
 	if (usb_disabled())
 		return -ENODEV;

@@ -1522,10 +1522,11 @@ static int __init vhci_hcd_init(void)
 		goto err_driver_register;
 
 	for (i = 0; i < vhci_num_controllers; i++) {
 
+		vhci = &vhcis[i];

This should be

		void *vhci = &vhcis[i];

because otherwise we mix code and definitions which is not so good style. (Yet we allow to do that in exceptional cases: 1) iterators in for-loops, and 2) RAII allocations with __free() in use.)

Quoted 7 lines

drivers/usb/usbip/vhci_hcd.c +1 -1

 		struct platform_device_info pdevinfo = {
 			.name = driver_name,
 			.id = i,
-			.data = &vhcis[i],
+			.data = &vhci,
 			.size_data = sizeof(void *),
 		};

Signature · 2 lines

With Best Regards,
Andy Shevchenko