[PATCH v2] usbip: vhci_hcd: let the driver core manage the sysfs attributes

[PATCH v2] usbip: vhci_hcd: let the driver core manage the sysfs attributes

[Deepanshu Kartikey]

The vhci attribute group is created in vhci_start() and removed in
vhci_stop(), guarded by usb_hcd_is_primary_hcd(). Both run from
usb_add_hcd()/usb_remove_hcd(), which are called once per hcd, so the
attach attribute is live while only one of the two hcds exists: it is
created during the first usb_add_hcd() before vhci_hcd_ss is set, and it
survives the first usb_put_hcd() on removal. A concurrent write to attach
can therefore reach a NULL or freed vhci_hcd_ss.

Register the group as dev_groups on the platform driver instead, so the
driver core creates the files before probe and removes them after remove
returns, and drop the sysfs_create_group()/sysfs_remove_group() calls from
the driver. The attributes have always been created only on vhci_hcd.0;
an is_visible() callback keeps them there.

vhci_init_attr_group() is moved into vhci_hcd_init() ahead of
platform_driver_register() so the group is populated before the core
reads it. The attribute array is still built at runtime because the
number of status attributes comes from CONFIG_USBIP_VHCI_NR_HCS and the
preprocessor cannot emit a variable number of __ATTR() declarations. If
statically declaring all 32 and hiding the unused ones with is_visible()
is preferred, I can respin that way.

Reported-by: syzbot+8753715f05759f1a10de@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8753715f05759f1a10de
Fixes: 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed support")
Link: https://lore.kernel.org/all/20260815143241.19047-1-kartikey406@gmail.com/T/ [v1]
Tested-by: syzbot+8753715f05759f1a10de@syzkaller.appspotmail.com
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
v2:
 - use dev_groups instead of moving sysfs_create_group() into probe (Greg)
 - add is_visible() to keep the attributes on vhci_hcd.0 only
 - move vhci_init_attr_group() into vhci_hcd_init()
---
 drivers/usb/usbip/vhci.h       |  1 +
 drivers/usb/usbip/vhci_hcd.c   | 37 ++++++++++------------------------
 drivers/usb/usbip/vhci_sysfs.c | 19 +++++++++++++++++
 3 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h
index 5659dce1526e..844500e3b0ea 100644
--- a/drivers/usb/usbip/vhci.h
+++ b/drivers/usb/usbip/vhci.h
@@ -121,6 +121,7 @@ struct vhci_hcd {
 extern int vhci_num_controllers;
 extern struct vhci *vhcis;
 extern struct attribute_group vhci_attr_group;
+extern const struct attribute_group *vhci_groups[];
 
 /* vhci_hcd.c */
 void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed);
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 39e8faf4c18c..4bace5b8b879 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1199,7 +1199,6 @@ static int vhci_start(struct usb_hcd *hcd)
 {
 	struct vhci_hcd *vhci_hcd = hcd_to_vhci_hcd(hcd);
 	int id, rhport;
-	int err;
 
 	usbip_dbg_vhci_hc("enter vhci_start\n");
 
@@ -1230,40 +1229,17 @@ static int vhci_start(struct usb_hcd *hcd)
 		return -EINVAL;
 	}
 
-	/* vhci_hcd is now ready to be controlled through sysfs */
-	if (id == 0 && usb_hcd_is_primary_hcd(hcd)) {
-		err = vhci_init_attr_group();
-		if (err) {
-			dev_err(hcd_dev(hcd), "init attr group failed, err = %d\n", err);
-			return err;
-		}
-		err = sysfs_create_group(&hcd_dev(hcd)->kobj, &vhci_attr_group);
-		if (err) {
-			dev_err(hcd_dev(hcd), "create sysfs files failed, err = %d\n", err);
-			vhci_finish_attr_group();
-			return err;
-		}
-		dev_info(hcd_dev(hcd), "created sysfs %s\n", hcd_name(hcd));
-	}
-
 	return 0;
 }
 
 static void vhci_stop(struct usb_hcd *hcd)
 {
 	struct vhci_hcd *vhci_hcd = hcd_to_vhci_hcd(hcd);
-	int id, rhport;
+	int rhport;
 
 	usbip_dbg_vhci_hc("stop VHCI controller\n");
 
-	/* 1. remove the userland interface of vhci_hcd */
-	id = hcd_name_to_id(hcd_name(hcd));
-	if (id == 0 && usb_hcd_is_primary_hcd(hcd)) {
-		sysfs_remove_group(&hcd_dev(hcd)->kobj, &vhci_attr_group);
-		vhci_finish_attr_group();
-	}
-
-	/* 2. shutdown all the ports of vhci_hcd */
+	/* shutdown all the ports of vhci_hcd */
 	for (rhport = 0; rhport < VHCI_HC_PORTS; rhport++) {
 		struct vhci_device *vdev = &vhci_hcd->vdev[rhport];
 
@@ -1513,6 +1489,7 @@ static struct platform_driver vhci_driver = {
 	.resume	= vhci_hcd_resume,
 	.driver	= {
 		.name = driver_name,
+		.dev_groups = vhci_groups,
 	},
 };
 
@@ -1541,6 +1518,10 @@ static int __init vhci_hcd_init(void)
 	if (vhcis == NULL)
 		return -ENOMEM;
 
+	ret = vhci_init_attr_group();
+	if (ret)
+		goto err_init_attr_group;
+
 	ret = platform_driver_register(&vhci_driver);
 	if (ret)
 		goto err_driver_register;
@@ -1565,9 +1546,12 @@ static int __init vhci_hcd_init(void)
 
 	return 0;
 
+
 err_add_hcd:
 	platform_driver_unregister(&vhci_driver);
 err_driver_register:
+	vhci_finish_attr_group();
+err_init_attr_group:
 	kfree(vhcis);
 	return ret;
 }
@@ -1576,6 +1560,7 @@ static void __exit vhci_hcd_exit(void)
 {
 	del_platform_devices();
 	platform_driver_unregister(&vhci_driver);
+	vhci_finish_attr_group();
 	kfree(vhcis);
 }
 
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index a7ede6fb3da9..8c0bff2ddabb 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -497,8 +497,27 @@ static void finish_status_attrs(void)
 	kfree(status_attrs);
 }
 
