Re: [PATCH v4 1/3] Bluetooth: Fix possible race with userspace of sysfs isoc_alt

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Feb 13, 2025 at 11:43:59AM +0800, Hsin-chen Chuang wrote:
> From: Hsin-chen Chuang <chharry@chromium.org>
> 
> Expose the isoc_alt attr with device group to avoid the racing.
> 
> Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> it also becomes the parent device of hci dev.
> 
> Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> ---
> 
> Changes in v4:
> - Create a dev node for btusb. It's now hci dev's parent and the
>   isoc_alt now belongs to it.
> - Since the changes is almost limitted in btusb, no need to add the
>   callbacks in hdev anymore.
> 
> Changes in v3:
> - Make the attribute exported only when the isoc_alt is available.
> - In btusb_probe, determine data->isoc before calling hci_alloc_dev_priv
>   (which calls hci_init_sysfs).
> - Since hci_init_sysfs is called before btusb could modify the hdev,
>   add new argument add_isoc_alt_attr for btusb to inform hci_init_sysfs.
> 
>  drivers/bluetooth/btusb.c        | 98 +++++++++++++++++++++++++-------
>  include/net/bluetooth/hci_core.h |  1 +
>  net/bluetooth/hci_sysfs.c        |  3 +-
>  3 files changed, 79 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1caf7a071a73..cb3db18bb72c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -920,6 +920,8 @@ struct btusb_data {
>  	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
>  
>  	struct qca_dump_info qca_dump;
> +
> +	struct device dev;
>  };
>  
>  static void btusb_reset(struct hci_dev *hdev)
> @@ -3693,6 +3695,9 @@ static ssize_t isoc_alt_store(struct device *dev,
>  	int alt;
>  	int ret;
>  
> +	if (!data->hdev)
> +		return -ENODEV;
> +
>  	if (kstrtoint(buf, 10, &alt))
>  		return -EINVAL;
>  
> @@ -3702,6 +3707,34 @@ static ssize_t isoc_alt_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(isoc_alt);
>  
> +static struct attribute *btusb_sysfs_attrs[] = {
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(btusb_sysfs);
> +
> +static void btusb_sysfs_release(struct device *dev)
> +{
> +	// Resource release is managed in btusb_disconnect

That is NOT how the driver model works, do NOT provide an empty
release() function just to silence the driver core from complaining
about it.

If for some reason the code thinks it is handling devices differently
than it should be, fix that instead.

thanks,

greg k-h