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

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: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=8753715f05759f1a10de
> Fixes: 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed support")
> Link: https://lore.kernel.org/all/[email protected]/T/ [v1]
> Tested-by: [email protected]
> Signed-off-by: Deepanshu Kartikey <[email protected]>
> ---
> 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