Message-ID: <2025021425-surgical-wackiness-0940@gregkh>
Date: Fri, 14 Feb 2025 12:37:12 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Hsin-chen Chuang <chharry@...gle.com>
Cc: linux-bluetooth@...r.kernel.org, luiz.dentz@...il.com,
	chromeos-bluetooth-upstreaming@...omium.org,
	Hsin-chen Chuang <chharry@...omium.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	Marcel Holtmann <marcel@...tmann.org>,
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
	Ying Hsu <yinghsu@...omium.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v5] Bluetooth: Fix possible race with userspace of sysfs
 isoc_alt

On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> From: Hsin-chen Chuang <chharry@...omium.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")

Wait, step back, why is this commit needed if you can change the alt
setting already today through usbfs/libusb without needing to mess with
the bluetooth stack at all?

> Signed-off-by: Hsin-chen Chuang <chharry@...omium.org>
> ---
> 
> Changes in v5:
> - Merge the ABI doc into this patch
> - Manage the driver data with device
> 
> 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.
> 
> Changes in v2:
> - The patch has been removed from series
> 
>  .../ABI/stable/sysfs-class-bluetooth          |  13 ++
>  drivers/bluetooth/btusb.c                     | 111 ++++++++++++++----
>  include/net/bluetooth/hci_core.h              |   1 +
>  net/bluetooth/hci_sysfs.c                     |   3 +-
>  4 files changed, 102 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-class-bluetooth b/Documentation/ABI/stable/sysfs-class-bluetooth
> index 36be02471174..c1024c7c4634 100644
> --- a/Documentation/ABI/stable/sysfs-class-bluetooth
> +++ b/Documentation/ABI/stable/sysfs-class-bluetooth
> @@ -7,3 +7,16 @@ Description: 	This write-only attribute allows users to trigger the vendor reset
>  		The reset may or may not be done through the device transport
>  		(e.g., UART/USB), and can also be done through an out-of-band
>  		approach such as GPIO.
> +
> +What:		/sys/class/bluetooth/btusb<usb-intf>/isoc_alt
> +Date:		13-Feb-2025
> +KernelVersion:	6.13
> +Contact:	linux-bluetooth@...r.kernel.org
> +Description:	This attribute allows users to configure the USB Alternate setting
> +		for the specific HCI device. Reading this attribute returns the
> +		current setting, and writing any supported numbers would change
> +		the setting. See the USB Alternate setting definition in Bluetooth
> +		core spec 5, vol 4, part B, table 2.1.
> +		If the HCI device is not yet init-ed, the write fails with -ENODEV.
> +		If the data is not a valid number, the write fails with -EINVAL.
> +		The other failures are vendor specific.

Again, what's wrong with libusb/usbfs to do this today?


> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1caf7a071a73..e2fb3d08a5ed 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;

Ah, so now this structure's lifecycle is determined by the device you
just embedded in it?  Are you sure you got this right?

