LinuxLists.cc - [PATCH -next] usb/usbip: fix wrong data added to platform device

2023-10-13 10:52:34

Subject: [PATCH -next] 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]/
Reported-by: [email protected]
Closes: https://lore.kernel.org/linux-usb/[email protected]/
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(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index f845b91848b9..dfbdc77108e5 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -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 *),
};

--
2.37.2

2023-10-13 12:06:28

by Hongren Zheng

[permalink] [raw]

Subject: Re: [PATCH -next] usb/usbip: fix wrong data added to platform device

On Fri, Oct 13, 2023 at 06:52:09PM +0800, Hongren Zheng wrote:
> .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]/
> Reported-by: [email protected]
> Closes: https://lore.kernel.org/linux-usb/[email protected]/
> 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]/

2023-10-13 19:55:36

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH -next] usb/usbip: fix wrong data added to platform device

2023-10-13 19:59:01

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH -next] usb/usbip: fix wrong data added to platform device

On Fri, Oct 13, 2023 at 06:52:09PM +0800, Hongren Zheng wrote:
> .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...

> 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];

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

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

--
With Best Regards,
Andy Shevchenko