+static umode_t vhci_attr_is_visible(struct kobject *kobj,
+				    struct attribute *attr, int n)
+{
+	struct platform_device *pdev = to_platform_device(kobj_to_dev(kobj));
+	/*
+	 * The attributes control every controller and have always lived on
+	 * vhci_hcd.0 only.  Keep them there now that the driver core creates
+	 * the group for each device.
+	 */
+	return pdev->id == 0 ? attr->mode : 0;
+}
+
+
 struct attribute_group vhci_attr_group = {
 	.attrs = NULL,
+	.is_visible = vhci_attr_is_visible,
+};
+
+const struct attribute_group *vhci_groups[] = {
+	&vhci_attr_group,
+	NULL,
 };
 
 int vhci_init_attr_group(void)
-- 
2.43.0

Re: [PATCH v2] usbip: vhci_hcd: let the driver core manage the sysfs attributes

[Greg KH]

On Sun, Aug 16, 2026 at 07:20:51AM +0530, Deepanshu Kartikey wrote:
> The vhci attribute group is created in vhci_start() and removed in
> vhci_stop(), guarded by usb_hcd_is_primary_hcd(). Both run from
> usb_add_hcd()/usb_remove_hcd(), which are called once per hcd, so the
> attach attribute is live while only one of the two hcds exists: it is
> created during the first usb_add_hcd() before vhci_hcd_ss is set, and it
> survives the first usb_put_hcd() on removal. A concurrent write to attach
> can therefore reach a NULL or freed vhci_hcd_ss.
> 
> Register the group as dev_groups on the platform driver instead, so the
> driver core creates the files before probe and removes them after remove
> returns, and drop the sysfs_create_group()/sysfs_remove_group() calls from
> the driver. The attributes have always been created only on vhci_hcd.0;
> an is_visible() callback keeps them there.
> 
> vhci_init_attr_group() is moved into vhci_hcd_init() ahead of
> platform_driver_register() so the group is populated before the core
> reads it. The attribute array is still built at runtime because the
> number of status attributes comes from CONFIG_USBIP_VHCI_NR_HCS and the
> preprocessor cannot emit a variable number of __ATTR() declarations. If
> statically declaring all 32 and hiding the unused ones with is_visible()
> is preferred, I can respin that way.
> 
> Reported-by: syzbot+8753715f05759f1a10de@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=8753715f05759f1a10de
> Fixes: 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed support")
> Link: https://lore.kernel.org/all/20260815143241.19047-1-kartikey406@gmail.com/T/ [v1]
> Tested-by: syzbot+8753715f05759f1a10de@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
> v2:
>  - use dev_groups instead of moving sysfs_create_group() into probe (Greg)
>  - add is_visible() to keep the attributes on vhci_hcd.0 only
>  - move vhci_init_attr_group() into vhci_hcd_init()
> ---
>  drivers/usb/usbip/vhci.h       |  1 +
>  drivers/usb/usbip/vhci_hcd.c   | 37 ++++++++++------------------------
>  drivers/usb/usbip/vhci_sysfs.c | 19 +++++++++++++++++
>  3 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h
> index 5659dce1526e..844500e3b0ea 100644
> --- a/drivers/usb/usbip/vhci.h
> +++ b/drivers/usb/usbip/vhci.h
> @@ -121,6 +121,7 @@ struct vhci_hcd {
>  extern int vhci_num_controllers;
>  extern struct vhci *vhcis;
>  extern struct attribute_group vhci_attr_group;

Is this still needed here?  And can't it be static now?

> @@ -1565,9 +1546,12 @@ static int __init vhci_hcd_init(void)
>  
>  	return 0;
>  
> +

Extra line?

thanks,

greg k-h

Re: [PATCH v2] usbip: vhci_hcd: let the driver core manage the sysfs attributes

[Deepanshu Kartikey]

On Sun, Aug 16, 2026 at 10:26 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> >  extern struct vhci *vhcis;
> >  extern struct attribute_group vhci_attr_group;
>
> Is this still needed here?  And can't it be static now?
>
> > @@ -1565,9 +1546,12 @@ static int __init vhci_hcd_init(void)
> >
> >       return 0;
> >
> > +
>
> Extra line?
>
> thanks,
>
> greg k-h

I have sent patch v3

Thanks

Deepanshu

Re: [PATCH v2] usbip: vhci_hcd: let the driver core manage the sysfs attributes

[Greg KH]

On Sun, Aug 16, 2026 at 11:41:41AM +0530, Deepanshu Kartikey wrote:
> On Sun, Aug 16, 2026 at 10:26 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> 
> > >  extern struct vhci *vhcis;
> > >  extern struct attribute_group vhci_attr_group;
> >
> > Is this still needed here?  And can't it be static now?
> >
> > > @@ -1565,9 +1546,12 @@ static int __init vhci_hcd_init(void)
> > >
> > >       return 0;
> > >
> > > +
> >
> > Extra line?
> >
> > thanks,
> >
> > greg k-h
> 
> I have sent patch v3

Please slow down, there is no rush here, you shouldn't be sending
patches more than one every few days.

greg k-h