>  };
>  
>  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,36 @@ 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)
> +{
> +	struct btusb_data *data = dev_get_drvdata(dev);

That feels wrong, it's embedded in the device, not pointed to by the
device.  So this should be a container_of() call, right?

> +
> +	kfree(data);
> +}
> +
> +static const struct device_type btusb_sysfs = {
> +	.name    = "btusb",
> +	.release = btusb_sysfs_release,
> +	.groups  = btusb_sysfs_groups,
> +};
> +
> +static struct attribute *btusb_sysfs_isoc_alt_attrs[] = {
> +	&dev_attr_isoc_alt.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(btusb_sysfs_isoc_alt);
> +
> +static const struct device_type btusb_sysfs_isoc_alt = {
> +	.name    = "btusb",
> +	.release = btusb_sysfs_release,
> +	.groups  = btusb_sysfs_isoc_alt_groups,
> +};
> +
>  static int btusb_probe(struct usb_interface *intf,
>  		       const struct usb_device_id *id)
>  {
> @@ -3743,7 +3778,7 @@ static int btusb_probe(struct usb_interface *intf,
>  			return -ENODEV;
>  	}
>  
> -	data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -3766,8 +3801,10 @@ static int btusb_probe(struct usb_interface *intf,
>  		}
>  	}
>  
> -	if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep)
> -		return -ENODEV;
> +	if (!data->intr_ep || !data->bulk_tx_ep || !data->bulk_rx_ep) {
> +		err = -ENODEV;
> +		goto out_free_data;
> +	}
>  
>  	if (id->driver_info & BTUSB_AMP) {
>  		data->cmdreq_type = USB_TYPE_CLASS | 0x01;
> @@ -3821,16 +3858,47 @@ static int btusb_probe(struct usb_interface *intf,
>  
>  	data->recv_acl = hci_recv_frame;
>  
> +	if (id->driver_info & BTUSB_AMP) {
> +		/* AMP controllers do not support SCO packets */
> +		data->isoc = NULL;
> +	} else {
> +		/* Interface orders are hardcoded in the specification */
> +		data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> +		data->isoc_ifnum = ifnum_base + 1;
> +	}
> +
> +	if (id->driver_info & BTUSB_BROKEN_ISOC)
> +		data->isoc = NULL;
> +
> +	/* Init a dev for btusb. The attr depends on the support of isoc. */
> +	if (data->isoc)
> +		data->dev.type = &btusb_sysfs_isoc_alt;
> +	else
> +		data->dev.type = &btusb_sysfs;

When walking the class, are you sure you check for the proper types now?
Does anyone walk all of the class devices anywhere?

> +	data->dev.class = &bt_class;
> +	data->dev.parent = &intf->dev;
> +
> +	err = dev_set_name(&data->dev, "btusb%s", dev_name(&intf->dev));

what does this name look like in a real system?  squashing them together
feels wrong, why is 'btusb' needed here at all?

> +	if (err)
> +		goto out_free_data;
> +
> +	dev_set_drvdata(&data->dev, data);
> +	err = device_register(&data->dev);
> +	if (err < 0)
> +		goto out_put_sysfs;
> +
>  	hdev = hci_alloc_dev_priv(priv_size);
> -	if (!hdev)
> -		return -ENOMEM;
> +	if (!hdev) {
> +		err = -ENOMEM;
> +		goto out_free_sysfs;
> +	}
>  
>  	hdev->bus = HCI_USB;
>  	hci_set_drvdata(hdev, data);
>  
>  	data->hdev = hdev;
>  
> -	SET_HCIDEV_DEV(hdev, &intf->dev);
> +	SET_HCIDEV_DEV(hdev, &data->dev);
>  
>  	reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
>  					GPIOD_OUT_LOW);
> @@ -3969,15 +4037,6 @@ static int btusb_probe(struct usb_interface *intf,
>  		hci_set_msft_opcode(hdev, 0xFD70);
>  	}
>  
> -	if (id->driver_info & BTUSB_AMP) {
> -		/* AMP controllers do not support SCO packets */
> -		data->isoc = NULL;
> -	} else {
> -		/* Interface orders are hardcoded in the specification */
> -		data->isoc = usb_ifnum_to_if(data->udev, ifnum_base + 1);
> -		data->isoc_ifnum = ifnum_base + 1;
> -	}
> -
>  	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_RTL) &&
>  	    (id->driver_info & BTUSB_REALTEK)) {
>  		btrtl_set_driver_name(hdev, btusb_driver.name);
> @@ -4010,9 +4069,6 @@ static int btusb_probe(struct usb_interface *intf,
>  			set_bit(HCI_QUIRK_FIXUP_BUFFER_SIZE, &hdev->quirks);
>  	}
>  
> -	if (id->driver_info & BTUSB_BROKEN_ISOC)
> -		data->isoc = NULL;
> -
>  	if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
>  		set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>  
> @@ -4065,10 +4121,6 @@ static int btusb_probe(struct usb_interface *intf,
>  						 data->isoc, data);
>  		if (err < 0)
>  			goto out_free_dev;
> -
> -		err = device_create_file(&intf->dev, &dev_attr_isoc_alt);

You have now moved the file, are you sure you don't also need to update
the documentation?


> -		if (err)
> -			goto out_free_dev;
>  	}
>  
>  	if (IS_ENABLED(CONFIG_BT_HCIBTUSB_BCM) && data->diag) {
> @@ -4099,6 +4151,16 @@ static int btusb_probe(struct usb_interface *intf,
>  	if (data->reset_gpio)
>  		gpiod_put(data->reset_gpio);
>  	hci_free_dev(hdev);
> +
> +out_free_sysfs:
> +	device_del(&data->dev);
> +
> +out_put_sysfs:
> +	put_device(&data->dev);
> +	return err;
> +
> +out_free_data:
> +	kfree(data);
>  	return err;
>  }
>  
> @@ -4115,10 +4177,8 @@ static void btusb_disconnect(struct usb_interface *intf)
>  	hdev = data->hdev;
>  	usb_set_intfdata(data->intf, NULL);
>  
> -	if (data->isoc) {
> -		device_remove_file(&intf->dev, &dev_attr_isoc_alt);
> +	if (data->isoc)
>  		usb_set_intfdata(data->isoc, NULL);
> -	}
>  
>  	if (data->diag)
>  		usb_set_intfdata(data->diag, NULL);
> @@ -4150,6 +4210,7 @@ static void btusb_disconnect(struct usb_interface *intf)
>  		gpiod_put(data->reset_gpio);
>  
>  	hci_free_dev(hdev);
> +	device_unregister(&data->dev);
>  }
>  
>  #ifdef CONFIG_PM
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05919848ea95..776dd6183509 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1843,6 +1843,7 @@ int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
>  
>  void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
>  
> +extern const struct class bt_class;
>  void hci_init_sysfs(struct hci_dev *hdev);
>  void hci_conn_init_sysfs(struct hci_conn *conn);
>  void hci_conn_add_sysfs(struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 041ce9adc378..aab3ffaa264c 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -6,9 +6,10 @@
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
>  
> -static const struct class bt_class = {
> +const struct class bt_class = {
>  	.name = "bluetooth",
>  };
> +EXPORT_SYMBOL(bt_class);

EXPORT_SYMBOL_GPL(), right?

thanks,

greg k-h

Powered by blists - more mailing lists